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

Preserve route_class when calling include_router #538

Merged
merged 2 commits into from
Oct 4, 2019

Conversation

dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Sep 17, 2019

Ensures that the route_class of the subrouter is preserved when including a subrouter in a parent router.


Right now, when including a router with a custom route_class, the route_class is not automatically preserved. This essentially prevents the use of custom route_class without special tricks. (For example, see #521, where overriding the route_class is used to enable msgpack parsing.)

This PR changes the behavior of include_router so that the route class is preserved during inclusion for any routes that are subclasses of APIRoute.


Closes #539

@codecov
Copy link

codecov bot commented Sep 17, 2019

Codecov Report

Merging #538 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #538   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         255    258    +3     
  Lines        6244   6307   +63     
=====================================
+ Hits         6244   6307   +63
Impacted Files Coverage Δ
fastapi/routing.py 100% <ø> (ø) ⬆️
tests/test_custom_route_class.py 100% <100%> (ø)
...ration_advanced_configurations/test_tutorial003.py 100% <0%> (ø)
...th_operation_advanced_configuration/tutorial003.py 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5ccb3c...b742bca. Read the comment docs.

@dmontagu
Copy link
Collaborator Author

@tiangolo I just re-ran the checks for this PR. Note #568 may serve as an alternative, but would require a little more effort. I'm not sure which way (if either) you'd be inclined to go.

@tiangolo tiangolo merged commit dd96351 into tiangolo:master Oct 4, 2019
@tiangolo
Copy link
Owner

tiangolo commented Oct 4, 2019

Thanks @dmontagu ! Great! 🚀 🎉

I just added a couple of tests to make sure we avoid possible future regressions with this. ✅

@dmontagu
Copy link
Collaborator Author

dmontagu commented Oct 4, 2019

Thanks, sorry the PR wasn't more complete!

@tiangolo
Copy link
Owner

tiangolo commented Oct 4, 2019

No worries at all, you're doing an awesome job helping with FastAPI here. With code and also helping others in issues and in Gitter.

You rock! 🎸 🚀 🎉 🙇‍♂️

@ifigueroap
Copy link

Hi, I'm wondering on whether it is possible to externally override the route_class of an included router. A use case would be to add a logging implementation (via subclassing of APIRoute) to existing routers. This is currently possible only if the source is available, and only by doing it before adding the routes, but it appears not to be possible for external modules. The route_class_override parameter is also only available at the actual call of add_api_route.

The crux of the issue, I think, is that routes are eagerly constructed with the route_class constructor in the add_api_route method, and then added to the self.routes attribute. Hence, overriding the route_class after the fact would perhaps require to reconstruct all the routes... I'm not sure that would be desireable nor possible...

Still, I think the use case for 'dynamic route_class overriding' is there...

Please let me know if you think this makes sense for the project, and in that case whether you would like me to submit a more refined issue.

Thank you for this great project!

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.

route_class not preserved when calling APIRouter.include_router
3 participants