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

No way to replace path without replacing query #111

Closed
Lucretiel opened this issue Sep 27, 2017 · 12 comments · Fixed by #1421
Closed

No way to replace path without replacing query #111

Lucretiel opened this issue Sep 27, 2017 · 12 comments · Fixed by #1421

Comments

@Lucretiel
Copy link

The following is unexpected behavior:

url1 = URL("http://www.example.com/foo?bar=10")
url2 = url1.with_path("/bar")

assert url1.query = url2.query  # Assertion fails

While I understand the design decision to treat the query and fragment components as part of the path, this has left us without an (efficient) way to preserve the query and fragment components when replacing just the path. The only way to do it is:

url2 = url1.with_path(new_path).with_query(url1.query_string).with_fragment(url1.fragment)

This is a total of 4 extra method calls, at least of which does a lot of unnecessary processing. I'm assuming that there's code out there that depends on this weird behavior, so I'd like to propose and, with permission, implement a new method, with_just_path, which replaces the path but leaves the fragment and query the same.

@asvetlov
Copy link
Member

Did you try URL.build?

@Lucretiel
Copy link
Author

Lucretiel commented Sep 27, 2017

I mean, that would work, but why even have the with_* methods in the first place? This seems like it'd fit in well with the other with methods. It's even MORE verbose, because I have to list out every single field in the URL in the build call.

@asvetlov
Copy link
Member

Honestly I don't want to blow up URL class with new methods.
Also I suspect your need is very rare: usually query/fragment is described by path, different paths have different allowed query params.
The only exception is passing in query some constant information like security token, it could be done by url.update_query() now.

@Lucretiel
Copy link
Author

My need is in use for a web framework, which is why I'm interested in reducing the amount of unnecessary work implied by with_query. In particular, I'm working on django-style path routing where the front of a path is cut off when it's passed to child route views. It's not exactly a very common use case, but I can imagine it'd be common for creating web frameworks based on yarl.

@asvetlov
Copy link
Member

Thus do you need cutting first path symbols from relative URL?
Make sense.
Maybe create a special method for this case?

@eternal-sorrow
Copy link

eternal-sorrow commented Jan 18, 2021

Then URL.with_host() should cut off the path, query and fragment altogether. Why the inconsistency? It doesn't make sense.

>>> URL('https://foo.tld/path?a=1#fragment').with_host('bar.tld')
URL('https://bar.tld/path?a=1#fragment')
>>> URL('https://foo.tld/path?a=1#fragment').with_path('/new/path')
URL('https://foo.tld/new/path')

@apalala
Copy link

apalala commented Apr 9, 2021

It is totally unexpected that .with_path() will remove query.

@JP-Ellis
Copy link

JP-Ellis commented Oct 9, 2023

Working with this library, and got this unexpected behaviour as well. The docs quite clearly say that with_query should:

Return a new URL with path replaced, encode path if needed.

Is there any appetite for a PR?

@trim21
Copy link

trim21 commented Nov 14, 2024

is this considered as a bug or by-design?

@asvetlov
Copy link
Member

It is a design decision.
I hear the need for path replacement with saving the query though.
We cannot just change the behavior, it will break existing code. What we can do is change URL.with_path() method to URL.with_path(path, *, keep_query=False).
Passing keep_query=True saves an existing query string.
Does it sound good?

@trim21
Copy link

trim21 commented Nov 14, 2024

sounds good,maybe also fragment …

@paul-nameless
Copy link
Contributor

Hi, I made some changes to support this behavior. Could you please take a look? Thank you!
#1421

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.

7 participants