starlette
starlette copied to clipboard
encode
Add `Request.url_for` query parameters
Simple implementation for #560 to start a discussion.
url_for only supports path parameters and in some scenarios like pagination, it would be useful to add query parameters to it.
and based on the discussion in the issue we could do one of the following:
-
We can make
url_forreturn aURLinstance that could use the existinginclude_query_params. I think this won't be a backwards-compatible change, as we're changing the signature ofurl_for. -
We could also do something like Flask does, use anything that matches as path parameters, and use the rest as query params. I don't think. this will be a backwards-compatible change either (Not 100% sure but I guess this will have a more limited impact but still there's a chance some clients might depend on
NoMatchFoundinstead of getting query params).
Currently the PR is just passing query_params to url_for to make this work but I think the second approach seems reasonable. If we reach a conclusion here, I'll update this PR.
Mutually exclusive with #1364
I think the second approach would make more sense anyway.
I am curious about one thing: Why not just create URL based on the returned results of url_for? Then let users add query parameters by themselves. It seems more concise.
I am curious about one thing: Why not just create URL based on the returned results of url_for? Then let users add query parameters by themselves. It seems more concise.
Well It is possible. But it could be a useful feature, specially in the Jinja2 templating and less verbose.
I am curious about one thing: Why not just create URL based on the returned results of url_for? Then let users add query parameters by themselves. It seems more concise.
I too much prefer the explicit approach (as proposed by @tomchristie in https://github.com/encode/starlette/issues/560#issuecomment-550507781). This gives peace of mind that one did not accidentally mix around some path and query parameters.
If we want something less verbose we might consider a shorthand for this operation like URL.q(...).
I am curious about one thing: Why not just create URL based on the returned results of url_for? Then let users add query parameters by themselves. It seems more concise.
I too much prefer the explicit approach (as proposed by @tomchristie in #560 (comment)). This gives peace of mind that one did not accidentally mix around some path and query parameters.
If we want something less verbose we might consider a shorthand for this operation like
URL.q(...).
I'm not really sure if I understand correctly.
The comment by Tom Christie, which you're referring to, is suggesting to change the signature of url_for to return URL instead of string. That way we can add extra methods like he explained.
But the comment you've quoted is suggesting that we do not change anything and leave to the end users to add query parameters to the url_for result, like using with urllib or URL from Starlette.
I am curious about one thing: Why not just create URL based on the returned results of url_for? Then let users add query parameters by themselves. It seems more concise.
I too much prefer the explicit approach (as proposed by @tomchristie in #560 (comment)). This gives peace of mind that one did not accidentally mix around some path and query parameters.
If we want something less verbose we might consider a shorthand for this operation like
URL.q(...).I'm not really sure if I understand correctly. The comment by Tom Christie, which you're referring to, is suggesting to change the signature of
url_forto returnURLinstead of string. That way we can add extra methods like he explained.But the comment you've quoted is suggesting that we do not change anything and leave to the end users to add query parameters to the
url_forresult, like using withurlliborURLfrom Starlette.
Maybe I didn't express myself well enough. I mean by this is that just wrap the previous url_for method to return a URL, and then the user adds or modifies the parameters themselves. Like this:
request.url_for("endpoint_name", key=value).include_query_params(name=value).__str__()
@abersheeran thanks for the clarification.
This is exactly the first approach and what Tom Christie mentioned. But that will be a backwards-incompatible change, I think.
If it's decided we can do it I'll update the PR.
Reviewing what Flask and Django do here...
- Django
reverse()does not support query parameters - https://docs.djangoproject.com/en/4.0/ref/urlresolvers/ - Flask
url_for(name, **values)automatically includes any unknown variable arguments as query parameters - https://flask.palletsprojects.com/en/2.0.x/api/#flask.url_for
I guess my preference would be if it already returned a URL instance, rather than a string.
But given that it doesn't it's prob okay to just match flask's style here, right?
I guess my preference would be if it already returned a
URLinstance, rather than a string. But given that it doesn't it's prob okay to just match flask's style here, right?
Yes, I think that makes sense.
I guess my preference would be if it already returned a
URLinstance, rather than a string. But given that it doesn't it's prob okay to just match flask's style here, right?
Given that both would be a breaking change I would strongly prefer the former. Why would both changes be breaking? Well it is a feasible use case to call url_for and catch the routing.NoMatchFound exception without wanting to get a url with query params.
Also if we adopt the flask style there are certain ambiguities like what should happen in the following case:
async def homepage(request):
return JSONResponse({'hello': request.url_for("homepage", foobar="321")})
routes = [
Route("/", endpoint=homepage),
Route("/{foobar}", endpoint=homepage)
]
app = Starlette(debug=True, routes=routes)
Should it return /?foobar=321 because it is the first or /321 because it is a path component and not a query param?
However if we choose to return an URL instance there is no such confusing/unexpected behaviour. Apart from that the most common use cases I can think of (RedirectResponse and templating (e.g. Jinja)) already support URL or simply convert it to a string. And if there are other use cases where this does not directly work it is trivial to write str(r.url_for(...)).
Given that both would be a breaking change I would strongly prefer the former
I totally agree that returning a URL from url_for would be clear and explicit. But as mentioned in the PR description I don't think the two approaches have really the same impact for the breaking change. Changing the signature would impact a lot more than adding query params behaviour to existing url_for.
And I think Jinja2 would already call __repr__ on the URL instance, so that when a URL is returned, that can be handled by starlette out of the box.
I totally agree that returning a
URLfromurl_forwould be clear and explicit. But as mentioned in the PR description I don't think the two approaches have really the same impact for the breaking change. Changing the signature would impact a lot more than adding query params behaviour tourl_for.
Could this be handled as a deprecation? It seems like the general consensus is that the API returning URL is better long term, so I would hate to see the better API locked out for backwards compatibility given that Starlette is <1.0.0 and has released minor breaking changes like this in the past (that is, I expect Starlette users to be at least somewhat flexible to small breaking changes like this one)
@adriangb I'm not sure how the deprecation will help. Now we are in 0.17.0 will we deprecate it in 0.18.0 or 1.0.0? I mean if we were on a major release like 1.17.0 it would make sense to deprecate in 2.0.0 but as you said since we don't have a major release, practically this can change.
Even if it's not required, it can soften the blow. The other pro is you might get someone pop into the discussion with good feedback because they saw the warning. For what it's worth, I would do something like deprecate in 0.18.0 and change in 0.19.0 or some other pre 1.0 release. I think there's precedent for this with startup events / lifespan right?
Looks like there's two options being discussed:
url_for(name: str, **path_params: Any, query_params: dict[str, Any] | None = None) -> str
url_for(name: str, **path_params: Any) -> URL
Bringing in #611 I prefer url_for(/, name: str, **path_params: Any) -> URL
I think there's also the possibility of doing something like Flask does:
url_for(name: str, **params: Any) -> str
To use any existing params as path parameters and use the remaining as query parameters.
I think there's also the possibility of doing something like Flask does:
url_for(name: str, **params: Any) -> strTo use any existing params as path parameters and use the remaining as query parameters.
Yes though that is ambiguous in some cases (https://github.com/encode/starlette/pull/1385#issuecomment-1007445543) and may lead to confusion. Even if deciding for one variant in ambiguous cases there would be no way to archive the opposite way if one requires that.
Actually, the question I wanted to ask is: "what's the deprecation path mentioned above?"
Hmm yeah good point since it would be a return value being changed it's hard to warn users, I can't think of any good way to deprecate it. Putting a warning in there would be super noisy.
Reusing Starlette URL class is a no-brainer here, IMO.
This makes URL construction a breeze and have no limits:
url_for('welcome').include_query_params(greet=1)
url_for('welcome').q(greet=1)
<a href="{{ url_for('welcome').include_query_params(greet=1) }}">welcome</a>
<a href="{{ url_for('welcome').q(greet=1) }}">welcome</a>
This is also very intuitive and have good DX. If we decide to improve API of URL class, then we have nothing to do with url_for as these are the same thing.
A solution with "magic" **params is not obvious, complex to implement, and can hide user typos.
As we go for 1.0, we can tolerate this API change.
I've read the thread again, and it looks like we are more in favor of having a URL returned, and I don't think there's any doubt about that. The only thought is "how to be user-friendly on the behavior change" - which I don't think there's a nice way.
Also, notice that this doesn't break jinja2 templates.
I'm fine with returning URL, and making it a breaking change without deprecation warning. Anyone against or have a better idea?
I use this approach quite a long time and the one downside I have is that I have to call str(request.url) when I need to pass current URL in headers (I use htmx, this is a common pattern these). Also, URL is not JSON-serializable.
But, in templates, it is a breeze of fresh air.
<a href="{{ request.url.include_query_params(search='term') }}">home</a>
It is a way simpler than making and using a dict in the endpoint callable or template.
I'm interested in this as well, mostly for template, using the jinja2 url_for most of the time
but this PR is missing to update the signature in here: https://github.com/encode/starlette/blob/337ae243b10d3c20db53b77bc3c7718bb4f1a164/starlette/templating.py#L82
