-
Notifications
You must be signed in to change notification settings - Fork 486
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/9387 - Support protected system metadata #9388
IQSS/9387 - Support protected system metadata #9388
Conversation
This reverts commit 4cccef3.
This reverts commit bd49930.
these methods were always using the update command (because they checked for isDraft after always creating a draft version). However, since the Create command assumes a new dsv not associated with the dataset yet, they also shouldn't use it. This commit reorders the isDraft check where needed and removes the logic to decide whether to use the Update or Create commands (since Update should always be used). This should be a big no-op since the old code always used Update 'accidentally').
needed for system Metadata check in CreateDatasetVersionCommand.
About setting up the testing, activating the feature, for example set '1234' on the citation metadata block.
The second and preferred one is via the
Removing is done by editing the properties file or with |
Hey @PaulBoon sorry to say this, but it looks like you're doing this in a much more complicated way than necessary.
HTH 😄 |
|
||
# METADATA SETTINGS | ||
#dataverse.metadata.block-system-metadata-keys |
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.
Not sure why you add this here, IMHO we should only add things that we have a default for. (Yes I am aware that there is a similar thing for OAI above - yet it looks like people start to mistake this file as editable for configuration, which is not how it's meant to be used)
String smdbString = JvmSettings.METADATA_BLOCK_SYSTEM_METADATA_KEYS.lookupOptional().orElse(null); | ||
if (smdbString != null) { | ||
JsonObject systemMetadataBlocks = JsonUtil.getJsonObject(smdbString); |
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.
How about adding a converter instead of fiddling with the String itself?
It's not very hard to do and makes code here much nicer to read and write. 😉 https://download.eclipse.org/microprofile/microprofile-config-3.0/microprofile-config-spec-3.0.html#_adding_custom_converters
JsonObject systemMetadataBlocks = JsonUtil.getJsonObject(smdbString); | ||
HttpServletRequest httpServletRequest = getRequest().getHttpServletRequest(); | ||
if (httpServletRequest != null) { | ||
String mdKey = httpServletRequest.getParameter(MetadataBlockServiceBean.SYSTEM_MD_KEY); |
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.
Wouldn't it make sense to move the parameter extraction to DataverseRequest
, adding a method there and avoid a bit more if/else stuff by returning an Optional<>
?
if (httpServletRequest != null) { | ||
String mdKey = httpServletRequest.getParameter(MetadataBlockServiceBean.SYSTEM_MD_KEY); | ||
for (MetadataBlock mdb : changedMDBs) { | ||
if (systemMetadataBlocks.containsKey(mdb.getName())) { |
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.
A thought: it has advantages to use a JSON object here, yes. What would also work: create a JvmSetting
with a placeholder at the end. Less complexity, no costly JSON parsing.
Asking for dataverse.metadata.block-system-metadata-keys.citation
would return the key String
if set for the citation block. You can access this variable setting via the same JvmSettings.lookupOptional()
, but you need to pass the name of the block as the var arg.
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.
OK, this was already heavily reviewed, by @poikilotherm and @PaulBoon, with all the feedback having been addressed. So mine is a review of the reviews for the most part. Everything appears to make sense, so I'm marking it ready for QA to be included in v5.14.
Conflicts: src/main/java/edu/harvard/iq/dataverse/SettingsWrapper.java src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AbstractCreateDatasetCommand.java src/main/java/edu/harvard/iq/dataverse/engine/command/impl/CreateNewDatasetCommand.java
Issues found: |
|
What this PR does / why we need it: Provides a mechanism to restrict metadata edits in specified blocks to those who know the secret key for that block. API only in this PR. See issue for use cases.
Which issue(s) this PR closes:
Closes #9387
Special notes for your reviewer:
Implementation was discussed in tech hours. As part of the work I created some utility methods to determine which blocks have changes when an update is made. These should be more efficient that keeping track of all the individual fields that have changed. I'm not sure if these could be useful elsewhere, e.g. in the version table where indicating just which blocks have changes might be a cheaper option when there are many versions, etc.
Also -
Suggestions on how to test this:
DANS has been testing this PR and @PaulBoon can probably provide more details. In essence, adding a key for a given metadata block should make that block not appear in the edit metadata display and cause all API calls for creating/updating a dataset/version where metadata in this block is changed should now require the extra key parameter for changes to succeed. (see the doc changes). All other blocks and functionality should be unaffected.
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?:
included
Additional documentation:
included