Skip to content
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

[FIX] records: use temp table in replacing refs #224

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jjmaksoud
Copy link
Contributor

@jjmaksoud jjmaksoud commented Mar 3, 2025

For the jsonb company dependent indirect references when replacing record refs, building the query using the original id mapping input can generate a large query based on the number of references being updated, leading to a stack depth limit exceeded error.

We can use the mapping saved in the temp table _upgrade_rrr instead.

TBG-1752

@robodoo
Copy link
Contributor

robodoo commented Mar 3, 2025

Pull request status dashboard

@jjmaksoud
Copy link
Contributor Author

upgradeci retry with always only documents_project_sale in version 18.0

@jjmaksoud jjmaksoud requested review from a team and asno-odoo March 4, 2025 08:45
@aj-fuentes
Copy link
Contributor

What examples do we have of such issue? Isn't this a problem of the caller to replace_records_reference_batch? Or that we should implement something perhaps splitting the id mapping?

@jjmaksoud
Copy link
Contributor Author

i saw this so far only in documents upgrade when replacing folders with documents here
example request 2592340: has over 360,000 folder mappings and the new query was fast ( <2s on the test server)
I think the subquery should be okay because the number of companies is expected to be low, the explode bucket size is manageable, and the mapping table has an index

@jjmaksoud jjmaksoud force-pushed the master-replace-cmp-dep-maji branch from cd641d9 to de159d9 Compare March 4, 2025 10:56
@KangOl
Copy link
Contributor

KangOl commented Mar 4, 2025

Can you add a test?

@jjmaksoud jjmaksoud force-pushed the master-replace-cmp-dep-maji branch from de159d9 to 7bdd490 Compare March 6, 2025 09:14
@jjmaksoud
Copy link
Contributor Author

The test depends on odoo/odoo#200509

@jjmaksoud jjmaksoud force-pushed the master-replace-cmp-dep-maji branch from 7bdd490 to 324ea17 Compare March 6, 2025 10:28
@jjmaksoud jjmaksoud force-pushed the master-replace-cmp-dep-maji branch from 324ea17 to 65d35c3 Compare March 19, 2025 09:42
@jjmaksoud
Copy link
Contributor Author

the community PR is merged and the test is now green

@jjmaksoud jjmaksoud force-pushed the master-replace-cmp-dep-maji branch from 65d35c3 to 44c3584 Compare March 19, 2025 15:07
Comment on lines 1412 to 1413
add_test_trigger(self.env.cr, "replace_record_references", "res_partner", "UPDATE", f"new.id = {p3.id}")
util.replace_record_references_batch(self.env.cr, mapping, "res.currency")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be context manager

Suggested change
add_test_trigger(self.env.cr, "replace_record_references", "res_partner", "UPDATE", f"new.id = {p3.id}")
util.replace_record_references_batch(self.env.cr, mapping, "res.currency")
with self.assertNotUpdated("res_partner", [p3.id]):
util.replace_record_references_batch(self.env.cr, mapping, "res.currency")

Signature: def assertNotUpdated(self, table, ids=None):.
When ids is None, the whole should be handled, which mean no INSERT.

We can also add the positive version def assertUpdated(self, table, ids=None)

These functions need to be added in a dedicated commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with the other assertX functions, the signature needs to take a msg argument.

assertNotUpdated(self, table, ids=None, msg=None)

Two contextmanager methods added to UnitTestCase to assert
wether records in the database are updated or not.
The assert requires a table name and an optional list of ids.
If ids are provided, those specific records will be checked
if updated or not. If no ids are provided then the validation
will be based on the insert or update of any record of that
relation.
For the jsonb company dependent indirect references when
replacing record refs, building the query using the
original id mapping input can generate a large query
based on the number of references being updated, leading
to a 'stack depth limit exceeded' error.

We can use the mapping saved in the temp table
_upgrade_rrr instead.
@jjmaksoud jjmaksoud force-pushed the master-replace-cmp-dep-maji branch from 44c3584 to 8ab406a Compare March 20, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants