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

Enhancement: route_reverse does not parse query parameters #2650

Open
jorgeecardona opened this issue Nov 11, 2023 · 5 comments
Open

Enhancement: route_reverse does not parse query parameters #2650

jorgeecardona opened this issue Nov 11, 2023 · 5 comments
Labels
Enhancement This is a new feature or request Routing This is related to our routing

Comments

@jorgeecardona
Copy link

jorgeecardona commented Nov 11, 2023

Summary

The function route_reverse does not parse query parameters, adding that would be very useful.

Basic Example

The following example has a single router at / with two query parameters p and q but the route_reverse method does not add them.

A request to / returns just Hello, World at / which is not correct, it should return Hello, World at /?q=1. A request to /?p=1 should return Hello, World at /?p=1&q=1, i.e. the function should deal gracefully with the default values as well.

from litestar import Litestar, get, Request

@get("/", name="hello_world")
async def hello_world(request: Request, p: int = 0, q: int = 0) -> str:
    path = request.app.route_reverse("hello_world", p=p, q=q+1)
    return f"Hello, World at {path}"

app = Litestar([hello_world])

Drawbacks and Impact

Maybe people rely on the method not returning the query params. A flag controlling the removal of query parameters could be added for them.

Unresolved questions

No response


Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh Litestar dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@jorgeecardona jorgeecardona added the Enhancement This is a new feature or request label Nov 11, 2023
@provinzkraut
Copy link
Member

provinzkraut commented Nov 11, 2023

I'm not saying we won't add this, but for your example there's actually a better way to achieve this:

from litestar import Litestar, get, Request

@get("/", name="hello_world")
async def hello_world(request: Request, p: int = 0, q: int = 0) -> str:
    query = request.query_params
    query["q"] = q + 1
    path = request.url.with_replacements(query=query)
    return f"Hello, World at {path}"

app = Litestar([hello_world])

Conceptually, the reason why your approach doesn't work is because route_reverse is providing a path only, which does not have query parameters. The example I'm using here instead operates on the URL, which does.

@peterschutt
Copy link
Contributor

In flask's url_for() any of the kwargs that aren't matched to path params are appended to the url as query params. They also allow setting things like _anchor=....

@JacobCoffee JacobCoffee added the Routing This is related to our routing label Dec 7, 2023
@jjgalvez
Copy link

jjgalvez commented Apr 4, 2024

I'd like to add to the request for this. I recently had a use case for a url that had both path parameters and query parameters. I ended up monkey patching route_reverse (I still need to check if I have to monkey patch url_for too) to allow route_reverse to allow for both query_parameters and path_parameters. I'm sure there are significantly betters ways to do this but here is my monkey patch:

from litestar import Litestar
from typing import Any

Litestar.route_reverse_org = Litestar.route_reverse

def route_reverse(self, name:str, 
                query_parameters: dict[str, str] | None = None, 
                path_parameters: dict[str, str] | None = None, 
                **other_path_parameters: Any) -> str:

    p = {}
    if path_parameters:
        p.update(path_parameters)
    if other_path_parameters:
        p.update(other_path_parameters)

    base_path = self.route_reverse_org(name=name, **p)

    if query_parameters:
        q = []
        for k,v in query_parameters.items():
            q.append(f'{k}={v}')
        return f'{base_path}?{"&".join(q)}'
    return base_path

Litestar.route_reverse = route_reverse

@peterschutt
Copy link
Contributor

We have the option of trying to migrate to a pattern like this, which allows us to be specific about where the supplied components live in the url, and also allow us to have names in the function signature that might conflict with path/query parameter names:

url = route_reverse("my_route", path={"scheme": "something"}, query={"anchor": "something"}, scheme="https", anchor="page_section")

Or, we can head in the same direction that flask's url_for has gone, and assume that any **kwargs that cannot be assigned to a path parameter is a query parameter (and maybe go so far as to have names like _scheme and _anchor in the signature to minimize their likelihood of conflict with path/query parameter names.

I feel like 1st is more correct, but maybe less convenient to use while 2nd is less perfect but more convenient to use.

@jjgalvez
Copy link

jjgalvez commented Apr 6, 2024

my vote would be for essentially the first proposal, although I would suggest changing the first parameters name from name to route_name to avoid collisions with common parameters, as well as not using the names path and query directly (using either _path _query or path_parameters query_parameters). to retain backward comparability I would retain **kwargs as only path parameters.
I'm confused about including schema and anchor. For schema shouldn't it inherit the applications schema? and could be different depending on deployment. For example http in dev https in productions? Also not sure about the usecase for anchor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request Routing This is related to our routing
Projects
Status: Ideas
Development

No branches or pull requests

5 participants