-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[18.0][FIX] auditlog: Enabling read logging raises ReadOnlySqlTransaction error. #3393
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
base: 18.0
Are you sure you want to change the base?
Conversation
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.
Thanks for the reproducing test! Will you work on a fix? In that case, please make the PR draft.
Or you can use assertRaises with an explanatory comment plus an entry in the readme roadmap/known issues to get this merged.
I could, but I don't have a design in mind. |
|
Would it work to check the readonly attribute of the cursor and then only log a warning? https://github.com/odoo/odoo/blob/18.0/odoo/sql_db.py#L510-L512 |
I don't believe so. I think it will be necessary to grab a writeable cursor just for audit logging and to reuse that cursor as there could be a large number of audit log writes. |
d88dbf1 to
a53485c
Compare
|
That sounds risky. Would it work to reset the |
|
It seems that any design is going to contradict the guidance from the commit:
I don't think that helps because the cursor is created according to the if self.env.cr.readonly:
with self.env.registry.cursor(readonly=False) as cr:
env = api.Environment(cr, SUPERUSER_ID, {})
return self.with_env(env).create_logs(...) |
|
Another source of inspiration could be how Odoo Footnotes |
The first link in my previous comment shows that what the decorator does is set the Our patch would be the first method encountered in the controller, so if we set _readonly to False, it wins. This change satisfies the test: |
a53485c to
ff9efc5
Compare
ff9efc5 to
d4447c2
Compare
Terrific! That covers the most direct case. I've added a second test to see if read logging is encountered for field computation, but it doesn't seem to be. Is that correct? None of my customers use "read" logging, so I don't have any expectation as to what the expected behavior is. |
|
@amh-mw I think that's alright. 'read' logs are not very practical in general I think, so if you are not invested in whatever logging is caused by Odoo internals maybe just leave out the second test. Seems to interfere with the first one as well. |
d4447c2 to
8c91d8e
Compare
Odoo 18 introduced read-only database cursors 1. Enabling
auditlogread logging causes each read-only request to raise a ReadOnlyTransaction error, which is handled internally by Odoo by retrying the request with a writable cursor. This seems guaranteed to cause slowness or repeated side effects.Footnotes
https://github.com/odoo/odoo/commit/584a172274caefb23e5d91b609fc893160535416 ↩