Skip to content
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

Closed
wants to merge 2 commits into from
Closed

Conversation

escowles
Copy link
Contributor

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

escowles referenced this pull request in fcrepo-exts/fcrepo-audit Apr 23, 2015
* 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) {
Copy link

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";
Copy link

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?

Copy link
Contributor Author

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.

Copy link

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@awoods
Copy link

awoods commented Apr 24, 2015

Resolved with: c5e15dd

@awoods awoods closed this Apr 24, 2015
@awoods awoods deleted the setURIProperty branch April 24, 2015 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants