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
Conversation
// 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 + "."); |
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.
Throw: FedoraInvalidNamespaceException
instead of RepositoryException
RepositoryException as suggested.
MalformedRdfException and FedoraInvalidNamespaceException as suggested.
Resolved with: 38d5939 |
|
||
final RdfStream replacementStream = new RdfStream().namespaces(inputModel.getNsPrefixMap()); | ||
|
||
// reject if update request contains any fcr namespacess | ||
final String fcrNS = inputModel.getNsPrefixURI("fcr"); |
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.
@awoods Doesn't this prevent round-tripping graphs that fcrepo4 provides with an fcr
prefix?
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.
fcrepo4 never uses the fcr
prefix. Please let me know if you find it anywhere in the code.
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.
Ok, if that's true.. why isn't this feature implemented down in https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/converters/PropertyConverter.java#L131?
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.
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?
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.
Sure. Just one question:
What's the purpose of the method https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/converters/PropertyConverter.java#L110, fcr to jcr namespace conversion?
I see it converted to jcr in https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/rdf/converters/PropertyConverter.java#L120
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 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.
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.
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.
https://jira.duraspace.org/browse/FCREPO-1244