Skip to content

improve URL matching algorithm in case of ambiguity #2898

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlexBrou
Copy link

@AlexBrou AlexBrou commented Mar 7, 2025

Summary

This is a solution for cases of ambiguity in the defined routes:

Example:
we have 2 routes:

  • /user/{user_id}
  • /user/myself

if i make a request to /user/myself, Starlette will match it to the /user/{user_id} route because it is defined first and it results in a Full Match. However, this is ambiguous and the result would be different if the endpoints were defined in a different order by the developer.
In my opinion, it shouldn't matter the order that they are defined in, Starlette should look for the best match in case of ambiguity.

Currently, it stops looking when it finds a full match.
I suggest it keeps looking for full matches and choses the one that is stricter, which is the lowest number of Path Parameters used

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@alex-oleshkevich
Copy link
Member

You have two alternative solutions here that don't involve code changes:

  1. reorganize routes in a way that parametrized routes go below static ones
  2. if user_id is int then mark it as follows user_id:int and Starlette will not match /myself in this case.

@AlexBrou
Copy link
Author

@alex-oleshkevich of course, reorganizing the routes was my solution for the app i was developing.
however, this is a silent problem. if someone defines 2 endpoints then that someone means for both of them to be accessed, not to have one consuming all chances of the other one being reached, and it is not clear at first why this problem happens (and working on big projects, refactoring stuff, fixing merge conflicts and all that this might occur).
this happened to me when i was refactoring a project that had about 60 endpoints.

even if this change is not merged, at least we should have a warning message saying "the endpoint /user/myself is unreachable due to the endpoint /user/{user_id}" . it would be helpful

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 this pull request may close these issues.

2 participants