Scaffold object type inheritance

[chillu]

Related to Scaffold input type inheritance

In line with ORM conventions, we want to expose fields from all subclasses when querying the base class (e.g. SiteTree). In contrast to the ORM, GraphQL exposes a type or interface for fields of the returned objects. While we could flatten all fields to the base type, it means a lot of fields which don't make sense without further context:

type SiteTree {
    id
    created
    title
    content
    myField <-- from Page
    redirectionType <-- from RedirectorPage
    externalUrl <-- from RedirectorPage
}

Instead, I propose we create distinct types for each subclass, each of which is a superset of its parent class.

type SiteTree {
    id
    created
    title
    content
}

type Page {
    id
    created
    title
    content
    myField
}

type RedirectorPage {
    id
    created
    title
    content
    myField
    redirectionType
    externalUrl
}

This structure allows us to declare multiple types as a union in queries:

union SiteTreeTypes = SiteTree | Page | RedirectorPage

type readSiteTree {
  [SiteTreeTypes]
}

The downside is that queries will always have to declare on which type they're expecting fields:

query readSiteTree() {
  ...on SiteTree {
    id
    created
    title
    content
  }
  ...on Page {
    myField
  }
  ...on RedirectorPage {
    redirectionType
    externalUrl
  }
}

While a type can implement more than one interface, a field (query response) can only declare a interface response type (or a union of types) - see discussion on facebook/graphql. We could use a single interface, but there's no "union of interfaces" in GraphQL.

Sam suggested types implementing multiple interfaces, but given the lack of "union of interface" support, I don't see much value in this. Also, it we can't have all types implement a SiteTreeInterface to get around writing ...on SiteTree for fields on the base type - it's either a single interface, or a union of types (verified through a modified ReadFileQueryCreator).

In general, the recommended approach makes it slightly harder to automatically construct GraphQL queries (e.g. for a dynamic React-based GridField, or for a full data exporter). But since most JS UI logic should know which fields it needs in components, I'd see this more of an edge case than a problem in practice. For example, the CMS tree view would always just query the base attributes on the SiteTree type since it doesn't need to display anything else.

There's some tests in the graphql-js reference implementation for this, and an interesting SO discussion on interfaces vs. unions. And a nice Medium posts about interfaces and unions

[sminnee]

I think that this seems to be a reasonable path to get started on.

However, I think that the expectation that we get this immutably perfect on the first try is an unreasonable expectation, and so we should mark all such APIs as @internal or experimental, and allow ourselves the possibility of a refactor in the future.

[chillu]

Most of this has been done in #20, but we don't have a union implementation - what's your take on this, @unclecheese?

[unclecheese]

So just to clarify, the question is, how can I get all descendants of SiteTree, and show all applicable fields, e.g.

readSiteTreeTypes {
  [SiteTreeTypes]
}

{
  "readSiteTreeTypes": [
     { ClassName: "SiteTree", Title: "Page 1", "RedirectURL": null, CustomField: null },
     { ClassName: "Page", Title: "Page 2", "RedirectURL": null, CustomField: "foo" },
     { ClassName: "Page", Title: "Page 3", "RedirectURL": "url" , CustomField: "bar" },
   ]
}

Is that what you're after?

[chillu]

How does your readSiteTreeTypes relate to my readSiteTree example above? The thing with unions is that they affect all query notation, even if you're not interested in the inherited attributes - so it's changing the entire API, hence we need to figure this out soon.

Without unions:

query readSiteTree() {
    id
    created
    title
    content
}

With unions:

query readSiteTree() {
  ...on SiteTree {
    id
    created
    title
    content
  }
}

[unclecheese]

Got it now, sorry, I misread your previous example. I think scaffolding union queries makes perfect sense. So when I add the type redirectorPage, instead of just creating queries for readPages and readSiteTrees that return those respective types, we would use union queries instead. That seems like a pretty small change on the backend.

I guess the question is if that's really what we want to impose on users of the API. (i.e. the ...on descriptions)

[chillu]

I guess the question is if that's really what we want to impose on users of the API. (i.e. the ...on descriptions)

You can't have it both ways unfortunately - using union types means the client can't rely on a field being present by default, so you need the ...on syntax everywhere.

