-
Notifications
You must be signed in to change notification settings - Fork 3
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
JCL-431: Improve containment validation #653
Conversation
if (getIdentifier().getPath().endsWith("/")) { | ||
final String container = normalize(getIdentifier()); |
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.
Newly, the condition is not normalized. This seems intentional.
But what happens with URIs of the form x/.
and x/y/..
?
Maybe we should normalize before checking whether it ends in a slash.
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.
getIdentifier
from RDFSource
via SolidRDFSource
is a URI
and getPath
is not normalized.
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've updated this to normalize the URI path
if (!getIdentifier().getPath().endsWith("/")) { | ||
messages.add("Container URI does not end in a slash"); | ||
} | ||
|
||
// Get the normalized container URI | ||
final String container = normalize(getIdentifier()); |
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.
As above.
Maybe worth extracting something like isContainer
for readability and consistency.
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.
done in 2b4b916
if (!object.getIRIString().startsWith(container)) { | ||
// Out-of-domain containment triple object | ||
|
||
// 1 URI Structure Tests |
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.
What do these numbers refer to? I looked in several place I imagined would be relevant, but haven't found anything like this.
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.
That was more for my own organization. I've backed that out
Resolves #634
This improves containment validation by considering more cases to be handled, specifically those containment triples that include fragment and query components.
The test cases have been expanded to include the additional cases.
The implementation makes more extensive use of normalized Java URI objects to achieve the improved validation.