-
Notifications
You must be signed in to change notification settings - Fork 530
add cascade for dataverse delete with metrics #12031
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: develop
Are you sure you want to change the base?
add cascade for dataverse delete with metrics #12031
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE metric DROP CONSTRAINT fk_metric_dataverse_id; | |||
| ALTER TABLE metric ADD CONSTRAINT fk_metric_dataverse_id FOREIGN KEY (dataverse_id) REFERENCES dataverse(id) ON DELETE CASCADE; | |||
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.
I don't think this would usually be generated (with the ON DELETE CASCADE part) - the Cascade remove annotation works at the ORM layer. At least I haven't seen it before looking at the table descriptions for other places we have cascade remove annotations. I guess I'd suggest seeing if the remove works without changing the db constraint if you haven't already.
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.
I re-tested everything and the annotation on Metric didn't do anything. The only change that works is the constraint change in this sql
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.
I tested both with upgraded db/dataverse and new db
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.
Hmm - that seems like a problem - what annotations did you try? Orphan?
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.
Are we missing the equivalent of
dataverse/src/main/java/edu/harvard/iq/dataverse/Dataset.java
Lines 144 to 145 in 70b6e26
| @OneToMany(mappedBy = "dataset", orphanRemoval = true, cascade = {CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST}) | |
| private List<DatasetMetrics> datasetMetrics = new ArrayList<>(); |
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.
yes. Dataverse doesn't have that. There is no DataverseMetrics class
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.
added
@OneToMany(mappedBy = "dataverse", orphanRemoval = true, cascade = {CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST}) private List<Metric> dataverseMetrics = new ArrayList<>();
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
What this PR does / why we need it: dataverse delete fails if there are cached metrics due to a foreign key constraint
Which issue(s) this PR closes: #12030
Special notes for your reviewer:
Suggestions on how to test this: create/publish dataverse. request metrics to cause a cached row in metric table. Delete the dataverse
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: