From ef563eb39cadea19d22454c80d9b6db471950e6a Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 30 Jan 2019 20:56:52 +1300 Subject: [PATCH 1/2] API Don't create "peripheral" operations by default 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. --- README.md | 95 +++++++++++++------ .../Scaffolders/DataObjectScaffolder.php | 4 + .../Scaffolders/OperationScaffolder.php | 30 ++++++ .../Scaffolders/SchemaScaffolder.php | 2 + .../Scaffolders/DataObjectScaffolderTest.php | 17 +++- .../Scaffolders/SchemaScaffolderTest.php | 4 + 6 files changed, 124 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 1f94e0ed5..52c8b8451 100644 --- a/README.md +++ b/README.md @@ -1501,11 +1501,11 @@ SilverStripe\GraphQL\Manager: ##... SilverStripe\CMS\Model\RedirectorPage: fields: [ID, ExternalURL, Content] + Page: + fields: [MyCustomField] operations: read: true create: true - Page: - fields: [MyCustomField] ``` **... Or with code**: @@ -1513,15 +1513,15 @@ SilverStripe\GraphQL\Manager: $scaffolder ->type('SilverStripe\CMS\Model\RedirectorPage') ->addFields(['ID', 'ExternalURL', 'Content']) + ->end() + ->type('Page') + ->addFields(['MyCustomField']) ->operation(SchemaScaffolder::READ) ->setName('readRedirectors') ->end() ->operation(SchemaScaffolder::CREATE) ->setName('createRedirector') ->end() - ->end() - ->type('Page') - ->addFields(['MyCustomField']) ->end(); ``` @@ -1554,44 +1554,84 @@ type SiteTreeWithDescendants { SiteTree | Page | RedirectorPage } -input RedirectorPageCreateInputType { - ExternalURL: String - RedirectionType: String - MyCustomField: String - Content: String - # all other fields from RedirectorPage, Page and SiteTree -} - input PageCreateInputType { MyCustomField: String Content: String # all other fields from Page and SiteTree } -input SiteTreeCreateInputType { - # all fields from SiteTree +query readPages { + PageWithDescendants } -query readRedirectors { - RedirectorPage +mutation createPage { + PageCreateInputType } +``` -query readPages { - PageWithDescendants +By default, types are created for the full inheritance tree. +In certain situations, you may also want to create operations for all these ancestors and descendants. +For example, `createPage` only allows you to create an object of type `Page`. +In order to define a `createRedirectorPage` equivalent to create an object of type `RedirectorPage`, +you need to opt in via the `cloneable` configuration setting: + + +**Via YAML**: +```yaml +SilverStripe\GraphQL\Manager: + schemas: + default: + scaffolding: + types: + MyProject\Post: + ##... + SilverStripe\CMS\Model\RedirectorPage: + fields: [ID, ExternalURL, Content] + Page: + fields: [MyCustomField] + operations: + read: true + create: + cloneable: true +``` + +**... Or with code**: +```php +$scaffolder + ->type('SilverStripe\CMS\Model\RedirectorPage') + ->addFields(['ID', 'ExternalURL', 'Content']) + ->end() + ->type('Page') + ->addFields(['MyCustomField']) + ->operation(SchemaScaffolder::READ) + ->setName('readRedirectors') + ->setCloneable(true) + ->end() + ->operation(SchemaScaffolder::CREATE) + ->setName('createRedirector') + ->end() + ->end(); +``` + +This will add the following fields to your schema: + +```graphql +input RedirectorPageCreateInputType { + ExternalURL: String + RedirectionType: String + MyCustomField: String + Content: String + # all other fields from RedirectorPage, Page and SiteTree } -query readSiteTrees { - SiteTreeWithDescendants +input SiteTreeCreateInputType { + # all fields from SiteTree } mutation createRedirector { RedirectorPageCreateInputType } -mutation createPage { - PageCreateInputType -} - mutation createSiteTree { SiteTreeCreateInputType } @@ -1604,8 +1644,8 @@ when `RedirectorPage` is also exposed), a union is returned, and you will need t with the `...on {type}` GraphQL syntax. ```graphql -query readSiteTrees { - readSiteTrees { +query readPages { + readPages { edges { node { __typename @@ -1613,6 +1653,7 @@ query readSiteTrees { ID } ...on RedirectorPage { + ID RedirectionType } } diff --git a/src/Scaffolding/Scaffolders/DataObjectScaffolder.php b/src/Scaffolding/Scaffolders/DataObjectScaffolder.php index 51999556e..b9dc52e17 100644 --- a/src/Scaffolding/Scaffolders/DataObjectScaffolder.php +++ b/src/Scaffolding/Scaffolders/DataObjectScaffolder.php @@ -412,6 +412,10 @@ public function cloneTo(DataObjectScaffolder $target) } } foreach ($this->getOperations() as $op) { + if (!$op->getCloneable()) { + continue; + } + $identifier = OperationScaffolder::getIdentifier($op); $target->operation($identifier); } diff --git a/src/Scaffolding/Scaffolders/OperationScaffolder.php b/src/Scaffolding/Scaffolders/OperationScaffolder.php index e304f2b15..b21ca92ff 100644 --- a/src/Scaffolding/Scaffolders/OperationScaffolder.php +++ b/src/Scaffolding/Scaffolders/OperationScaffolder.php @@ -48,6 +48,30 @@ abstract class OperationScaffolder implements ConfigurationApplier */ protected $args = []; + /** + * @var bool Indicates that this operation should be + * included in clones to other types. + */ + protected $cloneable = false; + + /** + * @return bool + */ + public function getCloneable() + { + return $this->cloneable; + } + + /** + * @param bool $cloneable + */ + public function setCloneable($cloneable) + { + $this->cloneable = $cloneable; + + return $this; + } + /** * @param string $name * @return string|null @@ -399,13 +423,19 @@ public function applyConfig(array $config) } } } + if (isset($config['resolver'])) { $this->setResolver($config['resolver']); } + if (isset($config['name'])) { $this->setName($config['name']); } + if (isset($config['cloneable'])) { + $this->setCloneable((bool)$config['cloneable']); + } + return $this; } diff --git a/src/Scaffolding/Scaffolders/SchemaScaffolder.php b/src/Scaffolding/Scaffolders/SchemaScaffolder.php index 89e6fe358..ef2ea79d6 100644 --- a/src/Scaffolding/Scaffolders/SchemaScaffolder.php +++ b/src/Scaffolding/Scaffolders/SchemaScaffolder.php @@ -60,6 +60,7 @@ class SchemaScaffolder implements ManagerMutatorInterface */ public static function createFromConfig($config) { + /** @var self $scaffolder */ $scaffolder = Injector::inst()->get(self::class); if (isset($config['types'])) { if (!ArrayLib::is_associative($config['types'])) { @@ -284,6 +285,7 @@ public function getMutations() public function addToManager(Manager $manager) { $this->registerFixedTypes($manager); + $this->registerPeripheralTypes($manager); $this->extend('onBeforeAddToManager', $manager); diff --git a/tests/Scaffolding/Scaffolders/DataObjectScaffolderTest.php b/tests/Scaffolding/Scaffolders/DataObjectScaffolderTest.php index 6e9af401a..ce9faed4d 100644 --- a/tests/Scaffolding/Scaffolders/DataObjectScaffolderTest.php +++ b/tests/Scaffolding/Scaffolders/DataObjectScaffolderTest.php @@ -480,7 +480,22 @@ public function testCloneTo() $this->assertContains('Title', $target->getFields()->column('Name')); $this->assertNotContains('RedirectionType', $target->getFields()->column('Name')); - $this->assertCount(2, $target->getOperations()); + $this->assertCount(0, $target->getOperations(), 'Does not clone operations by default'); + } + + public function testCloneToWithCloneable() + { + $scaffolder = new DataObjectScaffolder(FakeSiteTree::class); + $scaffolder->addFields(['Title', 'ID']); + $scaffolder->operation(SchemaScaffolder::READ); + $scaffolder->operation(SchemaScaffolder::UPDATE)->setCloneable(true); + + $target = new DataObjectScaffolder(FakeRedirectorPage::class); + $target = $scaffolder->cloneTo($target); + $this->assertEquals( + ['updateSilverStripeFakeRedirectorPage'], + $target->getOperations()->column('getName') + ); } /** diff --git a/tests/Scaffolding/Scaffolders/SchemaScaffolderTest.php b/tests/Scaffolding/Scaffolders/SchemaScaffolderTest.php index 27bbd5941..1ae415893 100644 --- a/tests/Scaffolding/Scaffolders/SchemaScaffolderTest.php +++ b/tests/Scaffolding/Scaffolders/SchemaScaffolderTest.php @@ -86,8 +86,10 @@ public function testSchemaScaffolderAddToManager() ->type(FakeRedirectorPage::class) ->addFields(['Created', 'TestPageField', 'RedirectionType']) ->operation(SchemaScaffolder::CREATE) + ->setCloneable(true) ->end() ->operation(SchemaScaffolder::READ) + ->setCloneable(true) ->end() ->end() ->type(DataObjectFake::class) @@ -284,9 +286,11 @@ public function testUnionInheritanceForTypes() ->addFields(['Title', 'RedirectionType']) ->operation(SchemaScaffolder::READ) ->setName('READ') + ->setCloneable(true) ->end() ->operation(SchemaScaffolder::DELETE) ->setName('DELETE') + ->setCloneable(true) ->end() ->end(); $scaffolder->addToManager($manager = new Manager()); From a6463c5e6edf769631fd87518bda0c906ea05ec7 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 15 Apr 2019 15:21:05 +1200 Subject: [PATCH 2/2] Update docblock Co-Authored-By: unclecheese --- src/Scaffolding/Scaffolders/OperationScaffolder.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Scaffolding/Scaffolders/OperationScaffolder.php b/src/Scaffolding/Scaffolders/OperationScaffolder.php index b21ca92ff..e1f342897 100644 --- a/src/Scaffolding/Scaffolders/OperationScaffolder.php +++ b/src/Scaffolding/Scaffolders/OperationScaffolder.php @@ -64,6 +64,7 @@ public function getCloneable() /** * @param bool $cloneable + * @return $this */ public function setCloneable($cloneable) {