[17.0][FIX] fastapi: Flush before exiting Fastapi environment#538
Conversation
|
Hi @lmignon, |
|
ping @simahawk |
lmignon
left a comment
lmignon
left a comment
There was a problem hiding this comment.
Thank you for the fix @grindtildeath. I'm surprised this problem hasn't been discovered before, as this module is used in so many projects. It would be nice to be able to reproduce it in an HttpCase to make sure there's no possible regression.
Because any sane stored computed field should be compute_sudo? |
|
@lmignon I tried to write a test in HttpCase but I'm not sure how we could (or if it is even possible to) test this part. @sbidoul That is actually already the default for stored computed fields: https://github.com/odoo/odoo/blob/17.0/odoo/fields.py#L442 But what it's doing is setting Writing this, I ought to precise we have this issue because we have a stored computed field whose value has to be exported through a connector when changed. For this we rely on an override of We could force the queue job to be created with the super user, or even call the flush in the fastapi endpoint where this field gets recomputed, but IMO it makes more sense to have all pending computations processed through the user triggering these, as having the public user there doesn't seem correct from my point of view. Now, another option could be to instantiate a dedicated cursor to execute the fastapi endpoint, so that the cursor is commited before we exit the context manager altering the environment, but I thought triggering a flush there was less intrusive (although I guess it will not prevent other issues if we have postcommit hooks being run with the public user). |
|
@grindtildeath thank for the additional info. To be clear I was not implying we should not implement this PR, just suggesting a reason as to why it was not discovered before. About this PR, could it influence the transaction retry mechanism? I kind of remember we worked a lot on this part last year. |
Indeed ...
Thank you for the explanation. I understand the problem better now.
+1
I would avoid this option. One flush before context exit seems the right approach and the less intrusive / complex one. |
| token = odoo_env_ctx.set(env) | ||
| try: | ||
| yield | ||
| env.flush_all() |
There was a problem hiding this comment.
@grindtildeath Can you add a comment above the new line with the motivation to do it. The aim is to quickly inform people reading the code why the call to flush_all is being made, and to prevent this line from being inadvertently removed during a migration.
There was a problem hiding this comment.
@lmignon I amended the commit with the code comment to recap why it's done.
Feel free to suggest a rephrase if it doesn't seem clear enough, and thank you for being so reactive 🙂
IMO This should not affect the retry mechanism. In the event of an exception, the error will bubble up to the retry method, as is already the case today when the call to |
As the fastapi dispatcher is altering the public request's environment before calling the endpoint, it will reuse the same DB cursor from this environment. Therefore, if fields are being marked to be recomputed during the execution of the fastapi endpoint, these will not be recomputed until the cursor is being commited, what happens when we are not anymore authenticated through as fastapi user. This means the recomputation of computed fields will be executed with the public user, what could potentially lead to inconsistencies or result in an AccessError. By calling flush before exiting the contextmanager yielding the altered Odoo environment, we ensure all the pending computations are executed while being authenticated with the fastapi user which must have more access rights than the public user.
grindtildeath
force-pushed
the
17.0-fix-fastapi-flush_computations
branch
from
b1c0e9a to
96f39ac
Compare
simahawk
left a comment
simahawk
left a comment
There was a problem hiding this comment.
makes sense and looks good
|
/ocabot merge patch |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at d8fba94. Thanks a lot for contributing to OCA. ❤️ |
5 participants
As the fastapi dispatcher is altering the public request's environment before calling the endpoint, it will reuse the same DB cursor from this environment.
Therefore, if fields are being marked to be recomputed during the execution of the fastapi endpoint, these will not be recomputed until the cursor is being commited, what happens when we are not anymore authenticated through as fastapi user. This means the recomputation of computed fields will be executed with the public user, what has a very high probability of resulting in an AccessError.
By calling flush before exiting the contextmanager yielding the altered Odoo environment, we ensure all the pending computations are executed while being authenticated with the fastapi user which must have more access rights than the public user.