• Option 1: Unions, using ...on, can retrieve properties from all types on the base query (readSiteTree). No direct need for a separate readReadirectorPage query, although the scaffolding will still allow for that specialisation.
• Option 2: No unions, no ...on, easier query creation for GraphQL consumers, need to use type-specific queries (readSiteTree vs. readRedirectorPage). Makes it impossible to return multiple types, forcing more query knowledge into the client. For example, an "page list" list might want to show a summary with the redirection target for a RedirectorPage, and the status code for ErrorPage. That's not possible with this option, you have to run one query per type. Similarly, full content exports from a SIlverStripe database through the API will get quite finnicky.
• Option 3: Unions on particular base queries only (readSiteTree returns SiteTree type only, readSiteTreeWithTypes returns unions). Most flexible for GraphQL consumers, but will likely cause confusion in terms of API surface
• Option 4: Add all fields on a base type, across all subclasses (no need for readRedirectorPage). Harder to read the API since fields will be on the base type without their ancestry context.

I'm leaning towards Option 1, even if it'll make writing queries a bit harder since you have to think about types that you're accessing attributes on.

[PapaBearNZ]

I think a lot of this is use-case dependent. When you look at GraphQL in SS as specifically feeding the CMS then all the ancestry and inheritance information is necessary and hence Option 1 looks to be the path of least resistance.

However, if you are looking at using SS as a data repository in a COPE scenario (which is mainly the angle I have been testing to date), then Option 4 is closer to what is required, because the front end is looking for pure, device-agnostic content only.

Would it be possible to provide either Option 1, or Option 4. configurably through the scaffolder?

[chillu]

Would it be possible to provide either Option 1, or Option 4. configurably through the scaffolder?

Yeah, that's the best of both worlds, and a good medium-term plan. But the SilverStripe Content API use case comes first in terms of core priorities - we need to start locking down the API surface for queries provided by core (such as readFile and readSiteTree).

[chillu]

Thanks for getting this over the line @unclecheese! There's a bit of a gotcha in your sizeof($union) > 1 logic flow: If you scaffold a class without inheritance, build your GraphQL client code around that, and later add a subclass to it, your client code breaks - even if it doesn't care about the additional fields, it'll need to use the ...on syntax now. It's probably enough of an edge case to ignore for now.

[unclecheese]

If the fields are common to all types, you don't need ...on, so I think we're okay, no?

class A extends DataObject {
  private static $db = ['Title' => 'Varchar'];
}

read A {
  Title
}

^ works

class B extends A {
  private static $db = ['SecondTitle' => 'Varchar'];
}

read A {
  Title
}

^ Still works.

read A {
  Title
  SecondTitle
}

^ fails

read A {
  Title
  ..on B {
     SecondTitle
  }
}

^ works

[chillu]

Have you tried this in a GraphQL client? http://facebook.github.io/graphql/#sec-Unions indicates that it's not possible, and from memory I've tried that when I was researching this ticket (and confirmed it wasn't):

With interfaces and objects, only those fields defined on the type can be queried directly; to query other fields on an interface, typed fragments must be used. This is the same as for unions, but unions do not define any fields, so no fields may be queried on this type without the use of typed fragments.

Sticking with the example from your README, the following fails:

query ReadPages {
  readPages {
    edges {
      node {
        Content
      }
    }
  }
}

{
  "data": null,
  "errors": [
    {
      "message": "Cannot query field \"Content\" on type \"SiteTreeWithDescendants\". However, this field exists on \"Page\", \"RedirectorPage\", \"SiteTree\". Perhaps you meant to use an inline fragment?",
      "locations": [
        {
          "line": 5,
          "column": 9
        }
      ]
    }
  ]
}

So the dev is safe until the type has been defined (due to a $manager->hasType() check in createTypeGetter(), but that's not changing my point - it means that you can't "futureproof" an API to work with later descendants by making it a "union of one", or opt out of this behaviour via scaffolding. You can always go custom in this case, so it's not a big deal.

[unclecheese]

Funny, Sam and I were talking about this last week, because he was questioning the forced use of ... on, and I thought I had confirmed otherwise, but now I see that it is compulsory for all unions. It makes sense -- if you add a new type to the union with a common field, you want your client to not have to even know it exists, and without ...on your could introduce overfetching.

Anyway, looks minor enough to leave alone, but the alternative, of course, would be to enforce unions unconditionally. It's probably not that bad an idea. Otherwise, DataObjects with descendants become a special case, and I'd prefer to obfuscate that detail. I think consistency is more important here than brevity.

[sminnee]

Have you tried this in a GraphQL client?

Might be a good idea to have a functional test suite that actually uses a (PHP?) GraphQL client to make a bunch of test queries?