Copilot commented on code in PR #13820: URL: https://github.com/apache/trafficserver/pull/13820#discussion_r4246199007
########## tests/gold_tests/query/replay/query-pass-through.replay.yaml: ########## @@ -0,0 +1,135 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +meta: + version: "1.0" + +autest: + description: 'Verify QUERY requests are proxied with their content' + + server: + name: 'server' + + client: + name: 'client' + + ats: + name: 'ts' + process_config: + enable_cache: true + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'http_trans|http_cache' + remap_config: + - from: "http://example.com/"; + to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"; + metric_checks: + - metric: 'proxy.process.http.query_requests' + value: 3 + +sessions: +- transactions: + + # A QUERY with content is forwarded to the origin, content and all. + - client-request: + method: QUERY + url: /search + version: '1.1' + headers: + fields: + - [Host, example.com] + - [Content-Type, application/sql] + - [Content-Length, "23"] + - [uuid, query-first] + content: + data: "SELECT * FROM t WHERE a" + + proxy-request: + method: QUERY + headers: + fields: + - [Content-Length, {value: "23", as: equal}] Review Comment: This test claims it verifies that `QUERY` content is proxied, but it only asserts the `Content-Length` header. To prevent regressions where the proxy forwards headers but drops/changes the body, add an assertion that validates the forwarded request body content (e.g., verify the origin receives the expected payload, if ATS replay supports request-body verification). ########## src/proxy/http/HttpTransact.cc: ########## @@ -6945,10 +6947,15 @@ HttpTransact::is_request_retryable(State *s) return false; } + // A QUERY is safe, but unlike the other safe methods it carries content. Once + // the request has gone out we can only replay it if that content was + // buffered. + bool const content_is_replayable = s->method != HTTP_WKSIDX_QUERY || s->state_machine->is_buffering_request_body; + // If safe requests are retryable, it should be safe to retry safe requests irrespective of bytes sent or connection state // according to RFC the following methods are safe (https://tools.ietf.org/html/rfc7231#section-4.2.1) // Otherwise, if there was no error establishing the connection (and we sent bytes)-- we cannot retry - if (!HttpTransactHeaders::is_method_safe(s->method) && s->current.state != CONNECTION_ERROR && + if ((!HttpTransactHeaders::is_method_safe(s->method) || !content_is_replayable) && s->current.state != CONNECTION_ERROR && s->state_machine->server_request_hdr_bytes > 0) { Review Comment: The PR description calls out nuanced retry semantics for `QUERY` (retry like safe methods, but only after send if the body can be replayed). There isn’t a gold test exercising this new branch (buffered vs non-buffered body) and it’s easy to regress because it depends on connection-error timing and buffering config. Consider adding a targeted replay test that forces a retry scenario and asserts: (1) `QUERY` retries when buffering is enabled and the request body is replayable, and (2) `QUERY` does not retry (or follows unsafe-method behavior) when buffering is disabled. ########## src/proxy/http/HttpTransact.cc: ########## @@ -6945,10 +6947,15 @@ HttpTransact::is_request_retryable(State *s) return false; } + // A QUERY is safe, but unlike the other safe methods it carries content. Once + // the request has gone out we can only replay it if that content was + // buffered. + bool const content_is_replayable = s->method != HTTP_WKSIDX_QUERY || s->state_machine->is_buffering_request_body; + // If safe requests are retryable, it should be safe to retry safe requests irrespective of bytes sent or connection state // according to RFC the following methods are safe (https://tools.ietf.org/html/rfc7231#section-4.2.1) // Otherwise, if there was no error establishing the connection (and we sent bytes)-- we cannot retry Review Comment: The comment references RFC 7231 and implies a fixed list of safe methods, but this PR updates safety semantics (RFC 9110 + draft `QUERY`). Please update the comment and link to match current semantics (and mention `QUERY`’s body replay constraint), so the code and documentation don’t diverge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
