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.

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

Validate Path duplication on added routes #5595

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

Validate Path duplication on added routes #5595

wants to merge 38 commits into from

Conversation

iudeen
Copy link
Contributor

@iudeen iudeen commented Nov 7, 2022 •
edited

Fixes: #5543

  • Add basic tests to the PR.
  • Add and verify websocket routes
  • Add prefix conditions to tests
  • Improve coverage to 100%

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

📝 Docs preview for commit e0cddac at: https://63695d24e20d1f00a259254c--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

📝 Docs preview for commit 3cb2eab at: https://63695e96b5d16d007eb5b0a6--fastapi.netlify.app

@iudeen iudeen marked this pull request as draft November 7, 2022 19:48
@iudeen iudeen marked this pull request as ready for review November 7, 2022 20:27
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

📝 Docs preview for commit 20661e1 at: https://63696a8fb5d16d0c7db5ae65--fastapi.netlify.app

@iudeen
Copy link
Contributor Author

iudeen commented Nov 7, 2022

I am not sure how to cover the dummy routes :/

@iudeen
Copy link
Contributor Author

iudeen commented Nov 8, 2022

I'm also in a dilemma if this would be ideal if it gets handled in Starlette. Any suggestions? 🤔

@github-actions
Copy link
Contributor

📝 Docs preview for commit 75de88e at: https://636ea4e9772b9030adb3335e--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit a096af7 at: https://63714cf0e98b380f2847fe84--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 8e7dc52 at: https://63762fc1c196ad7d4d9dff19--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 7921614 at: https://639cd9077626051bad0704db--fastapi.netlify.app

@iudeen
Merge remote-tracking branch 'upstream/master' into feat/exception-on…
52acf4d 
…-duplicate-path

# Conflicts:
#	fastapi/routing.py
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

📝 Docs preview for commit 52acf4d at: https://645a061083ff353b014582a5--fastapi.netlify.app

@iudeen
Merge remote-tracking branch 'upstream/master' into feat/exception-on…
a3357a7 
…-duplicate-path

# Conflicts:
#	fastapi/routing.py
@tiangolo
Copy link
Owner

📝 Docs preview for commit a3357a7 at: https://6494522b1e30803212194d96--fastapi.netlify.app

@iudeen
Copy link
Contributor Author

iudeen commented Jul 18, 2023

Is this a good approach? 🤔

@iudeen
Copy link
Contributor Author

iudeen commented Sep 2, 2023

@tiangolo, can you let me know if this approach is right? If not I can take suggestions :)

@tiangolo tiangolo added the feature New feature or request label Oct 2, 2023
andreygaag
andreygaag suggested changes Dec 14, 2023
View reviewed changes
fastapi/routing.py Show resolved Hide resolved
@alejsdev alejsdev added p3 and removed investigate labels Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreygaag andreygaag andreygaag requested changes

Assignees
No one assigned
Labels
feature New feature or request p3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Two functions with one path produce inconsistent results
4 participants
@iudeen @tiangolo @andreygaag @alejsdev