Skip to content

[16.0][IMP]fastapi: enable multi-slash routes#515

Merged
OCA-git-bot merged 4 commits into
OCA:16.0from
PyTech-SRL:16.0-fastapi-multislash-route
Jun 25, 2025
Merged

[16.0][IMP]fastapi: enable multi-slash routes#515
OCA-git-bot merged 4 commits into
OCA:16.0from
PyTech-SRL:16.0-fastapi-multislash-route

Conversation

@PicchiSeba

@PicchiSeba PicchiSeba commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

This is a Work In Progress PR, which attempts to address this issue #446.
Any suggestion is welcome

EDIT: depends on #524

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from e2a6cce to f7dda08 Compare April 1, 2025 09:00
@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from f7dda08 to efc5e40 Compare April 30, 2025 10:31
@PicchiSeba

PicchiSeba commented Apr 30, 2025

Copy link
Copy Markdown
Contributor Author

The error seems unrelated to this PR

AttributeError: module 'marshmallow' has no attribute 'pprint'

@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from efc5e40 to 78e8805 Compare April 30, 2025 12:11
@PicchiSeba PicchiSeba marked this pull request as ready for review April 30, 2025 12:13
@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch 3 times, most recently from c08daf1 to 66d4979 Compare May 5, 2025 12:59
Comment thread fastapi/models/fastapi_endpoint.py Outdated

@lmignon lmignon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Than you for the proposal @PicchiSeba

get_uid , get_app and get_endpoint no more receive a root path. They receive a path... The parameter name should be renamed. (see also my previous comment)

@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from 66d4979 to ebe773c Compare May 14, 2025 10:32
Comment thread fastapi/models/fastapi_endpoint.py Outdated

@lmignon lmignon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmignon

lmignon commented May 14, 2025

Copy link
Copy Markdown
Contributor

once #527 is merged, a rebase will solve the CI

@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from ebe773c to e5b9865 Compare May 15, 2025 07:18

@lmignon lmignon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this great improvement @PicchiSeba

LGTM (Code review, functionnal tests)

@PicchiSeba

Copy link
Copy Markdown
Contributor Author

Thank you for this great improvement @PicchiSeba

LGTM (Code review, functionnal tests)

Thank you for your support and technical expertize!

@lmignon

lmignon commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

ping @AnizR @sbidoul

@AnizR AnizR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the test class, the code seems good.
Thanks for your contribution!

Comment thread fastapi/tests/test_fastapi_multislash_demo.py Outdated
@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch 2 times, most recently from a90c2e5 to e00d3d1 Compare June 11, 2025 07:14
@PicchiSeba

Copy link
Copy Markdown
Contributor Author

I accidentally squashed the new test into the merge commit, it should be good now

@lmignon lmignon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the last changes. One little remaining comment.

Comment thread fastapi/tests/test_fastapi_multislash_demo.py Outdated
lmignon and others added 2 commits June 11, 2025 09:57
When we lookup the fastapi.endpoint to use to process a request at a given path, we must ensure that the matching mechanism on the path and the endpoint root_path is bases on the path parts. e.g. /a/b is not prefix of /a/bc.
In the same time improves robustness of the matching mechanism and avoid useless SQL queries
@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from e00d3d1 to 666a6b1 Compare June 11, 2025 07:58
@PicchiSeba

Copy link
Copy Markdown
Contributor Author

@lmignon I removed the useless test class

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@lmignon

lmignon commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

/ocabot merge minor

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-515-by-lmignon-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit d383536 into OCA:16.0 Jun 25, 2025
7 checks passed
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 6f480bf. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants