(MOVED) NEW: Replace _extends in favour of Unions/Interfaces

[unclecheese]

Overview

This pull request does away with the idiosyncratic and verbose _extends field we used to navigate the inheritance pattern of ORM-generated types and instead leverage some of the native abstractions provided by the GraphQL spec that are better suited for this purpose.

Relevant: #209

Docs: silverstripe/silverstripe-framework#9912

To be merged after

* [ ] silverstripe/silverstripe-asset-admin#1201
* [ ] silverstripe/silverstripe-versioned#331

• DOCS: new union and interface inheritance pattern silverstripe-framework#9912

Tests:

• Unit tests passing
• Doesn't break elemental
• Doesn't break asset admin

There are several components to this revision.

Part 1: Inherited fields / implicit exposures

• Exposing a type implicitly exposes all of its ancestors.
• Ancestors receive any fields exposed by their descendants, if applicable.
• Exposing a type applies all of its fields to descendants only if they are explicitly exposed also.

Serviced by: SilverStripe\GraphQL\Schema\DataObject\InheritanceBuilder

Example:

BlogPage:
  fields: 
    title: true
    content: true
    date: true
GalleryPage:
  fields:
    images: true
    urlSegment: true

This results in these two types being exposed with the fields as shown, but also results in a Page type:

type Page {
  id: ID! # always exposed
  title: String
  content: String
  urlSegment: String
}

Part 2: Interfaces

Any type that's part of an inheritance chain will generate interfaces. Each applicable ancestral interface is added to the type. Like the type inheritance pattern shown above, interfaces duplicate fields from their ancestors as well.

Additionally, a base interface is provided for all types containing common fields across the entire DataObject schema.

Serviced by: SilverStripe\GraphQL\Schema\DataObject\InterfaceBuilder

Example

Page:
  BlogPage extends Page
  EventsPage extends Page
    ConferencePage extends EventsPage
    WebinarPage extends EventsPage

This will create the following interfaces:

interface PageInterface {
  title: String
  content: String
}

interface BlogPageInterface {
  id: ID!
  title: String
  content: String
  date: String
}

interface EventsPageInterface {
  id: ID!
  title: String
  content: String
  numberOfTickets: Int
}

interface ConferencePageInterface {
  id: ID!
  title: String
  content: String
  numberOfTickets: Int
  venueAddress: String
}

interface WebinarPageInterface {
  id: ID!
  title: String
  content: String
  numberOfTickets: Int
  zoomLink: String
}

Types then get these interfaces applied, like so:

type Page implements PageInterface {}
type BlogPage implements BlogPageInterface & PageInterface {}
type EventsPage implements EventsPageInterface & PageInterface {} 
type ConferencePage implements ConferencePageInterface & EventsPageInterface & PageInterface {} 
type WebinarPage implements WebinarPageInterface & EventsPageInterface & PageInterface {} 

Lastly, for good measure, we create a DataObjectInterface that applies to everything.

interface DataObjectInterface {
  id: ID!
  # Any other fields you've explicitly exposed in config.modelConfig.DataObject.base_fields
}

type Page implements PageInterface & DataObjectInterface {}

Part 3: Unions and Union Queries

Models that have descendants will create unions that include themselves and all of their descendants. For queries that return those models, a union is put in its place.

Serviced by: SilverStripe\GraphQL\Schema\DataObject\InheritanceUnionBuilder

Example

type Page implements PageInterface {}
type BlogPage implements BlogPageInterface & PageInterface {}
type EventsPage implements EventsPageInterface & PageInterface {} 
type ConferencePage implements ConferencePageInterface & EventsPageInterface & PageInterface {} 
type WebinarPage implements WebinarPageInterface & EventsPageInterface & PageInterface {} 

Creates the following unions:

union PageInheritanceUnion = Page | BlogPage | EventsPage | ConferencePage | WebinarPage
union EventsPageInheritanceUnion = EventsPage | ConferencePage | WebinarPage

"Leaf" models like BlogPage, ConferencePage, and WebinarPage that have no exposed descendants will not create unions, as they are functionally useless.

This means that queries for readPages and readEventsPages will now return unions.

readPages {
  nodes {
    ... on BlogPage {
      date
    }
    ... on WebinarPage {
      zoomLink
    }
  }
}

This works fine for leaf models, but what if we want the title field? One option is to add it to each ... on clause, as we know its been propagated down through the inheritance builder in part 1, above. But a better way to do this is to leverage the common interface.

query {
  readPages {
    nodes {
      ... on PageInterface {
        id # in theory, this common field could be done on DataObjectInterface, but that gets a bit verbose
        title
        content
      }
      ... on EventsPageInterface {
        numberOfTickets
      }
      ... on BlogPage {
        date
      }
      ... on WebinarPage {
        zoomLink
      }
    }
  }
}

A good way of negotiating whether to use interfaces or types in the ... on block is to ask the question "Could this field appear on more than one type?" If the answer is yes, you want an interface.

