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
Adding FedoraResource.setURIProperty method #781
Conversation
* Adding InternalAuditor to create repository nodes for audit events * Updating project to work with current fcrepo4 * Switched to Jackson-2 JSON parsing library.
@@ -262,6 +264,15 @@ public Property getProperty(final String relPath) { | |||
} | |||
|
|||
@Override | |||
public void setURIProperty(final String relPath, final URI value) { |
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.
Can you add a Javadoc note like the one you have on this commit that indicates that no references are created with this method. That would be helpful in the future to let people understand the specialness of this method.
…rs from other functionality, and what the tradeoff is
@@ -395,6 +398,14 @@ public void testGetProperty() throws RepositoryException { | |||
} | |||
|
|||
@Test | |||
public void testSetURIProperty() throws URISyntaxException, RepositoryException { | |||
final String prop = "premis:hasEventRelatedObject"; | |||
final String uri = "http://localhost:8080/rest/1"; |
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.
The argument name and javadoc indicates that the URI is a relative-path:
https://github.com/fcrepo4/fcrepo4/pull/781/files#diff-688c65561dd248cf2a0096c799be5d18R89
This integration test is not a relative-path, and furthermore, the host and port (in a production scenario) may change or need idTranslation.
To be clear, are you expecting Resource.setURIProperty() to use full URLs or relative-paths? If full URLs, then are we concerned about context translation?
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.
The relPath argument is the property name, not the value. I copied the parameters javadoc from the getProperty() method for consistency, but I could update this to be clearer that it's the property name in abbreviated format "premis:hasEventRelatedObject", not the full URI.
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 also wonder about passing in values as full URLs that do not pass through an idTranslator. Is this a concern?
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.
It's a concern if those values are URIs of things in the repo.
On Thursday, April 23, 2015, Andrew Woods notifications@github.com wrote:
In
fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/FedoraResourceImplTest.java
#781 (comment):@@ -395,6 +398,14 @@ public void testGetProperty() throws RepositoryException {
}@Test
- public void testSetURIProperty() throws URISyntaxException, RepositoryException {
final String prop = "premis:hasEventRelatedObject";
final String uri = "http://localhost:8080/rest/1";
I also wonder about passing in values as full URLs that do not pass
through an idTranslator. Is this a concern?—
Reply to this email directly or view it on GitHub
https://github.com/fcrepo4/fcrepo4/pull/781/files#r28990896.
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.
The URIs are references to things in the repo -- identifiers of resources related to audit events. The intention with this approach is to build those URIs in the audit processor, and store them in the repository as URIs, so they are not deleted if the resource is deleted.
Resolved with: c5e15dd |
Adds kernel API for setting URI properties without conversion to reference types, to support the audit use case of preserving audit events that link to repository resources after those resources have been deleted. See fcrepo-exts/fcrepo-audit@3d7d6d3#commitcomment-10865344