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

Test suite fails with Python 3.13 due to route sorting #2012

Open
FelixSchwarz opened this issue Dec 10, 2024 · 3 comments
Open

Test suite fails with Python 3.13 due to route sorting #2012

FelixSchwarz opened this issue Dec 10, 2024 · 3 comments

Comments

@FelixSchwarz
Copy link
Contributor

I tried to run the test suite locally using Python 3.13 but there is a test failure in tests/test_utils.py::test_sort_routes:

=================================== test session starts ====================================
platform linux -- Python 3.13.1, pytest-8.3.4, pluggy-1.5.0 -- …/workspace/connexion/.tox/py313-pypi/bin/python
cachedir: .pytest_cache
rootdir: …/workspace/connexion
configfile: pyproject.toml
plugins: anyio-4.5.2, Faker-33.1.0, asyncio-0.18.3, cov-2.12.1
asyncio: mode=Mode.AUTO
collected 1 item                                                                           

tests/test_utils.py::test_sort_routes FAILED

========================================= FAILURES =========================================
_____________________________________ test_sort_routes _____________________________________
tests/test_utils.py:114: in test_sort_routes
    assert utils.sort_routes(routes) == expected
E   AssertionError: assert ['/users/{username}/items/{item}', '/users/me', '/users/{username}/items', '/users/{username}'] == ['/users/me', '/users/{username}/items/{item}', '/users/{username}/items', '/users/{username}']
E     
E     At index 0 diff: '/users/{username}/items/{item}' != '/users/me'
E     
E     Full diff:
E       [
E     +     '/users/{username}/items/{item}',
E           '/users/me',
E     -     '/users/{username}/items/{item}',
E           '/users/{username}/items',
E           '/users/{username}',
E       ]
================================= short test summary info ==================================
FAILED tests/test_utils.py::test_sort_routes - AssertionError: assert ['/users/{username}/items/{item}', '/users/me', '/users/{username}/items', '/users/{username}'] == ['/users/me', '/users/{username}/items/{item}', '/users/{username}/items', '/users/{username}']
  
  At index 0 diff: '/users/{username}/items/{item}' != '/users/me'
  
  Full diff:
    [
  +     '/users/{username}/items/{item}',
        '/users/me',
  -     '/users/{username}/items/{item}',
        '/users/{username}/items',
        '/users/{username}',
    ]
==================================== 1 failed in 0.13s =====================================

I am not sure if this is actually a problem with starlette routing or just a insignificant different but I guess, /users/me should really be the first route.

My current guess is that Python 3.13 changed its sorting behavior.

Python 3.12 calls /users/{username}/items/{path:path} < /users/me/{path:path} which results in False so /users/me is first in the final result.

Python 3.13 does the call as well but checks /users/me/{path:path} < /users/{username}/items/{path:path} immediately after (returns also False) which means there is no real order.

So my questions are:

  • Can you reproduce the Python 3.13 problem as well or is it just something on my side?
  • Is the different route sorting an actual problem?
  • If so, any ideas how to fix it?
@RobbeSneyders
Copy link
Member

Thanks for the report @FelixSchwarz!

I haven't been able to test it yet, but as long as the result is deterministic, I don't think this is an issue. As you mention there is no real order between these paths.

There are two things this sorting should achieve:

  • More specific paths are sorted before less specific paths. eg:
    • users/me should come before users/{username}
    • /users/{username}/items/special should come before /users/{username}/items/{item}
  • The sorting should be deterministic, so the result is the same if we use it in multiple places

So 2 questions for you:

  • Do you always get the same sorting error when running the test suite on 3.13?
  • Is this the only error when running into 3.13?

If so, we can add a test suite for 3.13 and change the test to just validate the two requirements I listed above instead of checking against a fixed sorting order.

@FelixSchwarz
Copy link
Contributor Author

Do you always get the same sorting error when running the test suite on 3.13?

Yes.

Is this the only error when running into 3.13?

I don't think so but this was the clearest issue for me. If I am a bit lucky, I can work on this issue over the next days/few weeks.

Afterwards I'd try to clear all warnings and then proceed with Python 3.14 so we stay ahead of the curve. We already had the first rebuilds for 3.14 (alpha obviously) in Fedora which should help adapting the code of dependencies which in turn should make it easier to support 3.14 in Connexion.

@FelixSchwarz
Copy link
Contributor Author

I just did a retest and actually it looks like the route sorting is the really the only issue (though two different tests fail).

I am not yet sure how to adapt the test. Right now I can think of two possibilities:

routes = [
    "/users/{username}",
    "/users/me",
    "/users/{username}/items",
    "/users/{username}/items/{item}",
]
sorted_routes = utils.sort_routes(routes)
assert sorted_routes.index("/users/me") < sorted_routes.index("/users/{username}")
assert sorted_routes.index("/users/{username}/items/{item}") < sorted_routes.index("/users/{username}/items")
assert sorted_routes.index("/users/{username}/items") < sorted_routes.index("/users/{username}")

Or a more generic solution:

import sys
expected_before_py313 = [
    "/users/me",
    "/users/{username}/items/{item}",
    "/users/{username}/items",
    "/users/{username}",
]
expected_since_py313 = [
    "/users/{username}/items/{item}",
    "/users/me",
    "/users/{username}/items",
    "/users/{username}",
]
expected = expected_before_py313 if sys.version_info < (3, 13) else expected_since_py313
assert sorted_routes == expected

While the first approach encapsulates the "more specific routes first", I find it harder to understand and it might be error prone. The second approach is just more lines of code and also looks ugly.

Or maybe a "hybrid" approach:

routes = [
    "/users/{username}",
    "/users/me",
    "/users/{username}/items",
    "/users/{username}/items/{item}",
]
sorted_routes = utils.sort_routes(routes)
# Python 3.13 changed the sorting behavior and there is also not a real order
# between "/users/me" and "/users/{username}/items/{item}". Therefore we
# just check that /user/me is before /users/{username} (more specific routes
# need to come first) and then check the rest.
assert sorted_routes.index("/users/me") < sorted_routes.index("/users/{username}")
expected_users_username =  [
    "/users/{username}/items/{item}",
    "/users/{username}/items",
    "/users/{username}",
]
assert sorted_routes == expected_users_username

I think I somehow like the last approach best because the "variable" part (/users/me) is handled explicitly but you can still read the expected order of the remaining routes.

@RobbeSneyders Any opinion which approach I should follow? Any other suggestion?

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

No branches or pull requests

2 participants