Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(encoded_)set_path should bypass edit_segments #709

Closed
alandefreitas opened this issue Mar 8, 2023 · 0 comments · Fixed by #710
Closed

(encoded_)set_path should bypass edit_segments #709

alandefreitas opened this issue Mar 8, 2023 · 0 comments · Fixed by #710
Assignees

Comments

@alandefreitas
Copy link
Member

alandefreitas commented Mar 8, 2023

The library has the convention that "/" (string_view) -> {} (segments) and "/./" (string_view) -> {""} (segments). Functions that go through edit_segments (insert, replace, ...) apply prefixes as required to maintain the Sequence representation. Meanwhile, normalization might not respect the mapping from segments -> Sequence.

url::set_path and url::set_encoded_path, on the other hand, has always assumed it can reuse edit_segments but that's incorrect because it should set the complete path string to exactly what the user asked for instead of attempting to maintain their sequence representation. Not fixing this leads to weird results such as assert(url("").set_path("/./") == "/././").

Thus, set_path should think in terms of the path string and only include extra prefixes where invalid urls would result. On the other hand, edit_segments should only be used when thinking in terms of the sequence: when we want the mapping of the path to the sequence.

Besides these wrong results, making edit_segments do two different things is dangerous, setting the complete path at once is more efficient, and removing workarounds from edit_segments will also make it more efficient.

@alandefreitas alandefreitas changed the title set_path should bypass edit_segments (encoded_)set_path should bypass edit_segments Mar 8, 2023
@alandefreitas alandefreitas self-assigned this Mar 8, 2023
alandefreitas added a commit to alandefreitas/url that referenced this issue Mar 10, 2023
alandefreitas added a commit to alandefreitas/url that referenced this issue Mar 10, 2023
alandefreitas added a commit that referenced this issue Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant