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

Respond with 400 status on request with empty path segments #840

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
2 participants
@awoods
Copy link
Member

commented Jul 14, 2015

protected static HttpHead headObjMethod(final String id) {
return new HttpHead(serverAddress + id);
}

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 15, 2015

Contributor

Did you factor this into the subclasses of AbstractResourceIT every time a HEAD request occurs?

This comment has been minimized.

Copy link
@awoods

awoods Jul 21, 2015

Author Member

yes

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 21, 2015

Contributor

Awesometastic.

@@ -1732,6 +1732,12 @@ public void testJsonLdProfile() throws IOException {
assertEquals("Should be two values!", 2, titles.findValues("@value").size());
}

@Test
public void testEmptyPath() {

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 15, 2015

Contributor

This isn't an empty path. Maybe testPathWithEmptySegment or at least a comment indicating the exact meaning here?

* @author awoods
* @since July 14, 2015
*/
public class InvalidResourceIdentifierException extends RepositoryRuntimeException {

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 15, 2015

Contributor

So this is specifically distinct from when the identifier is valid, but there is no such resource? You might want to add that as a comment. Also, you might want to remark that the notion of validity will depend on the IdentifierConverter chain in use.

@@ -90,6 +94,12 @@ public static void validatePath(final Session session, final String path) {
final String relPath = path.replaceAll("^/+", "").replaceAll("/+$", "");
final String[] pathSegments = relPath.split("/");
for (final String segment : pathSegments) {
// Empty path elements are not valid!

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 15, 2015

Contributor

No, this is the wrong place for this test. Putting it here assumes that HTTP URIs are translated wholemeal into JCR paths, which is the exact assumption that the IdentifierConverter system was designed to break. This test belongs in HttpResourceConverter or near it.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 15, 2015

Contributor

Also, is this looping test better than relPath.contains("//")?

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Author Member

I would take it even further in saying that this test should exist deeper in the core such that it is performed regardless of whether the request is over HTTP or native Java.

I also, completely agree in principle that this check breaks the design and intent of the IdentifierConverter. However, I believe consistency outweighs design principle in this case given the following:

#validatePath() called from HttpResourceConverter#doForward() is already doing something similar to what you are asking for.
#validatePath() called from NodeServiceImpl#exists() is currently used in 6 different places in the RESTful services: FedoraLdp and FedoraNodes. These calls from RESTful services to NodeServiceImpl#exists() are the source of the real issue of breaking the IdentifierConverter design.

I think the future refactoring of the #exists() calls to be more in line with the IdentifierConverter design will be much simpler and cleaner if we keep the validation logic consolidated versus scattering it throughout the codebase in the short term.

Your other comments can be trivially addressed once we get this question sorted.

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jul 17, 2015

Contributor

No, HttpResourceConverter#doForward() and NodeServiceImpl#exists()are correct usage of #validatePath() and #validatePath() is fine. You are implementing the new test in the wrong place. It should not be in #validatePath() This method processes JCR paths, but you have been asked to process an HTTP URI. The new test should be in HttpResourceConverter and nowhere else, and it should not be used anywhere else. You should also refactor FedoraLdp and FedoraNodes to avoid using #validatePath() via NodeService, and instead to use a validation strategy based on the use of an IdentifierConverter.

Consistency in error is no virtue.

This comment has been minimized.

Copy link
@awoods

awoods Jul 17, 2015

Author Member

It is difficult to agree or disagree with your statements without knowing exactly to which those locations or this method are referring. Would you please edit your comment to swap in the values for the references?

@awoods awoods force-pushed the awoods:fcrepo-1537 branch from a2f9f79 to 4afc25f Jul 21, 2015

@ajs6f

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2015

Squash-merged via:
01305e2

@ajs6f ajs6f closed this Jul 21, 2015

@awoods awoods deleted the awoods:fcrepo-1537 branch Jul 21, 2015

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.