GH-2370: Improve validation of dataset graph names

[kinow]

GitHub issue resolved #2370

Pull request Description:

@afs, @jmkeil, first stab at #2370. Had a look at ¹ and ², and I saw some parts of the text mentioning "percent-encoded" in requests, and that the backend is supposed to return 400, as it's doing now. From quickly reading those two (skipping parts, trying to find info for validation), I think

1. empty is valid
2. spaces are no-no
3. otherwise it must be a URI
4. encoded URL is no-no, it must be a valid URI, without encoded parts

I extended the current validation to also check 3 and 4 now. Does that sound right?

This is a draft until I confirm that the fix looks good, then I will either write the tests, or modify the PR again to move the code to some JS module as that a lot easier to write unit-test against, instead of testing code buried somewhere in Vue code/components.

Cheers

---

• Tests are included.
• Documentation change and updates are provided for the Apache Jena website
• Commits have been squashed to remove intermediate development commit messages.
• Key commit messages start with the issue number (GH-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.

---

See the Apache Jena "Contributing" guide.

Footnotes

1. https://www.w3.org/TR/2013/REC-sparql11-http-rdf-update-20130321/#http-post ↩

2. https://www.ietf.org/rfc/rfc3986.html ↩

[kinow]

"" → OK
" " → ERROR
"snoopy" → ERROR
"http://example.org/dataset" → OK
"http://example.org/dataset#graph" → OK
"http://example.org/dataset" → OK
"http://example.org/dataset#graph?Aaa" → OK
"http://example.org/dataset%23graph" → ERROR

[jmkeil]

Any valid absolute IRI should be permitted, including other schemes than http/https, like urn:uuid:6e8bc430-9c3a-11d9-9669-0800200c9a66.

"http://example.org/dataset%23graph" → ERROR

I think this should be OK too, as IRIs serve as identifiers in this case and therefore the Simple String Comparison for equivalence apples. (But @afs probably knows for sure.)

Maybe better leaf it up to the back-end, as it already has sophisticated methods to check that?

[kinow]

Any valid absolute IRI should be permitted, including other schemes than http/https, like urn:uuid:6e8bc430-9c3a-11d9-9669-0800200c9a66.

Haven't tried on this PR, but a quick test on the browser console accepts "urn:uuid:6e8bc430-9c3a-11d9-9669-0800200c9a66" too.

new URL("urn:uuid:6e8bc430-9c3a-11d9-9669-0800200c9a66")
URL { href: "urn:uuid:6e8bc430-9c3a-11d9-9669-0800200c9a66", origin: "null", protocol: "urn:", username: "", password: "", host: "", hostname: "", port: "", pathname: "uuid:6e8bc430-9c3a-11d9-9669-0800200c9a66", search: "" }

I think this should be OK too, as IRIs serve as identifiers in this case and therefore the Simple String Comparison for equivalence apples. (But @afs probably knows for sure.)

👍

Maybe better leaf it up to the back-end, as it already has sophisticated methods to check that?

In some other systems I've seen the UI sending a request to the backend to validate (e.g. Jenkins does that)... this way validation was only done in the backend, and the UI simply relied on the response being OK or NOK + the error explanation.

It's harder to test without the backend, and has an extra cost on the server, but could be a solution here too.
So that should pass the validation without issues.

Happy to implement either way. I think this new version at least improves the previous one. Not sure if there are some corner cases covered by the backend that are not handled correctly in JS though...

Thanks @jmkeil !

[jmkeil]

Not sure what would be the best solution. But the simplest solution I could think of would be to just do the request with the encoded graph name IRI and the back-end would complain, if it is invalid. Of course, that would have the draw back that the user might send a large payload only to learn about an invalid input. This could be avoided by sending a trial request with empty payload to test the graph name IRI. That way, no changes of the back-end are needed. Would that be feasible from the testing-perspective?

[kinow]

Would that be feasible from the testing-perspective?

I think that's doable too. Let's wait to see if @afs has other ideas or suggestions.

Thanks @jmkeil !

[kinow]

ping @afs , @rvesse ☝️ Don't recall much about the context here, but reading the comments, looks like I was waiting for other opinions on the best approach here. Any thoughts on this?

[afs]

Hi @kinow,

Reading back ...

If the client is doing checking and it will give better error messages. e.g. no actual spaces !!!
The server is going to check anyway but all it can say to a request is "no" or "yes".

If the client side parses the URI with uri-js that should be good. It is RFC 3986 compliant.

Addition checks for common mistakes (spaces!) are nice - better error messages.

Aside :: On the server side, Java java.net.URI is not good - it's more like the old RFC 2396, not RFC3986. Jena does not use it.

Percent should be allowed end-to-end - a URI really does have %-2-3 in it. The URI parser should be happy then when sent the request should be making it a safe - avoid applying the algorithm twice, once in Jane JS code and once by the HTTP client. The receiving code should decode the request properly once.

[kinow]

Created a separate module/function to make it easier to test/isolate the code.

[kinow]

The text had the part about encoded values... I removed it as in reality the error from the SPARQL docs (http%3A//www.example.com/other/graph) is captured in the new URL call. I believe the JS URL follows this doc, which references RFC 3986 & 3987 https://url.spec.whatwg.org/

[kinow]

Now the code in this UI component is dumb, and the logic is in the pure JS module (easier to test, etc.)

[kinow]

@afs, @ jmkeil I trust with this unit test it will be easier to review the code and decide if the new validation is looking good or not 🙂

[kinow]

Rebased, updated the code to simplify review... it should be ready for review again, @jmkeil , @afs.

[kinow]

UI when you visit the page to upload data:

When you enter a valid graph name:

And when you enter an invalid graph name:

There's one thing pending, and it's to block the "upload now" of each individual file. I can do that and add another test before it's merged, but we can review the validation for now.