[19.0][FIX] auditlog: Maximum recursion error when creating new accounts

[hi-oerp]

Module

Auditlog

Describe the bug

Maximum recursion error when creating new accounts

To Reproduce

Create a new rule for the account.account module
Check "Log Creates", "Log Writes" and "Full Log" in the new rule

Steps to reproduce the behavior:

Go to Accounting -> Configuration -> Chart of Account
Create a new account with code
Foreve loading and "RecursionError: maximum recursion depth exceeded"

Expected behavior
It should be able to save

Additional context
In Odoo, there are 2 methods in the AccountAccount class called _compute_code and _inverse_code.
Auditlog module keeps triggering these 2 methods and causing an infinite loop.

[hi-oerp]

Good morning.

@sebalix @StefanRijnhart @pedrobaeza @hbrunn Could you please help me validate this so we can proceed with the approval and merge?

Thanks!

[StefanRijnhart]

Thanks! Can you add a reproducing test to https://github.com/OCA/server-tools/tree/19.0/test_auditlog?

[StefanRijnhart]

If you check the failing tests, it looks like your changes prevent some other expected logs to be created, so it's not all well.

[hi-oerp]

Thanks! Can you add a reproducing test to https://github.com/OCA/server-tools/tree/19.0/test_auditlog?

Hello @StefanRijnhart I created the tests to verify the infinite recursion issue.

[hi-oerp]

If you check the failing tests, it looks like your changes prevent some other expected logs to be created, so it's not all well.

Hello @StefanRijnhart I have already verified the changes. Could you please regenerate the unit tests for the PR?

Thanks!

[StefanRijnhart]

Thanks! Nice work on building an actual recursion prevention mechanism. However, I don't think we need to resort to manipulating threads but instead add the auditlog_guard dictionary to the context. Because it is an editable, it can still be updated in nested calls. Would you mind refactoring it?

[hi-oerp]

Thanks! Nice work on building an actual recursion prevention mechanism. However, I don't think we need to resort to manipulating threads but instead add the auditlog_guard dictionary to the context. Because it is an editable, it can still be updated in nested calls. Would you mind refactoring it?

Thank you very much. @StefanRijnhart

As a first approach, I tried implementing this using the context, but in some cases the context is lost, which causes the recursion issue to reappear. I also attempted to pass the control flag through the transaction, but the recursion continue.

As a last resort, I had to implement the guard using a thread variable.

Note: If this solution is acceptable, I will also need to apply the same implementation to the fast log.

Thanks in advance!

[hi-oerp]

Hello @StefanRijnhart I have pushed the latest changes. I also added the context used to skip the read operation.

Please let me know if you have any comments or suggestions. Thank you very much for your feedback!

[StefanRijnhart]

Can you be more specific under which the context gets lost? I have a POC here locally with the guard in the context and the test still succeeds.

[hi-oerp]

Hello @StefanRijnhart

Thank you very much for the code review. I've addressed all the requested changes.

Could you please take another look? If everything looks good, I'd appreciate it if you could proceed with the integration. Once it's merged, I'll be able to close the remaining ticket I have for this module.

See you!

[hi-oerp]

Hello @StefanRijnhart , Could you please validate whether it is possible to merge this PR?

Thank you in advance!

[StefanRijnhart]

Thanks, looking good!

[pedrobaeza]

Can you remove [19.0] from the commit message?

[hi-oerp]

Hello @pedrobaeza , The recommendations were implemented!

Thank you!

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[StefanRijnhart]

Making major version bump to signal the modified context key name.

/ocabot merge major

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 19.0-ocabot-merge-pr-3646-by-StefanRijnhart-bump-major, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at ffc2d08. Thanks a lot for contributing to OCA. ❤️