Sparqlbuilder should reuse IRI validation code

[barthanssens]

See comment on GH-1038, currently it is assumed (hard-coded) that IRIs start with either mailto:, http: or https:
So the code will break on other valid schemes like urn: etc.
Recommended approach is to reuse the existing IRI validation code

[reckart]

@jeenbroekstra I'm considering to implement this and currently am looking for the IRI validation code in RDF4J. Any pointers?

[reckart]

Is it org.eclipse.rdf4j.model.util.URIUtil.isValidURIReference(String)?

[reckart]

Or just call ParsedIRI.create(iri) and see if it throws an exception?

[abrokenjester]

The URIUtil is somewhat obsolete, as that only does validation for URIs, not IRIs. ParsedIRI is probably the place to go. It's currently not really set up to be used as a util outside of usage in the parsers, but it has very complete support for RFC 3987 (IRI) syntax validation.

[anmolsinghal98]

I would like to work on this issue. Please suggest what needs to be done regarding this.

[abrokenjester]

Hi @anmolsinghal98 that'd be great! Please have a look at the contributor guidelines first, in particular the points on how to sign the Eclipse Contributor Agreement. Let me know if you need any help with any of it.

As for where to start with this issue: have a look at the sparqlbuilder code, particularly the part that parses/validates IRIs. A good way to approach might be to write some tests that use various legal and illegal iri strings and then fix test failures as you go. Like said above: the ParsedIRI class contains very complete validation code, so it would be ideal if you could reuse that.

Feel free to set up an early draft and, even if it doesn't work yet, submit as draft PR so we can discuss.

[anmolsinghal98]

Thank you for your guidance. Will proceed accordingly and let you know in case of difficulties.

[deepali17043]

@anmolsinghal98 , @29kritika and I have signed the Eclipse Contributor Agreement. Now as per contributor guidelines, we'd like to inform that we would be working on the issue.

[anmolsinghal98]

@jeenbroekstra I have made a pull request adding some tests for ParsedIRI class. The same is being used in the Sparqlbuilder Rdf class to create and instantiate IRIs. Please let me know how to proceed with the issue. I am yet to determine where exactly the IRI instantiation is failing.

[abrokenjester]

@jeenbroekstra I have made a pull request adding some tests for ParsedIRI class. The same is being used in the Sparqlbuilder Rdf class to create and instantiate IRIs. Please let me know how to proceed with the issue. I am yet to determine where exactly the IRI instantiation is failing.

...that is surprising - I had it in my head that the SparqlBuilder didn't use the ParsedIRI at all.

Let me take a closer look, but it might be that this issue has in fact already been fixed.

[abrokenjester]

Hm. From what I can tell we indeed already use ParsedIRI.create to validate IRI syntax - this was fixed as part of #1321.

However, what the SparqlBuilder currently does is just assume that something is a prefixed name rather than a full IRI if validation fails, so it returns the original input string as the value.

I'm afraid this isn't a simple bug that can be easily fixed. The builder should verify that, if something doesn't parse as a legal IRI, it is at least a legal prefixed name (meaning that the prefix is known and together they form a valid IRI). But the way the builder is designed doesn't easily make that possible.

[abrokenjester]

We'll need to do some design and set some expectations around what we want SparqlBuilder to do. Currently, it is a very light-weight DSL that just produces a query string without doing much of anything in terms of validating that the result is legal SPARQL.

I would suggest to park this issue for now, and perhaps open a separate discussion around how we could best approach that.

[anmolsinghal98]

Alright. Thanks for the clarification.

[reckart]

IMHO the builder should not be too sophisticated in terms of verification. I have been able to work around many shortcomings simply because the current builder architecture is lightweight and forgiving.