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

Reject fedora resource property update that contains fcr namespace prefix. #673

Closed
wants to merge 3 commits into from

Conversation

lsitu
Copy link
Contributor

@lsitu lsitu commented Dec 6, 2014

// reject if update request contains any fcr namespacess
final String fcrNS = request.getPrefix("fcr");
if (StringUtils.isNotBlank(fcrNS)) {
throw new RepositoryException("Update content contains fcr namespace " + fcrNS + ".");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw: FedoraInvalidNamespaceException instead of RepositoryException

MalformedRdfException and FedoraInvalidNamespaceException as suggested.
@awoods
Copy link

awoods commented Dec 11, 2014

Resolved with: 38d5939

@awoods awoods closed this Dec 11, 2014

final RdfStream replacementStream = new RdfStream().namespaces(inputModel.getNsPrefixMap());

// reject if update request contains any fcr namespacess
final String fcrNS = inputModel.getNsPrefixURI("fcr");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awoods Doesn't this prevent round-tripping graphs that fcrepo4 provides with an fcr prefix?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fcrepo4 never uses the fcr prefix. Please let me know if you find it anywhere in the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing this further down the stack makes sense.
@lsitu, would you be available to rollback your updates to FedoraResourceImpl.java and add logic to PropertyConverter.java?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the method getJcrNamespaceForRDFNamespace can now safely be removed:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/converters/PropertyConverter.java#L120

...but it would be good if you could double check to see if it is indeed true.

PropertyConverter.getPropertyNameFromPredicate() is used to get the backend JCR property for a given RDF predicate, and to register the namespace if it is not found in the backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean we can remove line https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/converters/PropertyConverter.java#L120 safely? But I don't see the RDF predicate is converted without line 120 above.

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