Skip to content
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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Astn
Copy link

@Astn Astn commented Feb 23, 2017

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 
       }
    }
}

@Astn Astn changed the title Add basic support for Union types. Add basic support for Union types. Solves #3 Feb 23, 2017
@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

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

and getGraphqlPolymorphicObjectType
@coveralls
Copy link

coveralls commented Feb 24, 2017

Coverage Status

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

@dherault
Copy link
Member

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
Copy link
Author

Astn commented Feb 24, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants