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

Using URLDecoder instead of String.replaceAll #429

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@escowles
Copy link
Contributor

commented Jul 28, 2014

escowles referenced this pull request Jul 28, 2014

Handle spaces in path names
- 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 ) {

This comment has been minimized.

Copy link
@awoods

awoods Jul 28, 2014

Member

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.

This comment has been minimized.

Copy link
@escowles

escowles Jul 28, 2014

Author Contributor

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.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 28, 2014

Contributor

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?

This comment has been minimized.

Copy link
@awoods

awoods Jul 28, 2014

Member

@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.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 28, 2014

Contributor

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?

escowles added some commits Jul 28, 2014

@awoods

This comment has been minimized.

Copy link
Member

commented Jul 28, 2014

@awoods awoods closed this Jul 28, 2014

@cbeer cbeer deleted the urldecode branch Oct 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.