Adding support for documenting OpenAPI callbacks through APIRoutes #722
Conversation
Codecov Report
@@ Coverage Diff @@
## master #722 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 277 279 +2
Lines 7210 7256 +46
=====================================
+ Hits 7210 7256 +46
Continue to review full report at Codecov.
|
|
Fixes issue #673 |
|
Thanks! I'll review it soon. |
|
@tiangolo - any update on this? Anything you'd like to discuss? |
There was a problem hiding this comment.
We also keep 100% test coverage of the docs .py files, which it looks like you haven’t added. I would recommend importing the app declares in the docs file inside your test file, rather than duplicating the code. You should be able to find examples along these lines in some of the other test files.
|
@dmontagu - thanks for taking the time to review the changes! I'll incorporate the changes you requested, and rebase the code against the latest release. Before moving ahead though, I just want to confirm that you are OK with the approach of defining the callbacks in this manner, and that I'm not abusing any of the existing constructs 😉 |
|
It looks like you are properly following all conventions, as far as I can tell. 👍 I haven't closely read the openapi spec for callbacks yet, and probably would want to before merging this, but as long as the schema you are outputting matches the OpenAPI spec I would be happy to support this feature, and I think the implementation here is good. At some point I would like to refactor the endpoint creation process so that we don't have to copy the long list of openapi keyword arguments into each of the different route decorator methods; it makes adding features like this somewhat daunting / painful. But approaches to that have been brought up in other issues, and for now I am happy to continue following that convention. |
|
Yep, this looks good, thanks for making this! 🚀 🍰 Maybe it feels weird to have a full router to declare endpoints that will never be used, but most of the logic was already there, and it will be the most familiar thing to use for FastAPI users, so I think it's fine, it's the most sensible approach. For the rest, agreed with @dmontagu (thanks for the review!). I also want to check some things that could be updated later, but some I don't know if they would work or not, I'll do it afterwards on top of your changes. |
|
I'm currently working on this, I'll send my updates on top of your branch once they are done. So you can just wait a bit. |
Co-Authored-By: dmontagu <35119617+dmontagu@users.noreply.github.com>
|
Thanks! 🎉 🚀 I updated it a bit. I also added some extras that improve how this feature works. Some of the updates:
|
Added support for automatic generation and validation (via pydantic) of OpenAPI callback objects.
This is currently done by providing a list of
APIRouteobjects to a new keyword argument in the path decorators (post, get, put, etc.).This is convenient, as that list can easily be compiled by creating an
APIRouterobject, and defining the expected (callback) routes. This has the added benefit that all Request and Response Schemas are added to the documentation, and the router used for defining the callbacks makes a solid base for mocking/testing the callback endpoints that a client would implement.Please let me know if you have any comments/concerns.
Right now, the tests are failing because the coverage decreased a little. I'll try to see if this can be fixed. I can also squash and rebase my changes if needed.