-
Notifications
You must be signed in to change notification settings - Fork 488
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
IQSS/8889 - second try to limit file pids management #9721
IQSS/8889 - second try to limit file pids management #9721
Conversation
This reverts commit 36c9be5. Revert "no need to delete since we aren't using the null/unset val now" This reverts commit a0ac9e3. Revert "add second setting" This reverts commit 6944823. Revert "check collection-level setting" This reverts commit 4d6ea3e. Revert "flip to default being false" This reverts commit 14e59ca. Revert "check PID setting in create dataverse command" This reverts commit 33f994d. Revert "also only sleep when reigstering in this call" This reverts commit d9f8e8a. Revert "update docs" This reverts commit dc5d000. Revert "require superuser and for :FilePIDsEnabled to be set to allow changes" This reverts commit 3e9262f. Revert "don't mint file PIDs in collections where they are not allowed" This reverts commit aef24bb.
This reverts commit c6d4269.
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 think I spotted something to change in the docs.
@@ -2775,14 +2775,35 @@ timestamps. | |||
:FilePIDsEnabled | |||
++++++++++++++++ | |||
|
|||
Toggles publishing of file-level PIDs for the entire installation. By default this setting is absent and Dataverse Software assumes it to be true. If enabled, the registration will be performed asynchronously (in the background) during publishing of a dataset. | |||
Toggles publishing of file-level PIDs for the entire installation. By default this setting is absent and Dataverse Software assumes it to be false. If enabled, the registration will be performed asynchronously (in the background) during publishing of a dataset. |
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.
By default this setting is no longer absent, right?
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.
No - a new installation will not have it set, which we now interpret as false. Existing installs which had it not set will now have it set true.
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 can't deploy. Please see the Flyway error below.
INSERT INTO setting(content, lang, name) VALUES | ||
('true', '', ':FilePIDsEnabled') | ||
ON CONFLICT DO NOTHING; |
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'm getting this error (I'm on 687c872):
dev_dataverse> Caused by: org.flywaydb.core.internal.sqlscript.FlywaySqlScriptException: Migration V5.13.0.3__8889-change-filepids-default.sql failed
dev_dataverse> ------------------------------------------------------------
dev_dataverse> SQL State : 23514
dev_dataverse> Error Code : 0
dev_dataverse> Message : ERROR: new row for relation "setting" violates check constraint "non_empty_lang"
dev_dataverse> Detail: Failing row contains (1, true, , :FilePIDsEnabled).
dev_dataverse> Location : db/migration/V5.13.0.3__8889-change-filepids-default.sql (/opt/payara/deployments/dataverse/WEB-INF/classes/db/migration/V5.13.0.3__8889-change-filepids-default.sql)
dev_dataverse> Line : 1
dev_dataverse> Statement : INSERT INTO setting(content, lang, name) VALUES
dev_dataverse> ('true', '', ':FilePIDsEnabled')
dev_dataverse> ON CONFLICT DO NOTHING
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.
This error goes away if you change empty string to NULL: https://github.com/IQSS/dataverse/pull/9721/files#r1270667037
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.
Here's a suggestion for the sql script.
@@ -0,0 +1,3 @@ | |||
INSERT INTO setting(content, lang, name) VALUES | |||
('true', '', ':FilePIDsEnabled') |
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.
('true', '', ':FilePIDsEnabled') | |
('true', NULL, ':FilePIDsEnabled') |
Is the above what we want instead? NULL instead of empty string? With empty string I'm getting the error below.
Issues found:
curl "http://localhost:8080/api/admin/333906/registerDataFile"
Final log entries provide several streams of info: Also happens when AllowEnablingFilePIDsPerCollection is set to false and enablefilepid isn't set
|
|
Run2 with file pids enabled: {"status":"OK","data":{"message":"Datafile registration complete.0 of 1008 unregistered, published files registered successfully."}}
|
re: 2. It looks like the check to get the authenticated user was inside the loop and it's error message was getting caught/logged and not causing the call to fail with a 403. I've changed that now. (My guess is that you had no/a bad api key and the method just ran without succeeding, looping through all the files, rather than failing up front.) |
@qqmyers re: 2, registerDataFileAll, that is an admin endpoint so it shouldn't need an api key, right? |
There are many admin calls that require an authenticated user, and a few that require superuser. A lot of them, but not all, are ones I created so I may have not gone with the convention. FWIW - I think requiring a key does mean that the action log records who ran the commands in the api call. |
What this PR does / why we need it: This PR replaces #9716 which was accidentally closed via a bad push to develop (see dv-tech slack channel). This PR recreates the prior one and adds additional changes to use a second :AllowEnablingFilePIDsPerCollection setting that enables/disables the API to change whether FIle PIDs are allowed per collection. (The :FilePIDsEnabled setting is now just a way to set the global default.)
Which issue(s) this PR closes:
Closes #8889
Special notes for your reviewer: I hopefully have cleaned up the develop branch and gotten everything in here correctly. I'll check again tomorrow morning and will scan the docs/code once more to see if I missed anything we talked about in slack and the old PR. (the c6d4269 reverts the squash of all the commits that were reverted in develop).
Suggestions on how to test this: Setting :AllowEnablingFilePIDsPerCollection=true should enable the API call for superusers to set FilePIDs on/off per collection. Setting the global default via :FilePIDsEnabled should allow you to compare a collection using that global default and one where you've used the API to flip from true to false or vice versa. There is an automated test that verifies publishing a dataset after a collection has had its setting flipped does indeed have it's file PIDs handled correctly.
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?: modified from the ones for the original PR - can be combined with those (I think it should be cut/paste to replace them).
Additional documentation: