Repository navigation
Fix path building: encode values once and keep literal slashes - #24
Merged
Merged
Conversation
String.pathSafe percent-encoded each value, and urlRequest(in:) then assigned the joined path to URLComponents.path, which encodes again. A value like "a b" went out as "a%2520b". This has been the case since the first commit; 5077dd2 fixed the same problem for query and form values in 2019, but the path side was missed because every path test used plain values. - String.pathSafe encodes a value as one segment, including "/", "?" and "#", and keeps "." and ".." as encoded segments so URL resolution doesn't collapse them. - PathTemplate passes every component through a normalizer that keeps valid %XX escapes and encodes anything else not allowed in a path. The result is assigned to percentEncodedPath, so nothing is encoded twice and custom PathRepresentable values that return raw text don't trap. - Literal template text keeps its slashes and honours %XX escapes. - Form body encoding is byte-identical to before.
includesSlash: false removed whatever slash came before the value,
including one written in the template, so
"testing/\(path: \.name, includesSlash: false).test" built
"testingfirst.test". It now removes only a slash the template inserted
automatically, which is what the 2021 failing test on feature/path-slash-fix
expected.
Default interpolations are unchanged: each value is its own segment, so
"files/\(path: \.id).json" still builds "files/42/.json". Embed a value with
includesSlash: false ("files/42.json"). The PathTemplate docs and
Examples.md now state the rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes two long-standing bugs in how
PathTemplatebuilds request paths:a bwent out asa%2520b.includesSlash: falsedeleted slashes written in the template."testing/\(path: \.name, includesSlash: false).test"builttestingfirst.test.1. Encode path values exactly once
The bug had been there since the first commit.
String.pathSafeencoded each value, thenurlRequest(in:)assigned the joined path toURLComponents.path, which encodes again. In 2019,5077dd2("Possible fix for double-encoding issues") fixed the same problem for query and form values, but the path side was missed, because every path test used plain values. I found no issue, PR, or test that relied on the double encoding.String.pathSafeencodes a value as one segment, including/,?, and#. It keeps.and..as encoded segments so URL resolution doesn't collapse them.percentEncodedPath. The normalizer keeps valid%XXescapes and encodes anything else not allowed in a path. CustomPathRepresentablevalues that return raw text are now encoded instead of trapping.%XXescapes. Form body encoding is byte-identical to before.2. Keep literal slashes next to embedded values
This was the failing test added on
feature/path-slash-fixin January 2021, which never got a fix.includesSlash: falseremoved whatever slash came before the value. It now removes only a slash the template inserted automatically.Default interpolations are unchanged: each value is its own path segment, so
"files/\(path: \.id).json"still buildsfiles/42/.json. Embedding a value usesincludesSlash: false, givingfiles/42.json. ThePathTemplatedocs and Examples.md now state the rule. No existing test expectation changed.Behaviour changes to review
/inside aStringvalue is now sent as%2F. Before,"folder/file.txt"became two segments. This is the one change someone might have relied on deliberately. It matches RFC 3986 and OpenAPI, and it stops values from adding path structure. To insert a value that spans several segments, conform a type toPathRepresentableand return its separators unencoded.a%20bis now sent asa%2520b(a literal%20)./withincludesSlash: falsenow keep that slash. They previously lost it.Testing
url.absoluteStringandpercentEncodedPathfor:%, Unicode, embedded/and//,?,#,.and.., and a pre-encoded valuefiles/{id}.jsonfiles/42/.json, pinnedswift:6.0in Docker): 55 tests pass. Linux runs fewer suites becauseEndpointsMockingis Apple-only.Coverage (line, macOS)
PathTemplate.swift98.3%,Endpoint+URLRequest.swift97.0%,Array+Form.swift85.7%.Endpointsoverall is 74.9%; the low-coverage files aren't touched by this PR.