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 · 8 comments
Open

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

FelixSchwarz opened this issue Dec 10, 2024 · 8 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?

@chrisinmtown
Copy link
Contributor

I checked https://docs.python.org/3/whatsnew/3.13.html and didn't see the word "sort" anywhere. Maybe I'm being unrealistic, but I think it's important to understand what changed. That knowledge could/should guide the fix to the test case, and hopefully in a way that the test case will keep working in 3.14 and beyond.

@chrisinmtown
Copy link
Contributor

Python 3.13 does the call as well but checks /users/me/{path:path} < /users/{username}/items/{path:path}

@FelixSchwarz I'm trying to understand this, I only see /users/me/{path:path} in this one place, is that right?

@FelixSchwarz
Copy link
Contributor Author

@chrisinmtown I also did look for sorting changes in Python 3.13 but did not find something obvious beside "optimized sort functions". Of course it would be nice to know what caused the change.

However the test code currently relies on undefined behavior: As I mentioned in the first post, there is no total ordering between these routes (A >= B and B >= A). So the proposed adaption is actually more generic and probably "more correct".

I'm trying to understand this, I only see /users/me/{path:path} in this one place, is that right?

Not sure I understand your question. Could you try to rephrase it?

@chrisinmtown
Copy link
Contributor

chrisinmtown commented Jan 28, 2025

Not sure I understand your question. Could you try to rephrase it?

@FelixSchwarz all I was trying to say is, I don't see path /users/me/{path:path} in the test results anywhere, but I do see that you mentioned it in your comment above. Am I blind?

Also I'm struggling with your statement "there is no total ordering between these routes". I think there's a total ordering for all strings :) what is the specific problem here? Maybe it's just too early in my day right now :)

@FelixSchwarz
Copy link
Contributor Author

Ah, I now I see what you mean. Connexion automatically appends "/{path:path}" to each route in sorting (see SortableRoute constructor).

Also I'm struggling with your statement "there is no total ordering between these routes". I think there's a total ordering for all strings :) what is the specific problem here? Maybe it's just too early in my day right now :)

Nah, this was just me being sloppy (and just recovering from a week of illness). What I meant is that utils.sort_routes(routes) internally builds SortableRoute instances from these strings. Because of their specific __lt__ implementation it can happen that neither "A < B" is true nor "B < A" (and of course "A != B") when it comes to SortableRoute. That violates "total ordering"

As @RobbeSneyders wrote, there is no real order between certain paths so the behavior above is not a problem. The only problem then is how to encode the different outcomes in a test.

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

3 participants