nelson-ai / semantic-graphql

Create GraphQL schemas from RDF ontologies
MIT License
28 stars 4 forks source link

isIRI check is out of spec #1

Closed Astn closed 7 years ago

Astn commented 7 years ago

First off, thank you for open sourcing this project! I have found that is has already saved me and my company a lot of time! Thank you for all the work you put into this!


According to https://tools.ietf.org/html/rfc3987#section-2.2 An IRI must start with a SCHEME followed by a ":"

IRI = scheme ":" ihier-part [ "?" iquery ] [ "#" ifragment ]

Where

scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

Currently isIRI is checking that:

iri.startsWith('http')

Given that IRI is a superset of URI, the examples from URI @ https://tools.ietf.org/html/rfc3986#section-1.1.2 show that all of the following should pass the isIRI check.

1.1.2.  Examples
   The following example URIs illustrate several URI schemes and
   variations in their common syntax components:

      ftp://ftp.is.co.za/rfc/rfc1808.txt
      http://www.ietf.org/rfc/rfc2396.txt
      ldap://[2001:db8::7]/c=GB?objectClass?one
      mailto:John.Doe@example.com
      news:comp.infosystems.www.servers.unix
      tel:+1-816-555-1212
      telnet://192.0.2.16:80/
      urn:oasis:names:specification:docbook:dtd:xml:4.1.2

I suggest that for we replace

function isIri(iri) {
   return typeof iri === 'string' && iri.startsWith('http'); // very basic check
}

with

function isIri(iri) {
  // According to: https://tools.ietf.org/html/rfc3987#section-2
  // IRI            = scheme ":" ihier-part [ "?" iquery ] [ "#" ifragment ]
  // every IRI requires a scheme followed by ":".
  return typeof iri === 'string' && iri.includes(':'); 
}

I believe this would be a significant step forward.

If its determined that even further improvements be made, I would suggest that we ensure that the scheme portion does not include any of the reserved characters. see:

   reserved       = gen-delims / sub-delims
   gen-delims     = ":" / "/" / "?" / "#" / "[" / "]" / "@"
   sub-delims     = "!" / "$" / "&" / "'" / "(" / ")"
                  / "*" / "+" / "," / ";" / "="

I can submit a pull request for this shortly, and have been working on a few fixes like this in the following fork : https://github.com/athlinks/semantic-graphql/tree/addUnionSupport

dherault commented 7 years ago

Hi @Astn, thanks for the first issue of the lib :). Please note that even if it does the job, it is still not production-ready. I was planning to include semantic-toolkit as a dependency to perform this kind of check (and support blank nodes and many other semantic features). You will find your fix in it !