Pre-validates paths for invalid namespaces #473
Conversation
- Adds FedoraInvalidNamespaceException type - New FedoraInvalidNamespaceExceptionMapper - Modifies Service to add new validatePath method - Call validatePath from AbstractService.nodeExists function - Fixes Unit Test that require a mock NamespaceRegistry This resolves https://www.pivotaltracker.com/story/show/61500426
…epositoryException Resolves: https://www.pivotaltracker.com/story/show/78860776
| * @see org.fcrepo.kernel.services.Service#validatePath(javax.jcr.Session, java.lang.String) | ||
| */ | ||
| @Override | ||
| public void validatePath(final Session session, final String path) throws RepositoryException { |
There was a problem hiding this comment.
Please add a unit test for this new method.
There was a problem hiding this comment.
This method does not appear to "Override" any methods from its parent class or interfaces.
Also, it can probably be "private".
There was a problem hiding this comment.
About the unit test. As this function normally returns nothing and throws an exception on error. I've seen that some people suggest catch-exception with mockito? Is there a best practice?
There was a problem hiding this comment.
Here is an example of the machinery that Junit provides for expecting exceptions:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/TransactionServiceImplTest.java#L120
There was a problem hiding this comment.
Also, could you please add an integration test for this. Possibly in:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-kernel-impl/src/test/java/org/fcrepo/integration/kernel/impl/services/NodeServiceImplIT.java
|
Please rebase this commit on master... as master has changed. |
- Adds FedoraInvalidNamespaceException type - New FedoraInvalidNamespaceExceptionMapper - Modifies Service to add new validatePath method - Call validatePath from AbstractService.nodeExists function - Fixes Unit Test that require a mock NamespaceRegistry This resolves https://www.pivotaltracker.com/story/show/61500426
…nto topic-61500426
- Remove integration tests where function replaced with comments - Added new integration test for exists() function.
|
I promise not to add any more commits to this behemoth. |
| * @throws RepositoryException | ||
| */ | ||
| public boolean exists(final Session session, final String path) throws RepositoryException; | ||
| public void validatePath(final Session session, final String path); |
There was a problem hiding this comment.
This method should likely not be a part of the Service.java interface.
|
Please rebase on master once again. |
- Adds FedoraInvalidNamespaceException type - New FedoraInvalidNamespaceExceptionMapper - Modifies Service to add new validatePath method - Call validatePath from AbstractService.nodeExists function - Fixes Unit Test that require a mock NamespaceRegistry This resolves https://www.pivotaltracker.com/story/show/61500426
- Adds FedoraInvalidNamespaceException type - New FedoraInvalidNamespaceExceptionMapper - Modifies Service to add new validatePath method - Call validatePath from AbstractService.nodeExists function - Fixes Unit Test that require a mock NamespaceRegistry This resolves https://www.pivotaltracker.com/story/show/61500426
- Remove integration tests where function replaced with comments - Added new integration test for exists() function.
…nto topic-61500426 Conflicts: fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/services/AbstractService.java
|
This has become a rebasing disaster. See #480 for hopefully a better PR. |
This resolves https://www.pivotaltracker.com/story/show/61500426