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

API Don't create "peripheral" operations by default #206

Conversation

chillu
Copy link
Member

@chillu chillu commented Jan 30, 2019

When scaffolding Page with a read operation,
the default behaviour used to be the creation of readPage, readSiteTree, readRedirectorPage etc.
In fact, the readPage query is sufficient, since it returns a union of all these types. The rest is noise,
with the slight advantage that types at the end of an inheritance chain (without descendants)
get away without unions in the return type.

The default behaviour is a bit more useful on create and update operations,
since those require a specific input type - unions aren't allowed here.
So in order to fully create subclasses, you need createPage vs. createRedirectorPage.

I'm proposing that we make this well-documented opt-in behaviour,
with the admin/graphql schema opting in for each type on create and update.
I don't see a reason why anyone would want to opt in on read or delete.

For the more common case where scaffolding is used to create a more limited readonly public schema,
this reduces the mental overhead for devs, noise in the schema itself,
and cuts down schema generation time because less "helper types" like readRedirectorPageConnectionType are required.

If this PR is accepted, we'll need to add the following to the
upgrading guide at https://github.com/silverstripe/silverstripe-graphql/releases/edit/untagged-b4d095bafd3edac22f15


Previously, scaffolded operations on DataObject would
create the equivalent operations on both ancestors and descendants.
If you want readPage, you'd also get readSiteTree and readRedirectorPage.
That's often not desired, so we're making it opt-in via a new cloneable config setting.
Note that it's most common to clone create and update operations,
since those rely on specific input types for a class
(e.g. createPage has createPageInputType, while createRedirectorPage has createRedirectorPageInputType).

SilverStripe\GraphQL\Manager:
  schemas:
    default:
      scaffolding:
        types:
          Page:
            operations:
              read:
              	cloneable: true

When scaffolding Page with a read operation,
the default behaviour used to be the creation of readPage, readSiteTree, readRedirectorPage etc.
In fact, the readPage query is sufficient, since it returns a union of all these types. The rest is noise,
with the slight advantage that types at the end of an inheritance chain (without descendants)
get away without unions in the return type.

The default behaviour is a bit more useful on create and update operations,
since those require a specific input type - unions aren't allowed here.
So in order to fully create subclasses, you need createPage vs. createRedirectorPage.

I'm proposing that we make this well-documented opt-in behaviour,
with the admin/graphql schema opting in for each type on create and update.
I don't see a reason why anyone would want to opt in on read or delete.

For the more common case where scaffolding is used to create a more limited readonly public schema,
this reduces the mental overhead for devs, noise in the schema itself,
and cuts down schema generation time because less "helper types" like readRedirectorPageConnectionType are required.
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good idea to me. I wonder about the naming of "cloneable" though - we're indicating that children will also be scaffolded, but "clone" doesn't really indicate that to me. What do you think about something like "scaffoldChildren," "includeChildren," or "recursive"?

Co-Authored-By: unclecheese <unclecheese@leftandmain.com>
@unclecheese unclecheese merged commit 4084355 into silverstripe:master Apr 15, 2019
@unclecheese unclecheese deleted the pulls/master/opt-in-cloneable-ops branch April 15, 2019 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants