Using URLDecoder instead of String.replaceAll #429
Conversation
- IT demonstrating failure when creating an object with a space in its path and then trying to perform a SPARQL update on it Resolves: https://www.pivotaltracker.com/story/show/72296804
| } catch ( UnsupportedEncodingException ex ) { | ||
| LOGGER.warn("Required encoding (UTF-8) not supported, trying undecoded path",ex); | ||
| path = subjects.getPathFromSubject(subject); | ||
| } catch ( NullPointerException ex ) { |
There was a problem hiding this comment.
Why is there a NPE catch clause? I do not believe "subjects" nor "subject" will ever be null at this point, and I am not seeing an NPE defined on the decode() method.
There was a problem hiding this comment.
Without the NPE catch clause, four tests fail because getPathFromSubject() returns null:
JcrPropertyStatementListenerTest.testAddedProhibitedStatement:150 » NullPointer
JcrPropertyStatementListenerTest.testAddedStatement:166 » NullPointer
JcrPropertyStatementListenerTest.testAddRdfType:257 » NullPointer
JcrPropertyStatementListenerTest.testAddRdfTypeForNonMixin:301 » NullPointer
I'd assumed that there were circumstances where this could be null, but if they just need to be mocked in the tests, then we can do that instead of catching the NPE.
There was a problem hiding this comment.
The question is also about the semantics of getPathFromSubject(). Is it defined for any possible subject in the Fedora-managed namespaces, or only for some subset thereof, the subset that is backed by actual persisted JCR nodes?
There was a problem hiding this comment.
@escowles, I think that instead of working around erroneous mocking, we should write the correct functional code and mock accordingly. The "testAddedStatement" works by changing line:158 to:
when(mockSubjects.getPathFromSubject(mockSubject)).thenReturn("/some/path");
when(mockSession.getNode("/some/path")).thenReturn(mockSubjectNode);
I assume a similar update will work for the other tests.
@ajs6f, as for the semantics of getPathFromSubject(), it appears to depend on the IdentifierTranslator implementation. HttpIdentifierTranslator requires a valid JCR input path.
There was a problem hiding this comment.
Well, that's a very basic problem. OO design 101 says that the caller of getPathFromSubject() shouldn't need to care (shouldn't even be able to care) about the impl-- IOW the semantics should be defined in IdentifierTranslator. (That's the old GraphSubjects, which I just renamed.)
Maybe a topic for committer discussion?
|
Resolved with: 3279475 Related to: https://www.pivotaltracker.com/story/show/72296804 |
See 494ab59#commitcomment-7172975