Elemental

One of the primary motivators for this refactor was the awkwardness of querying for content blocks. Almost by definition, content blocks are always abstractions. You're never going to query for a BaseElement type specifically. You're always asking for an assortment of its descendants, which adds a lot of polymorphism to the query.

query {
  readElementalPages {
    nodes {
      elementalArea {
        elements {
          nodes {
            ... on BaseElementInterface {
              title
              id
            }
            ... on ContentBlock {
              html
            }
            ... on CTABlock {
              link
              linkText
            }
          }
        }
      }
    }
  }
}

To consider

This has the undesirable effect of breaking GraphQL APIs when a subclass is added. We've always thought of this as having dangerous consequences for the use of GraphQL in the CMS, but now that we're using a build step to generate the schema, it's conceivable that we could mitigate that problem at build time with static query generation for CMS consumption.

For users consuming GraphQL in their own app code, this does indeed break their APIs, and we should make sure they're aware of it. There are an increasing number of these "heads up" cases that we want to include in the build script, and I'm proposing a SchemaAuditInterface API, where you could add your own set of auditors that would compare the previous schema to the next one and throw warnings or fatals when something isn't right.

A simple auditor that checks if any query has changed its return type would make a great auditor to start with.

Also in this PR

• className allowed by default for DataObject types
• New base_fields setting for fields that are required to be exposed on each model
• SchemaContextProvider is now more appropriately named SchemaConfigProvider
• Allow query to be pre-parsed in QueryHandlerInterface
• Fire events for graphqlQuery, graphqlMutation
• New QueryStateProvider for exposing shared query state to resolvers
• JSONBlob is now natively supported (bespoke scalar for elemental now deprecated)

[bummzack]

I don't have large scale public APIs running, but the potential side-effects of breaking the API seem like a rather big downside to this approach.

Not sure if I understand correctly, but is it mainly a problem when you add a Subclass to a class that was a "leaf-model" beforehand?

How will the audit system work? Does the developer add queries that are important to the API? Or is this also auto-generated? This might also generate a lot of warnings, whenever you install a module that exposes models via GraphQL, or am I missing something here?

[unclecheese]

Yeah, this has always been at the core of the struggle with how to handle inheritance. Unions are the right thing to do, but the determination to use one or not is driven by circumstances that are only tangentially unrelated to the schema, which creates some instability in your API.

The early results from our experiment with "pseudo-unions" to mitigate this problem haven't been promising. It's very confusing and cumbersome, and it's just a bad look for Silverstripe to have a content API that doesn't adhere to basic conventions.

The question is really over what cost we're willing to pay for the guarantee of not breaking the API when you add a subclass. While not rare, it's still pretty edge-casey, and I reckon we can gain a lot of ground through good documentation, and through notices in the build.

My idea of an auditor was to simply invoke something like audit(Schema $previous, Schema $next): void on one or many implementations. A standard one could be:

• Get all top-level and nested queries for previous
• Get all top-level and nested queries for next
• Compare return types
• Notice on mismatch "This change may break your queries"

[blueo]

Whats the nature of the breakage you get when adding a subclass? do queries break due to names changing on unions/interfaces? I'm just wondering if it could be mitigated by producing a fragment that you could the import via the #import "../../MyGeneratedFragment.graphql" syntax

[unclecheese]

Yeah, so when a query changes from a list of a single concrete type to a union, every field needs to be in an ...on clause. We made the mistake early on in 3.x of thinking fields that were common to everything in the union would be okay, but that's not actually valid. You need to explicitly assign everything to an ...on when you're in a union.

query {
  readPages {
    title
    content
    ... on BlogPage {
      date
    }
  }
}

We assumed that would work (after all, title and content are on all types in the union) and would therefore allow your queries to be backward compatible. But unfortunately, that's not he case.

query {
  readPages {
    ... on PageInterface {
      title
      content
    }
    ... on BlogPage {
      date
    }
  }
}

Therefore, adding a subclass to an exposed type, and exposing that subclass, will break things.

Notably, this upgrade does not break elemental. You would think readElementsForArea would be broken now because it currently returns [BaseElement], which seemingly would become a union of all the blocks. But as long as none of those subclasses are exposed to the admin schema, it's still going to use the single type list.

But if you do this:

... Schema:
  schemas:
    admin:
      models:
        My\Block:
          fields: '*'

You just broke the CMS.

[blueo]

possibly bad idea - default every dataobject in the CMS/admin schema to a union from the get go - so no single types to break when they change/are subclassed.

I feel this as less of a problem in front-facing schemas as code that consumes those is under the control of the one adding the config.

[unclecheese]

Well here we go again. tl;dr, we probably don't need the unions in this PR, and all the chatter above regarding breaking APIs is probably not a thing.

#209 (comment)

[unclecheese]

We're using this branch so much now (e.g. silverstripe/headless module) that it's a pain to keep it on a fork, so I've moved the branch to the main repo and have issued a new pull request using that.

#393