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

Abstract routing API sorts routes. #1941

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BorekZnovustvoritel
Copy link

Fixes #1879.

Changes proposed in this pull request:

Let me know if anything else needs adjustments.

dkoston added a commit to dkoston/connexion that referenced this pull request Oct 31, 2024
@dkoston
Copy link

dkoston commented Oct 31, 2024

Just an FYI that I've confirmed this fixes routing bugs in connexion that exist today.

For example, attempt to add these 2 routes:

paths:
 /users/{user_id}:
    get:
 /users/accept:
   get:

In 3.1.0 GET /users/accept is routed to GET /users/{user_id}

Not sure if there's more needed in this PR to get it merged but we've had to fork connexion until it will be as AsyncApp broke our routing.

LMK if I can help out with some test cases or whatever to get this rolling.

CC: @RobbeSneyders

@coveralls
Copy link

Coverage Status

coverage: 94.153% (+0.003%) from 94.15%
when pulling 8111037 on BorekZnovustvoritel:sort-routes-api
into 7c4fcc4 on spec-first:main.

@RobbeSneyders
Copy link
Member

@dkoston adding a test would be very much appreciated and will prevent us from accidentally breaking the behavior in the future.

@dkoston
Copy link

dkoston commented Nov 4, 2024

@RobbeSneyders is there any documentation on how to run the test suite?

I added some tests but running the lines from .github/workflows/pipline.yml results in
almost all tests failing even though I haven't changed any code other than adding a test.

pip install --upgrade pip
pip install "poetry<2" "tox<4" "tox-gh-actions<3" "coveralls<4"
tox

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.

RoutingMiddleware should sort paths
4 participants