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
Users can now delete large literal property values. #911
Conversation
- Large literals that are stored in the binary store are now removeable via standard means. Resovles: https://jira.duraspace.org/browse/FCREPO-1561
if (v.equals(valueToRemove)) { | ||
remove.set(true); | ||
return false; | ||
String strVal = ""; |
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.
Better final String strVal;
.
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.
Okay, I'll change that. Thanks!
- Users can now delete large literals that, because of their size, get stored in the binary store. This commit is just to clean up the code Resolves: https://jira.duraspace.org/browse/FCREPO-1561
LOGGER.debug("Removing value from property {}", propertyName); | ||
property.remove(); | ||
|
||
String strVal = ""; |
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.
Again, better final String strVal;
.
-Users can now delete large literals that had been stored in the binary store. Resolves: https://jira.duraspace.org/browse/FCREPO-1561
New check in. This is much cleaner (I hope!). I'm not sure how to test the single-value case. It does pass all the repository tests, and I know one tests a remove a singe-value property. Is that enough? (I did test the multi-value case as much as I could.) Sort of an aside but worth mentioning -- from testing, I noticed that it will not get into this removeNodeProperty method if you try to remove a value on a property and that value doesn't exist. That check is happening somewhere above this code. Overall it's a silent fail, but begs the question of "what if I mistyped the value and really do want to delete "Foo", and I'm not really paying attention to the fact that it failed because I typed DELETE WHERE { <> ex:test "foo" } instead." Sort of like you were saying in an above comment - the user goes on their way thinking it succeeded. Separate bug? The original bug is resolved by my changes though. |
remove.set(true); | ||
return false; | ||
} | ||
|
||
LOGGER.warn("Failed to find value for property '{}'. Unable to compare value (type: {}) " + |
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.
Is this LOGGER statement correct? We are inspecting a multi-valued property here. If the value to be removed (strValueToRemove
) does not equal strVal
, all that means is that the value to remove is another one in the list.
For example, the multi-valued property may have the following values: "a", "b", "c"
. If this request is trying to remove "b", then "a" and "c" will fall through to this LOGGER statement.
That is a perfectly expected case, not a "warning".
And the wording Failed to find...
seems to raise unnecessary flags.
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.
You're right, I'll remove that. It would be confusing and unnecessary 'noise'.
This looks good... and works as desired. Thank you! Since that test class uses the following configuration which specifies There are several examples of using sparql-update to update properties starting here: |
- Added test for bug fix for deletion of large literals bug Resolves: https://jira.duraspace.org/browse/FCREPO-1561
Fcrepo 1561 incremental
- Moved test to a place where it actually works Part of resolving: https://jira.duraspace.org/browse/FCREPO-1561
Resolved with: 8cceaeb |
via standard means.
Resolves: https://jira.duraspace.org/browse/FCREPO-1561