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] util/records: fix delete_unused DOCSTRING #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pirols
Copy link
Contributor

@Pirols Pirols commented Jun 18, 2024

delete_unused, in spite of its name, deletes records even if they are used, provided they are not part of a foreign key with restrict ondelete clause.

@Pirols Pirols requested review from a team and jjmaksoud June 18, 2024 13:06
@robodoo
Copy link
Contributor

robodoo commented Jun 18, 2024

Pull request status dashboard

@Pirols
Copy link
Contributor Author

Pirols commented Jun 18, 2024

upgradeci skip

Copy link

@UemuS UemuS left a comment

Choose a reason for hiding this comment

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

We could also mention that it does not handle indirect references.
If table 1 is referenced with ON DELETE CASCADE by table 2 and table 2 is restricted by table 3, the upgrade would crash.
I'm trying to fix this in addition to some other cases, but it's a bit tricky

@diagnoza
Copy link
Contributor

diagnoza commented Jun 18, 2024

I thought that it's somewhat implied that if there's no direct link in the database, the record won't be removed :p The term "used" is used in the sense that there is a constraint linking one record to another.

Imho what would be more important to mention is what Yazid said.

@Pirols
Copy link
Contributor Author

Pirols commented Jun 18, 2024

We could also mention that it does not handle indirect references. If table 1 is referenced with ON DELETE CASCADE by table 2 and table 2 is restricted by table 3, the upgrade would crash.

That's a good idea, I will add a warning.

So the set null is the same as restrict, only cascade is ignored when checking fk on other tables.

This is right and I will clarify it in the DOCSTRING.

However, I think we should consider addressing this: the behavior sounds to be inconsistent when we have a set clause vs a restrict one on a direct/indirect reference. See the possibilities:

  • Direct restrict reference -> no removal ✅
  • Direct set reference -> no removal ✅
  • Indirect restrict reference -> failure. Not ideal but at least it raises a concern ⚠️
  • Indirect set reference -> (my untested guess) removes the records (and triggers the indirect ondelete clause). Inconsistent case ❌

But well if @UemuS manages to solve the indirect issue, this inconsistency will also be gone, just wanted to acknowledge it.

I thought that it's somewhat implied that if there's no direct link in the database, the record won't be removed :p

Exactly! And that's the problem with the name/DOCSTRING: they don't explain that records will still be removed if their references can cascade.

@Pirols Pirols force-pushed the master-fix_delete_unused_docline-pied branch from 5c481a4 to 00b29ae Compare June 18, 2024 15:42
@Pirols
Copy link
Contributor Author

Pirols commented Jun 18, 2024

upgradeci skip

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
`delete_unused`, in spite of its name, deletes records even if they are used,
provided they are not part of a foreign key with `restrict` or `set` ondelete
clause (iow: allowing for cascading foreign keys).
@Pirols Pirols force-pushed the master-fix_delete_unused_docline-pied branch from 00b29ae to 0675a6a Compare June 19, 2024 08:05
@Pirols Pirols requested review from a team and asno-odoo June 19, 2024 08:05
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.

None yet

5 participants