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: restore has_one with scope #551

Merged
merged 5 commits into from
Mar 12, 2024
Merged

Conversation

zygzagZ
Copy link
Contributor

@zygzagZ zygzagZ commented Jan 29, 2024

When recursively restoring a has_one relation, Paranoia tries to restore any matching deleted record. It ignores the association scope. This can cause both false positives and false negatives when restoring.

This PR imitates this commit from acts_as_paranoid. I am not sure if intentionally, but that commit introduced scope merging for has_one relations there.

when looking for parent.alpha (or beta) to restore, loading association fails, because there are no undeleted records
lib/paranoia.rb:204 association_data = send(association.name) = nil
the procedure at lib/paranoia.rb:224 ignores association scope and restores the lowest-id record, :gamma
effect: beta was never restored and is nil, gamma was restored incorrectly

I am not sure if reflection.scope is the correct way to recover association's scope, acts_as_paranoid does this differently. It works in AR 5.1-7.1 though.
I have tested this PR in ruby 3.2.2 in rails 7.1.3 and 6.1.7.6. It also does work in 5.1.7 application, but I failed to resolve gems in ruby 2.5.8/2.6.9 (and run the suite)

@zygzagZ
Copy link
Contributor Author

zygzagZ commented Jan 29, 2024

I am not sure what happens with has_many relations, they might need patching too.

@mathieujobin mathieujobin merged commit f68cb23 into rubysherpas:core Mar 12, 2024
25 checks passed
@mathieujobin
Copy link
Collaborator

Thank you very much for this

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.

2 participants