JosiahWI commented on code in PR #12618:
URL: https://github.com/apache/trafficserver/pull/12618#discussion_r2469757133
##########
tests/gold_tests/headers/replays/normalized_ae_varied_transactions.replay.yaml:
##########
@@ -190,14 +209,23 @@ sessions:
delay: 100ms
server-response:
- <<: *404_response
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [ Transfer-Encoding, chunked ]
+ - [ Cache-Control, max-age=300 ]
+ - [ Content-Encoding, br ]
+ - [ Vary, Accept-Encoding ]
+ - [ Connection, close ]
+ - [ X-Response-Identifier, Br-Accept-Encoding ]
proxy-response:
status: 200
headers:
fields:
- - [ X-Response-Identifier, { value: Gzip-Accept-Encoding, as: equal } ]
- - [ X-Cache, { value: hit-fresh, as: equal } ]
+ - [ X-Response-Identifier, { value: Br-Accept-Encoding, as: equal } ]
+ - [ X-Cache, { value: miss, as: equal } ]
Review Comment:
Why shouldn't the client reply with the gzip encoded alternate from the
previous response as in the old revision?
##########
tests/gold_tests/headers/replays/normalized_ae_varied_transactions.replay.yaml:
##########
@@ -722,6 +751,7 @@ sessions:
fields:
- [ Transfer-Encoding, chunked ]
- [ Cache-Control, max-age=300 ]
+ - [ Content-Encoding, "br, gzip" ]
Review Comment:
This does not make sense. Just choose one or the other or leave it off. it
doesn't matter for the purpose of the test.
##########
tests/gold_tests/headers/replays/normalized_ae_varied_transactions.replay.yaml:
##########
@@ -110,16 +110,25 @@ sessions:
delay: 100ms
server-response:
- <<: *404_response
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [ Transfer-Encoding, chunked ]
+ - [ Cache-Control, max-age=300 ]
+ - [ Content-Encoding, deflate ]
+ - [ Vary, Accept-Encoding ]
+ - [ Connection, close ]
+ - [ X-Response-Identifier, Deflate-Accept-Encoding ]
proxy-response:
status: 200
headers:
fields:
- - [ X-Response-Identifier, { value: Empty-Accept-Encoding, as: equal }
]
- - [ X-Cache, { value: hit-fresh, as: equal } ]
+ - [ X-Response-Identifier, { value: Deflate-Accept-Encoding, as: equal
} ]
+ - [ X-Cache, { value: miss, as: equal } ]
Review Comment:
The implication of the test setup in the old revision is that the server
does not have a content representation with the `deflate` encoding. The server
response here should be a version of the resource with no content encoding, or
a 406 response. The 404 response used in the old revision seems ok for the
purpose of the test as well. In any case, the origin should not return a
deflate encoded resource here.
The interesting question here is whether ATS is conforming to Vary semantics
by replying out of cache with the empty response. Section 5.3.4 of RFC 4231
explicitly says that the **origin server** should respond with no encoding if
it can't supply a requested encoding. Together with the rules for caching
responses with a Vary header described in section 4.1 of RFC 7234, I believe
the old revision of this test may be nonconforming.
In summary, I think the correct expectation for the test, to minimize
unnecessary changes, should be:
- client requests deflate encoding
- server replies with 404 Not Found
- proxy forwards 404 Not Found (cache miss)
or alternatively:
- client requests deflate encoding
- server replies with 200 OK and no content encoding
- proxy forwards the 200 OK (cache miss)
##########
tests/gold_tests/headers/replays/normalized_ae_varied_transactions.replay.yaml:
##########
@@ -133,14 +142,23 @@ sessions:
delay: 100ms
server-response:
- <<: *404_response
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [ Transfer-Encoding, chunked ]
+ - [ Cache-Control, max-age=300 ]
+ - [ Content-Encoding, br ]
+ - [ Vary, Accept-Encoding ]
+ - [ Connection, close ]
+ - [ X-Response-Identifier, Br-Accept-Encoding ]
proxy-response:
status: 200
headers:
fields:
- - [ X-Response-Identifier, { value: Empty-Accept-Encoding, as: equal }
]
- - [ X-Cache, { value: hit-fresh, as: equal } ]
+ - [ X-Response-Identifier, { value: Br-Accept-Encoding, as: equal } ]
+ - [ X-Cache, { value: miss, as: equal } ]
Review Comment:
Similarly to the previous case, the server should return 404 Not Found here,
and the proxy should forward it.
##########
tests/gold_tests/headers/replays/normalized_ae_varied_transactions.replay.yaml:
##########
@@ -162,6 +180,7 @@ sessions:
fields:
- [ Transfer-Encoding, chunked ]
- [ Cache-Control, max-age=300 ]
+ - [ Content-Encoding, gzip ]
Review Comment:
This is not strictly necessary, but I suppose it doesn't hurt.
--
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]
