Skip to content

Add test for HTTP::Request with resource string //#15546

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
miry:request-double-slash
May 4, 2025
Merged

Add test for HTTP::Request with resource string //#15546
straight-shoota merged 1 commit intocrystal-lang:masterfrom
miry:request-double-slash

Conversation

@miry
Copy link
Contributor

@miry miry commented Mar 9, 2025

While testing the awscr-signer library job, I noticed a change in the behavior of HTTP::Request#path when handling the case of //:

Old behavior:

request = HTTP::Request.new("GET", "//", HTTP::Headers.new)
request.path.should eq("/")

New behavior:

request = HTTP::Request.new("GET", "//", HTTP::Headers.new)
request.path.should eq("//")

I am trying to add more tests to cover changes from #15499

@miry miry changed the title wip: Add test to cover the double slach Add test to cover the double slach Mar 9, 2025
@straight-shoota
Copy link
Member

I think going from / to // in #15499 is actually a welcome change. Request#path shouldn't worry about URI semantics. It just needs to represent the request's original path as accurately as possible.

I might even question whether it makes a lot of sense to parse this through URI. Instead, it could just return the raw request target, capped at the first ? character. But that's a separate discussion.

So anyway for this PR I think we should just change the expected value to match the actual one and then we're good.

@miry miry force-pushed the request-double-slash branch from a9e16e4 to 7da7738 Compare March 13, 2025 21:38
@straight-shoota straight-shoota requested a review from a team May 1, 2025 20:27
@straight-shoota straight-shoota added this to the 1.17.0 milestone May 1, 2025
@straight-shoota straight-shoota changed the title Add test to cover the double slach Add test for HTTP::Request path // May 2, 2025
@straight-shoota straight-shoota merged commit 93c54bd into crystal-lang:master May 4, 2025
31 of 32 checks passed
@straight-shoota straight-shoota changed the title Add test for HTTP::Request path // Add test for HTTP::Request with resource string // May 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants