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

Bug: Routing based on path parameter data type is not using first matching route handler #3622

Open
1 of 4 tasks
fernhaven opened this issue Jul 11, 2024 · 4 comments
Open
1 of 4 tasks
Labels
Bug 🐛 This is something that is not working as expected

Comments

@fernhaven
Copy link

fernhaven commented Jul 11, 2024

Description

Was told Litestar is intended to route differently based on the type of the path parameter:

Q: Is Litestar intended to route differently based on the type of the path parameter
A: ... this only works if the parameters you pass in are actually compatible and distinct, e.g. using a str is basically a catch-all, so if you provide that last, it will always match

However, when providing route handlers in the order of int path param handler followed by str path param handler, the str path param handler is the only one that is called in the example code below.

URL to code causing the issue

No response

MCVE

from litestar import Litestar, get


@get("/{id:int}")
async def hello_world_int(id: int) -> str:
    return f"Hello, world (int)! {id}"


@get("/{id:str}")
async def hello_world_str(id: str) -> str:
    return f"Hello, world (str)! {id}"

# Expect that path "/42" would route to hello_world_int
# and path "/abc" would go to hello_world_str
app = Litestar([hello_world_int, hello_world_str])

Steps to reproduce

1. Run app: `litestar run`
2. In browser access: 127.0.0.1:8000/42
3. Result: Hello, world (str)! 42  
  Error: (expected to see "(int)" rather than "(str)"
4. In browser access: 127.0.0.1:8000/ABC
5. Result (as expected): Hello, world (str)! ABC

Screenshots

No response

Logs

No response

Litestar Version

2.9.1

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

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 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
@fernhaven fernhaven added the Bug 🐛 This is something that is not working as expected label Jul 11, 2024
@chyvak357
Copy link

I faced the same problem. I'm trying to resolve uuid and int as path parameter, but in all cases only the first declared function will be called.

@IvanovCosmin
Copy link

I managed to reproduce this issue with the following unit test.
I am currently attempting understanding the issue, and hopefully I will come up with a fix

from litestar import get
from litestar.testing.helpers import create_test_client

def test_conflicting_handlers() -> None:
    @get("/{id:int}")
    async def hello_world_int(id: int) -> str:
        return f"Hello, world (int)! {id}"


    @get("/{id:str}")
    async def hello_world_str(id: str) -> str:
        return f"Hello, world (str)! {id}"

    with create_test_client(route_handlers=[hello_world_int, hello_world_str]) as client:
        response = client.get("/42")
        assert response.status_code == 200
        assert "int" in response.text # FAILED

        response = client.get("/ABC")
        assert response.status_code == 200
        assert "str" in response.text

@IvanovCosmin
Copy link

IvanovCosmin commented Sep 11, 2024

Hi @provinzkraut @Goldziher I need your guidance to come up with a solution. Please guide me towards someone else if you are not the right people to speak to. Also please guide me to the forum where we should have this discussion.

The problem here is that the RouteTrieNode add_route_to_trie doesn't differentiate between types of path parameters. When one is encountered it adds a PathParameterSentinel that can only be unique per parent node.

# this is the line that checks if a PathParameterSentinel is already in the children of the parent.
# on the second route it is already there and a new node is not created
if next_node_key not in current_node.
    current_node.children[next_node_key] = create_node(path_template=route.path_format)

Now that I understand the problem there are 2 solutions:

  1. Introduce support for multiple path parameters differentiated by type.
  • This will require replacing PathParameterSentinel with types for each of the supported parameter type. (ex: PathParameterSentinel.STR, PathParameterSentinel.INT, etc.)
  • I have already started working on it. The solution became a little bit complex, but will likely be a cool feature in the end.
  • I can submit a draft PR if you guys are ok with this.
  1. Introduce validations for routes overriding eachother (the won't fix)
  • There should be at least some sort of error telling the user that this is not something that is supported.
  • There doesn't seem to be any validation at all on overriding routes, the traversal code seems to just delete the second route in the static route case. I can't talk about other types of routes.

My vote is against introducing this new feature, and instead work on proper validations.
With my understanding of the vision of Litestar is that parsing and validation parameters should be encoded at the level of the handler's function signature. You shouldn't have to have 2 routes for 2 different handlers. I think that for this case we should have some sort of union parameter, like this

@get("/{id}")
async def hello_world(id: int | str) -> str:
    return f"Hello, world! {id}"

app = Litestar([hello_world])

@provinzkraut
Copy link
Member

Thanks for the investigation here @IvanovCosmin!

My vote is against introducing this new feature, and instead work on proper validations.

I agree. That seems to be the proper way to handle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 This is something that is not working as expected
Projects
None yet
Development

No branches or pull requests

4 participants