Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 25, 2025
Merged

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

merged 4 commits into from
Jun 25, 2025

Conversation

PicchiSeba
Copy link
Contributor

@PicchiSeba PicchiSeba commented Apr 1, 2025 •
edited
Loading

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
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
This was referenced Apr 28, 2025
Open
Open
@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from f7dda08 to efc5e40 Compare April 30, 2025 10:31
@PicchiSeba
Copy link
Contributor Author

PicchiSeba commented Apr 30, 2025 •
edited
Loading

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
lmignon
lmignon reviewed May 14, 2025
View reviewed changes
lmignon
lmignon requested changes May 14, 2025
View reviewed changes
Copy link
Contributor

@lmignon lmignon left a comment

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
lmignon
lmignon reviewed May 14, 2025
View reviewed changes
lmignon
lmignon requested changes May 14, 2025
View reviewed changes
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

see PyTech-SRL#1

@lmignon
Copy link
Contributor

lmignon commented May 14, 2025

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 approved these changes May 15, 2025
View reviewed changes
Copy link
Contributor

@lmignon lmignon left a comment

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
Contributor Author

Thank you for this great improvement @PicchiSeba

LGTM (Code review, functionnal tests)

Thank you for your support and technical expertize!

@lmignon lmignon mentioned this pull request May 15, 2025
Closed
@sebastienbeau sebastienbeau added this to the 16.0 milestone Jun 3, 2025
@lmignon
Copy link
Contributor

lmignon commented Jun 4, 2025

ping @AnizR @sbidoul

AnizR
AnizR reviewed Jun 4, 2025
View reviewed changes
Copy link
Contributor

@AnizR AnizR left a comment

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!

@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
Contributor Author

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

lmignon
lmignon requested changes Jun 11, 2025
View reviewed changes
Copy link
Contributor

@lmignon lmignon left a comment

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.

lmignon and others added 2 commits June 11, 2025 09:57
@lmignon @PicchiSeba
[IMP] fastapi: Ensures endpoint matching is based on path parts
55adbd2 
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
[IMP]fastapi: add test for multi-slash scenario
666a6b1
@PicchiSeba PicchiSeba force-pushed the 16.0-fastapi-multislash-route branch from e00d3d1 to 666a6b1 Compare June 11, 2025 07:58
@PicchiSeba
Copy link
Contributor Author

@lmignon I removed the useless test class

@OCA-git-bot
Copy link
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
Copy link
Contributor

lmignon commented Jun 25, 2025

/ocabot merge minor

@OCA-git-bot
Copy link
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
Contributor

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

@PicchiSeba PicchiSeba deleted the 16.0-fastapi-multislash-route branch June 25, 2025 09:01
This was referenced Jun 28, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simahawk simahawk simahawk left review comments

@lmignon lmignon lmignon approved these changes

@AnizR AnizR AnizR approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
6 participants
@PicchiSeba @OCA-git-bot @lmignon @simahawk @AnizR @sebastienbeau