-
Notifications
You must be signed in to change notification settings - Fork 541
File Metadata Update - Empty values clear fields #11439
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
Changes from 11 commits
9baf2d5
6d871aa
fd800fc
dea4904
8dcb065
2fce5fd
26f07b7
10939ce
f5ddcff
1615fb9
8a57fc8
6fedf6e
eaf49a9
1320d51
b61bb1c
8ef92d2
7563cf8
7a7a84f
0eaca6c
0c72a89
509e55a
c465180
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| ### Edit File Metadata empty values should clear data | ||
|
|
||
| Previously the API POST /files/{id}/metadata would ignore fields with empty values. Now the API updates the fields with the empty values essentially clearing the data. Missing fields will still be ignored. | ||
|
|
||
| An optional query parameter (sourceInternalVersionTimestamp) was added to ensure the metadata update doesn't overwrite stale data. | ||
|
|
||
| See also [the guides](https://dataverse-guide--11359.org.readthedocs.build/en/11359/api/native-api.html#updating-file-metadata), #11392, and #11359. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import edu.harvard.iq.dataverse.search.savedsearch.SavedSearchServiceBean; | ||
| import edu.harvard.iq.dataverse.settings.SettingsServiceBean; | ||
| import edu.harvard.iq.dataverse.util.BundleUtil; | ||
| import edu.harvard.iq.dataverse.util.DateUtil; | ||
| import edu.harvard.iq.dataverse.util.FileUtil; | ||
| import edu.harvard.iq.dataverse.util.SystemConfig; | ||
| import edu.harvard.iq.dataverse.util.json.JsonParser; | ||
|
|
@@ -51,6 +52,7 @@ | |
|
|
||
| import java.io.InputStream; | ||
| import java.net.URI; | ||
| import java.time.Instant; | ||
| import java.util.*; | ||
| import java.util.concurrent.Callable; | ||
| import java.util.logging.Level; | ||
|
|
@@ -451,6 +453,21 @@ protected void validateInternalVersionNumberIsNotOutdated(Dataset dataset, int i | |
| } | ||
| } | ||
|
|
||
| protected void validateInternalTimestampIsNotOutdated(DataFile dataFile, String sourceInternalVersionTimestamp) throws WrappedResponse { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming we go this route, I think we'll want to replace the method above as well - perhaps convenience methods that take a file or dataset calling one method having a datasetversion param that has the main logic?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting overloading or a single method
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting changing the dataset method to also compare the timestamp?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm mostly suggesting that we have a validateInternalVersion(DatasetVersion dv, String v) method - with the logic to get that datasetversion from the dataset or file outside that. I hadn't thought about whether that's two methods or one for dvObject (or whether it would be easier to get the right DatasetVersion back in the calling methods so we don't have to find it again for this validation test). I am also suggesting that whatever we decide to do should be done for both the file and dataset api calls, so ~yes, assuming we're agreed on using the timestamp, it would mean removing the existing dataset check and using your new code.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think refactoring the dataset api should be done in a separate issue
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qqmyers Since the previous issue that introduced the sourceInternalVersionNumber hasn't been released I will refactor it with this issue
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stevenwinship Please, after you are done with this PR let me know if there are any breaking changes we will need to address regarding the dataset metadata update endpoint usage, thanks 🫡
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @g-saracca Ok. Just so you know the qp sourceInternalVersionNumber is removed and replaced with sourceLastUpdateTime. So to update you'll need to send the lastUpdateTime instead of the internalVersionNumber
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I look at the new qp I realize I don't like the name. I will be refactoring the qp name from sourceInternalVersionTimestamp to sourceLastUpdateTime to match the json attribute |
||
| Date date = sourceInternalVersionTimestamp != null ? DateUtil.parseDate(sourceInternalVersionTimestamp, "yyyy-MM-dd'T'HH:mm:ss'Z'") : null; | ||
| if (date == null) { | ||
| throw new WrappedResponse( | ||
| badRequest(BundleUtil.getStringFromBundle("jsonparser.error.parsing.date", Collections.singletonList(sourceInternalVersionTimestamp))) | ||
| ); | ||
| } | ||
| Instant instant = date.toInstant(); | ||
|
stevenwinship marked this conversation as resolved.
|
||
| if (dataFile.getFileMetadata().getDatasetVersion().getLastUpdateTime().toInstant().getEpochSecond() != instant.getEpochSecond()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a > relationship be used here? Is there ever a case where the exact timestamp isn't known? E.g. some edit operation (like changing a file restriction or editing terms) where you'd know the local time of the api call but not the exact db timestamp and would want to avoid an extra call to get it (while still protecting from someone else having changed it in the next few minutes)? I'm torn - anything but != opens a small hole, but the simplification might be worth the small risk (although there are possible time skew issues).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think if you were restricting access you would not need to send the timestamp. I can't image one person restricting and another person unrestricting. If we don't know the timestamp then we need to have a configuration value to say an update can't be made within X minutes of another update. I think it's normal to get the data before updating. Any ui would do this and I don't think a straight api call to make a blind change is something we should condone.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, we allow blind by making this parameter optional. |
||
| throw new WrappedResponse( | ||
| badRequest(BundleUtil.getStringFromBundle("abstractApiBean.error.datafileInternalVersionTimestampIsOutdated", Collections.singletonList(sourceInternalVersionTimestamp))) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| protected DataFile findDataFileOrDie(String id) throws WrappedResponse { | ||
| DataFile datafile; | ||
| if (id.equals(PERSISTENT_ID_KEY)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3205,3 +3205,4 @@ updateDatasetFieldsCommand.api.processDatasetUpdate.parseError=Error parsing dat | |
|
|
||
| #AbstractApiBean.java | ||
| abstractApiBean.error.datasetInternalVersionNumberIsOutdated=Dataset internal version number {0} is outdated | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove this bundle entry? It seems to be unused now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
| abstractApiBean.error.datafileInternalVersionTimestampIsOutdated=File Metadata internal version timestamp {0} is outdated | ||
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 trying to get this in 6.7? If not, this should be placed under "v6.8" in the changelog.
sourceInternalVersionNumberis gone right? Should we mention that?Also, I would suggest replacing the smart quotes with straight quotes and writing "JSON" in all caps.
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.
fixed