nelson-ai / semantic-graphql

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

Add basic support for Union types. Solves #3 #4

Open Astn opened 7 years ago

Astn commented 7 years ago

This solves #3 Please review and let me know what kind of changes are needed.

The current behavior when dealing with a range of resources was to return an interface for Resource. The range was being ignored.

function getGraphqlPolymorphicObjectType(g/*, ranges*/) {
    // TODO: 
    return getGraphqlInterfaceType(g, rdfsResource);
}

This pull request changes that behavior to now use the range of resources and return GraphQLUnionType with those resources as a part of the union.

This will break queries that previously pulled properties directly out of rdfsResource e.g.,

query {
    polymorphicObjectReference {
         id   // <- id from rdfsResourceInterface
    }
}

A migration path for the same behavior could be:

query {
    polymorphicObjectReference {
        ... on rdfsResourceInterface {
           id 
       }
    }
}

Additionally, the other types and interfaces from the Union should now be available e.g.,

query {
    polymorphicObjectReference {
        ... on rdfsResourceInterface {
           id 
       }
       ... on unionTypeFoo {
           foo 
       }
       ... on unionTypeBarInterface {
           bar 
       }
    }
}
coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.8%) to 73.904% when pulling c017fd957f98df5fd8622123c596968352927556 on athlinks:AddBasicUnionSupport into 2a2b6a7acd787e4abdc2f7cc435acecb02282c95 on nelson-ai:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.9%) to 77.662% when pulling 07e3b11d99d7872ef68b8f412fcb10f8831d973e on athlinks:AddBasicUnionSupport into 2a2b6a7acd787e4abdc2f7cc435acecb02282c95 on nelson-ai:master.

dherault commented 7 years ago

Re @Astn, thanks for the PR, this is great. Unfortunatly I don't think it should be merged. The main reason is it adds too much complexity for only one feature. I'm not sure yet how the main engine of the lib will evolve, and how it will takle concerns like polymorphism

What about the following: if the range is an owl:unionOf or owl:intersectionOf, return RdfResourceInterface as a starting point for support.

Astn commented 7 years ago

I'm looking at http://facebook.github.io/graphql/#sec-Fragments-On-Composite-Types and trying to get a the same behavior out of this graphql library.

They have the example

fragment fragOnUnion on CatOrDog {
  ... on Dog {
    name
  }
}

Which is the exact use case I have if you replace CatOrDog with Agent.

fragment fragOnAgent on AgentUnion {
  ... on Person {
    name
  }
}

The owl definition looks like this:

schema:agent
  rdf:type owl:ObjectProperty ;
  rdfs:comment "The direct performer or driver of the action (animate or inanimate). e.g. *John* wrote a book." ;
  rdfs:domain schema:Action ;
  rdfs:label "agent" ;
  rdfs:range [
      rdf:type owl:Class ;
      owl:unionOf (
          schema:Organization
          schema:Person
        ) ;
    ] ;

I think part of the problem we are dealing with is that schema:Orginization and schema:Person do not implement an "agent" interface. But that case is pervasive in schema.org. Both Person and Orginization subclass owl:Thing

schema:Person
  rdf:type owl:Class ;
  rdfs:comment "A person (alive, dead, undead, or fictional)." ;
  rdfs:label "Person" ;
  rdfs:subClassOf owl:Thing ;
  owl:equivalentClass <http://xmlns.com/foaf/0.1/Person> ;
.
schema:Organization
  rdf:type owl:Class ;
  rdfs:comment "An organization such as a school, NGO, corporation, club, etc." ;
  rdfs:label "Organization" ;
  rdfs:subClassOf owl:Thing ;
.

So at least in this case where we have an ObjectProperty with a Range [ Union ] returning a RdfResourceInterface wont work for us, as the consumer of the API wont get any information about what could be returned, and would be forced to make a subsequent query directly for the concrete type they got a resourceID back for. That pushes type resolution back to the client increasing client complexity, and also significantly increasing latency and thus user experience.

I agree that adding a big chunk of code for one feature can feel heavy handed, and I'm more then happy to take suggestions or find ways to simplify the solution. But I have to have a way to keep moving forward and have a solution that has our graphql API to supporting fragments over ObjectProperties that reference owl:UnionOf in an idiomatic graphql style.

I'd love your suggestions on how to solve this problem in a way that works for both of us.