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
[16.0] FastAPI Integration #291
Conversation
lmignon
commented
Sep 28, 2022
•
edited
edited
- manage exceptions and transaction rollback (through the use of a dedicated starlette middleware???)
- manage languages
- works with extendable-pydantic
- depends on [16.0] endpoint_route_handler: migration to v16 web-api#17
- improves UI to alert the user that the routing map is out of sync and must be synchronized when one element involved into the routing definition is updated or a new app is created.
- rework the documentation to address the security aspect from the start and to evolve its implementation in the examples given in the following topics.
23aabb2
to
e54b56a
Compare
@Julien00859 Here it's a usage of the |
6503eb6
to
bb4c85d
Compare
@sebastienbeau @sbidoul @simahawk @StefanRijnhart I took some time to write some documentation explaining what writing a REST api is and how to do it with this new addon. I still have to understand why the tests are failing in our CI but not on my laptop. Your comments and suggestions are welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is seriously great !
fastapi/depends.py
Outdated
def authenticated_partner_from_basic_auth_user( | ||
user: Users = Depends(basic_auth_user), # noqa: B008 | ||
) -> Partner: | ||
return user.partner_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use env["res.partner"].browse(user.partner_id)
, so the partner is not sudo?
fastapi/readme/ROADMAP.rst
Outdated
because the integration of the fastapi is based on the use of a specific middleware | ||
that convert the WSGI request consumed by odoo to a ASGI request. The question | ||
is to know if it is also possible to develop the same kind of bridge for the | ||
WebSockets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the same challenge applies to streaming large responses in the asgi-wsgi bridge?
Before you start, we must define some terms: | ||
|
||
* **App**: A FastAPI app is a collection of routes, dependencies, and other | ||
components that can be used to build a web application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also an ASGI app?
|
||
* **'odoo_env'**: Returns the current odoo environment. | ||
* **'fastapi_endpoint'**: Returns the current fastapi endpoint model instance. | ||
* **'authenticated_partner'**: Returns the authenticated partner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention authenticated_partner_env and when to use it vs odoo_env?
* You can change the implementation of the route handler. | ||
* You can override the dependencies of the route handler. | ||
* You can add a new route handler. | ||
* You can extend the model used as parameter or as response of the route handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elaborate this? One can add optional fields (or required fields with default values) to the request.
In the response, one can add fields, or make optional fields required, but not make required fields optional.
One can remove optional fields from a response (depending on what optional means exactly - null vs absent).
_name = "demo.fastapi.endpoint" | ||
_description = "Demo Endpoint" | ||
|
||
def echo(self, message: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@api.model
?
|
||
The liskov substitution principle has also to be respected. That means that | ||
if you extend a model, you must add new required fields or you must provide | ||
default values for the new optional fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elaborate?
|
||
* Use plural for the name of a service. For example, if you provide a service | ||
that allows you to manage the sale orders, you must use the name 'sale_orders' | ||
and not 'sale_order'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention the concept of collection of resources which must be named with a plural (/sale_orders/{id}
?
|
||
|
||
|
||
* ... and many more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Say a word about input validation. Be paranoid about what clients send, assume they will send invalid id's, or attempt to access forbidden data.
app.dependency_overrides[ | ||
authenticated_partner_impl | ||
] = authenticated_partner_impl_override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.dependency_overrides[ | |
authenticated_partner_impl | |
] = authenticated_partner_impl_override | |
app.dependency_overrides[ | |
authenticated_partner_impl | |
] = authenticated_partner_impl_override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is wrong, so when self.app is different then "demo" it raise an error as authenticated_partner_impl_override is not defined
cac1747
to
df2f6ed
Compare
…pi service excution
* add default empty method to use as dependency to get the authenticated partner * improves the demo app to illustrate the way the dependency overrides mechanism can be used to provide the right implementation to use to retrieve the authenticated partner according to the security method configured on the app * add tests for the demo app to show how the TestClient class and the dependey overrides functianality should be used to easily write tests
This method can be used to get access to the fastapi.endpoint record into your router's methods
ensure transation is rolled back in case of error and allows override / extension of the error handling globally or by app
df2f6ed
to
dca1b58
Compare
fastapi/models/fastapi_endpoint.py
Outdated
def _get_app_exception_handlers( | ||
self, | ||
) -> Dict[ | ||
int | Type[Exception], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int | Type[Exception], | |
Union[int, Type[Exception]], |
With this notation fastapi support python 3.7+ as you can see documentation section union. Otherwise we would be locked to python 3.10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @flachica
Happy to see someone making a try with this draft PR. I plan to work on it in the coming days to publish a first release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thoroughly studying this module and related technology. I intend to use it extensively in the very short term. I understand that it is in an initial state and I will do my best to contribute according to my possibilities and assuming that changes will come and I will have to adapt. Thank you very much for all the effort, especially in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've two issues to solve to be complete.
The management of the languages to return the translated info into the requested language if available. This will be based on the accept-language http headerdone in 1a750d4The use of extendable-pydantic is not yet completely functional. For some processes, the fastapi lib use internal methods from the pydantic lib without going through the model class directly. This means that the extension mechanisms implemented by the extendable-fastapi library at the class definition level are bypassed in these cases. I have identified at this stage 2 places that need to be adapted so that the schema for an extended model is correctly generated in the documentation and the serialization is based on the extended class definition and not the base class.Fixed into the last release of extenable-pydantic (0.0.2)
We'll probably find others issues but I'm confident that nothing should call into question the approach, which seems to me to be validated from a technical point of view. I'll start to use it for my own projects in the coming weeks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flachica The integration of extendable-pydantic and fastapi is now fixed into the last version of extendable-pydantic (>=0.0.2) I've to adapt the documentation to specify that an extra odoo addon is required to 'extendable_fastapi'. I'll add the management of the languages tomorrow.
fastapi/readme/USAGE.rst
Outdated
""" | ||
# TODO we should declare a technical user with read access only on the | ||
# fastapi.endpoint model | ||
return env["fastapi.endpoint"].sudo().browse(_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return env["fastapi.endpoint"].sudo().browse(_id) | |
return env["fastapi.endpoint"].browse(_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In endpoint sudo not must be used
I have built my enpoint and it works perfectly. Now it turns out that I've done the tests for that endpoint and it doesn't work. If the enpoint only returns a message it works fine, but if it has to access the database it doesn't throw the exception, nor does it exit self.env[model].search(). I don't know what I'm doing wrong. I'm waiting for the writing of that part of documentation of security that you mention that is pending. |
@flachica About security you've 2 point to take into account.
The principles are explained here https://github.com/OCA/rest-framework/pull/291/files#diff-d864270e7f736cc705f667b331dd7242c7841de1783c06d10550a27ede6d6ed3R803 |
In the same time, applies the security guidelines for the demo app
@flachica In my last commit I improved the documentation about security aspects and provides now a basic group to use when creating your own security group for your app. |
I have read it. It's great |
selection_add=[("demo", "Demo Endpoint")], ondelete={"demo": "cascade"} | ||
) | ||
demo_auth_method = fields.Selection( | ||
selection=[("api_key", "Api Key"), ("http_basic", "HTTP Bacic")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selection=[("api_key", "Api Key"), ("http_basic", "HTTP Bacic")], | |
selection=[("api_key", "Api Key"), ("http_basic", "HTTP Basic")], |
fastapi/readme/USAGE.rst
Outdated
<record id="my_demo_app_group" model="res.groups"> | ||
<field name="name">My Demo Endpoint Group</field> | ||
<field name="users" eval="[(4, ref('my_demo_app_user'))]" /> | ||
<field name="implied_ids" eval="[(4, ref('fast_api.group_fastapi_endpoint_runner'))]" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast_api -> fastapi
fastapi/readme/USAGE.rst
Outdated
|
||
.. code-block:: python | ||
|
||
from ..depends import odoo_env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from ..depends import odoo_env | |
from odoo.addons.fastapi.depends import odoo_env |
fastapi/readme/USAGE.rst
Outdated
|
||
.. code-block:: python | ||
|
||
from ..depends import odoo_env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from ..depends import odoo_env | |
from odoo.addons.fastapi.depends import odoo_env |
May I know the fastapi version you used? I tried to run these code recently but got error "cannot import name 'APIRouter' from 'fastapi'" |
AFAIK Tha APIRouter is in all versions of fastapi https://github.com/tiangolo/fastapi/blob/c81e136d75f5ac4252df740b35551cf2afb4c7f1/fastapi/routing.py#L478 |
Dear @lmignon, while reviewing the security issue of the The tests I have done have been from The question is this: Is there any plan to move the base_rest dependent modules that are already in 16.0 to fastapi? Is there any progress in this regard? It is not to duplicate work. I would like to contribute and I think that in order to maximize the value it is good to reach an agreement. |
When a field is of an extended type, the assemebled class is not part of the know classes computed by fastapi from the fastapi endepoint definition. Is such a case, we must add the extented type to the list of knwon classes
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at db3e878. Thanks a lot for contributing to OCA. ❤️ |
to declare the way your partner will be provided. In some case, this | ||
partner will come from the authentication mechanism (ex jwt token) in other cases | ||
it could comme from a lookup on an email received into an HTTP header ... | ||
See the fastapi_endpoint_demo for an example""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon should this not raise NotImplementedError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll do that in the fastapi_auth_jwt
PR.
Yes, it should be.
…On Thu, Jun 8, 2023 at 12:30 PM Stéphane Bidoul ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In fastapi/depends.py
<#291 (comment)>:
> +from .schemas import Paging
+
+if TYPE_CHECKING:
+ from .models.fastapi_endpoint import FastapiEndpoint
+
+
+def odoo_env() -> Environment:
+ yield odoo_env_ctx.get()
+
+
+def authenticated_partner_impl() -> Partner:
+ """This method has to be overriden when you create your fastapi app
+ to declare the way your partner will be provided. In some case, this
+ partner will come from the authentication mechanism (ex jwt token) in other cases
+ it could comme from a lookup on an email received into an HTTP header ...
+ See the fastapi_endpoint_demo for an example"""
@lmignon <https://github.com/lmignon> should this not raise
NotImplementedError ?
—
Reply to this email directly, view it on GitHub
<#291 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEE2WXPP2XWDBZ4TQ2YECTXKGSSVANCNFSM6AAAAAAQXOT7K4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi, I am using in the manifest: ["base", "base_rest_auth_api_key", "base_rest_datamodel",] Do I need to use fastapi? If so, how should it be implemented? |
@Josesosa07 So if you are starting a new project, it is better to start with the |