Change url_for signature to return a URL instance

[aminalaee]

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_for return a URL instance that could use the existing include_query_params. I think this won't be a backwards-compatible change, as we're changing the signature of url_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 NoMatchFound instead of getting query params).

---

Update: Based on the discussions it's been decided to change the url_for signature to return a URL instance which will enable the caller to include_query_params to the URL both in application and templates.

[Kludex]

Mutually exclusive with #1364

[aminalaee]

Mutually exclusive with #1364

I think the second approach would make more sense anyway.

[abersheeran]

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.

[aminalaee]

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.

[septatrix]

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(...).

[aminalaee]

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.

[abersheeran]

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.

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__()

[aminalaee]

@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.

[tomchristie]

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

[aminalaee]

Which one do you think would be more appropriate here? How do we decide?

[tomchristie]

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?

[aminalaee]

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?

Yes, I think that makes sense.

[septatrix]

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?

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(...)).

[aminalaee]

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.

[adriangb]

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 url_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)

[aminalaee]

@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.

[adriangb]

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?

[aminalaee]

Let's wait for @tomchristie input on this.

[Kludex]

What would be the new signature? @aminalaee @adriangb

[adriangb]

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

[aminalaee]

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.

[septatrix]

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.

Yes though that is ambiguous in some cases (#1385 (comment)) 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.

[Kludex]

Actually, the question I wanted to ask is: "what's the deprecation path mentioned above?"

[Kludex]

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?

[alex-oleshkevich]

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.

[aminalaee]

@Kludex I will update the PR then based on the agreement.

[euri10]

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:

starlette/starlette/templating.py

Line 82 in 337ae24

return request.url_for(name, **path_params)

[aminalaee]

@euri10 Thanks, I need to update the PR.

[aminalaee]

I think adding a shortcut method URL.q() in addition to the URL.include_query_params() can be useful.

And also I didn't see any mention of return type of url_for method in the docs to update, maybe we need to add a new section?

[Kludex]

Maybe we can add the Signature: url_for(name, **path_params) -> URL here https://www.starlette.io/routing/#reverse-url-lookups ?

[Kludex]

I think adding a shortcut method URL.q() in addition to the URL.include_query_params() can be useful.

The way it is right now doesn't solve the issue, right?

[aminalaee]

The way it is right now doesn't solve the issue, right?

Not sure what you mean, but this will solve the initial issue and query params can be added to the output of url_for.
About the q shortcut I think it has some pros, but it's also confusing because should it be shortcut to include_query_params or replace_query_params.

[Kludex]

It looks like all involved already agreed that this is a better API, our only concern was it being a breaking change.