-
Notifications
You must be signed in to change notification settings - Fork 61
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
FEATURE: DataObject scaffolding #20
Conversation
3f70b60
to
a7356db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, massive effort - looking great! Thanks for building in all the sanity checks around YAML configuration, and for creating an expressive system with PHP interfaces. Ran out of steam after 25 comments, will have a second pass tomorrow :)
README.md
Outdated
@@ -243,6 +243,305 @@ This will create a new member with an email address, | |||
which you can pass in as query variables: `{"Email": "[email protected]"}`. | |||
It'll return the new `ID` property of the created member. | |||
|
|||
## Scaffolding DataObjects into the Schema | |||
|
|||
Making a DataObject accessible through the GraphQL API involves quite a bit of boilerplate. In the above example, we can see that creating endpoints for a query and a mutation requires creating three new classes, along with an update to the configuration, and we haven't even dealt with data relations yet. For applications that require a lot of business logic and specific functionality, an architecture like this affords the developer a lot of control, but for developers who just want to make a given model accessible through GraphQL with some basic Create, Read, Update, and Delete operations, scaffolding them can save a lot of time and reduce the clutter in your project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep markdown line lengths to ~120 chars? Makes diffs easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you capitalise "Create" etc here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had just spent the previous eight hours defining ClassNames. ;-)
README.md
Outdated
|
||
**Code**: | ||
```php | ||
class Post extends DataObject implements ScaffoldingProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing namespace for Post
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was trying to keep things brief, as that was already shown, but yeah, probably better to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, but from memory it was inconsistent - some of the example classes had the MyProject
namespace
README.md
Outdated
$scaffolder->dataObject(Post::class) | ||
->addFields(['ID','Title','Content']) | ||
->query('readPosts', function() { | ||
return Post::get()->limit(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated once Will's pagination PR is merged in - might be follow-up work (not blocking this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
README.md
Outdated
- My\Project\Post | ||
``` | ||
|
||
### Scaffolding DataObjects through the Config layer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels a bit weird to describe the YAML-based confi here, without actually going into details ("scaffolding goes here"), then switching back to the provider-based docs. I'd prefer to start with YAML scaffolding, since that's what we'd assume is more frequently used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will reverse those two paragraphs.
README.md
Outdated
]) | ||
->setResolver(function($obj, $args) { | ||
$post = Post::get()->byID($args['ID']); | ||
$post->Title = $args['NewTitle']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs canEdit()
checks
|
||
foreach ($fields as $fieldName) { | ||
$result = $instance->obj($fieldName); | ||
if ($result instanceof DBField) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we deal with getters? Force $casting? Then we should throw an exception if the result is not an instance of DBField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we could try to be smart about it, seeing as there are only four GraphQL field types.
Have updated to:
foreach ($fields as $fieldName) {
$result = $instance->obj($fieldName);
if ($result instanceof DBField) {
$typeName = $result->config()->graphql_type;
$fieldMap[$fieldName] = (new TypeParser($typeName))->toArray();
} else if(is_bool($result)) {
$fieldMap[$fieldName] = ['type' => Type::boolean()];
} else if(is_int($result)) {
$fieldMap[$fieldName] = ['type' => Type::int()];
} else if(is_float($result)) {
$fieldMap[$fieldName] = ['type' => Type::float()];
} else {
$fieldMap[$fieldName] = ['type' => Type::string()];
}
}
); | ||
|
||
$this->setResolver(function ($object, array $args, $context, $info) { | ||
if (singleton($this->dataObjectName)->canDelete()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be called on the actual object, not the singleton
public function __construct($dataObjectName) | ||
{ | ||
$this->dataObjectName = $dataObjectName; | ||
$this->args = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this accept a List of IDs by default? It would help with the current case of asset admin batch deletes, and my gut feel is that it's going to be a common requirement in web apps
$obj = DataList::create($this->dataObjectName) | ||
->byID($args['ID']); | ||
|
||
if ($obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should throw an exception if the object can't be found
$ops[] = constant($constStr); | ||
} else { | ||
throw new \Exception( | ||
"Invalid operation: $op" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention the context here to make it debuggable (e.g. $dataObjectName)
4b2d6e4
to
5fb77fd
Compare
Codecov Report@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 87.27% 90.51% +3.23%
==========================================
Files 18 36 +18
Lines 566 1402 +836
==========================================
+ Hits 494 1269 +775
- Misses 72 133 +61
Continue to review full report at Codecov.
|
d05717b
to
ea08f67
Compare
} | ||
|
||
return $comments; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Daaaaaamn! That's much easier to digest!
* The entry point for a GraphQL scaffolding definition. Holds DataObject type definitions, | ||
* and their nested Mutation/Query definitions. | ||
*/ | ||
class GraphQLScaffolder implements ManagerMutatorInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is in the SilverStripe\GraphQL
namespace the class should be called Scaffolder
instead of GraphQLScaffolder
- it's redundant information. Given this will be frequently used to reference constants, a shorter class name is good news as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that, but there's also DataObjectScaffolder
, QueryScaffolder
, OperationScaffolder
, etc. I'm not sure I'm comfortable with it being Scaffolder
in that context, but I agree that GraphQL
is redundant.
SchemaScaffolder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah SchemaScaffolder
works for me.
tests/ScaffoldingTest.php
Outdated
namespace SilverStripe\GraphQL; | ||
|
||
use SilverStripe\Dev\SapphireTest; | ||
// use GraphQL\Type\Definition\Type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests need to be commented in again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were written before a massive change in architecture and API, so I commented them out just to get this to a reviewable state. I'm working on the new test suite right now.
README.md
Outdated
For these examples, we'll imagine we have the following model: | ||
|
||
```php | ||
namespace MyProject\GraphQL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ORM model classes shouldn't live in a MyProject\GraphQL
namespace - in and of themselves, they have nothing to do with GraphQL, and it'll tend to confuse devs (by wrongly assuming there's something special about these models). I'd suggest putting them in a MyProject
only (rather than a more verbose MyProject\Model\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, have changed the namespace to MyProject
for everything.
README.md
Outdated
|
||
**Via YAML**: | ||
``` | ||
SilverStripe\GraphQL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs yaml
markdown syntax marking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
README.md
Outdated
fields: [ID, Title, Content] | ||
operations: | ||
read: true | ||
create: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't have update: true
, yet has an updatePost()
query example below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have changed the example to CreatePost
instead
Content | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point you probably should describe how the readPosts
naming was generated (automatically inferred from singular_name and plural_name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added an explanation
README.md
Outdated
operations: | ||
read: | ||
args: | ||
StartingWith: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StartingWith
is a bit of a confusing argument example, particularly as a slight variation from the StartsWith
ORM filter. Is there a reason you didn't just use Title
as the arg name and perform a partial match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, have changed to PartialMatch
.
README.md
Outdated
'StartingWith' => 'String' | ||
]) | ||
->setResolver(function($obj, $args) { | ||
$list = Post::get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably shouldn't have any examples without canView()
checks, I'd add at least a PHP comment like // ... canView checks
. It's too easy for devs to assume that scaffolded resolvers will magically take care of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a simple canView($context['currentMember'])
check.
return $this; | ||
} | ||
|
||
if ($config['operations'] === '*') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README mentions all
, but the code only accepts *
.
README.md
Outdated
operations: [CREATE, READ, UPDATE, DELETE] | ||
``` | ||
|
||
> As shorthand, the expression `[CREATE, READ, UPDATE, DELETE]` can also be expressed as `operations: all`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is: Why isn't there a GraphQLScaffolder::ALL
constant as well? Constants are a way to communicate type strictness and avoid misspelling. Also, the README mentions all
, but the code only accepts *
. Can we just make this GraphQLScaffolder::ALL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another slew of nitpicks! ;)
As a general note, all GraphQL fields should start lowercase, to distinguish them from types starting with uppercase. This clashes with our SilverStripe conventions (DataObject properties start with uppercase). I've written https://github.com/silverstripe/silverstripe-graphql/blob/master/src/Util/CaseInsensitiveFieldAccessor.php for this purpose. Doesn't necessarily need to be solved in this PR, but we need to address it soon (for query args and query/type fields) |
if (is_subclass_of($resolver, ResolverInterface::class)) { | ||
$this->resolver = Injector::inst()->create($resolver); | ||
} else { | ||
var_dump($resolver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug code
src/Scaffolding/Util/TypeParser.php
Outdated
*/ | ||
public function __construct($rawArg) | ||
{ | ||
if (!preg_match('/^([A-Za-z]+)(!?)(\s*=\s*(.*))?/', $rawArg, $matches)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protip: You can use a non-capturing group in regexes to avoid leaving out $matches[3]
here: ^([A-Za-z]+)(!?)(?:\s*=\s*(.*))?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I had been wondering if that was possible. Thanks.
|
||
// Todo: this is totally half baked | ||
$this->setResolver(function ($object, array $args, $context, $info) { | ||
if (singleton($this->dataObjectClass)->canCreate()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All can*()
checks should pass in $context['currentMember']
- an attempt to decrease the dependence of globals throughout the framework (Session dependency in DataObject::can*()
). At the moment it's just future proofing, but it's pretty much always clearer to pass in context explicitly
{ | ||
public function resolve($object, $args, $context, $info) | ||
{ | ||
$post = Post::get()->byID($args['ID']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs canEdit()
check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the CRUD operations now have permission checks.
README.md
Outdated
->setResolver(MyResolver::class) | ||
->end(); | ||
``` | ||
Or... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just add the second variation as an inline comment on the first code example, for brevity's sake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
README.md
Outdated
``` | ||
|
||
``` | ||
mutation UpdatePost($ID: ID!, $Input: PostUpdateInputType!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention somewhere around here that permission constraints are enforced by the built-in resolvers
README.md
Outdated
|
||
**GraphQL** | ||
``` | ||
mutation updatePostTitle(ID: 123, NewTitle: 'Foo') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing $
in args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
} | ||
|
||
mutation createRedirectorPage { | ||
RedirectorPageCreateInputType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you missing those input types in your type
list above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Have added them.
parent::__construct($operationName, $this->typeName()); | ||
|
||
$this->setResolver(function ($object, array $args, $context, $info) { | ||
$list = DataList::create($this->dataObjectClass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this filter by canView()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but you have to use filterByCallback
if you want to be thorough. Otherwise, a simple check against a singleton could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This callback would need to comply with the OperationResolver
interface right? $info
would need a ResolveInfo
type hint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm... If you passed it a named class, or an instance, it has to implement OperationResolver
, but AFAIK, there's no way to enforce an argument signature on a closure.
08a9573
to
db22c0d
Compare
src/Manager.php
Outdated
use GraphQL\Type\Definition\ObjectType; | ||
use GraphQL\Error; | ||
use GraphQL\Type\Definition\Type; | ||
use SilverStripe\Security\Member; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Aaron - you'll need this back in again, it's used for the member methods at the bottom - I think this is why your tests are failing ATM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have replaced this.
40ea1f9
to
e4be8ff
Compare
e4be8ff
to
406b770
Compare
406b770
to
644644f
Compare
b987bc0
to
baf8b93
Compare
baf8b93
to
d8af239
Compare
Some weird rebasing going on here. :P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed this from a code-style and ORM usage perspective. I'm going to assume that @chillu has / will covered the functional review and is happy with the graphql API usage.
examples/_config/config.yml
Outdated
@@ -12,3 +12,46 @@ SilverStripe\GraphQL: | |||
authenticators: | |||
- class: SilverStripe\GraphQL\Auth\BasicAuthAuthenticator | |||
priority: 10 | |||
scaffolding_providers: | |||
- My\Project\Post |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the significance of My\Project\Post
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the examples/
directory, so this is just an example of how you would set up your config with registered scaffolding providers.
src/FieldCreator.php
Outdated
@@ -82,7 +82,7 @@ public function getAttributes() | |||
$attributes['type'] = $type; | |||
} | |||
|
|||
$resolver = $this->getResolver(); | |||
$resolver = $this->getResolver(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: We need to run phpcbf
over this project to fix all extraneous whitespaces and formatting errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have done.
src/GraphiQLController.php
Outdated
/** | ||
* @var string | ||
*/ | ||
protected $template = 'GraphiQL'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From 4.0 onwards best practice is to namespace templates, please. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better, just make the template name GrahpiQLController.ss
, put it in the correct folder (templates\SilverStripe\GraphQL
), and you don't need to declare the config at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this feature was supposed to be deleted in this 4e3a325 but at some point a rebase got stuffed.
src/InterfaceTypeCreator.php
Outdated
@@ -2,28 +2,18 @@ | |||
|
|||
namespace SilverStripe\GraphQL; | |||
|
|||
use GraphQL\Type\Definition\InterfaceType; | |||
use GraphQL\Type\Definition\InterfaceType as BaseInterfaceType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alias isn't necessary here.
src/Manager.php
Outdated
* Execute an arbitrary operation (mutation / query) on this schema | ||
* @param string $query | ||
* @param array $params | ||
* @param null $schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid phpdoc; Should be at least one non-null arg type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, phpdoc description getting deleted.
/** | ||
* OperationScaffolder constructor. | ||
* | ||
* @param null $operationName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a non-null param type descriptor (and elsewhere).
} | ||
foreach($config['args'] as $argName => $argData) { | ||
if(is_array($argData)) { | ||
if(!isset($argData['type'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is a bit weird.
* @param string $class | ||
* @param null $resolver | ||
* | ||
* @return bool|QueryScaffolder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to return any bool does it?
*/ | ||
public static function typeNameForDataObject($class) | ||
{ | ||
$typeName = Config::inst()->get($class, 'table_name', Config::UNINHERITED) ?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. :)
templates/GraphiQL.ss
Outdated
@@ -0,0 +1,9 @@ | |||
<!DOCTYPE html> | |||
<html lang="en"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed earlier, please move to namespaced folder matching the controller FQN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't even be here. It's an artefact of the bad rebasing you were referring to.
0183b6d
to
62cdf22
Compare
Required to make scaffolding work cleanly. See #20 (comment)
Constructor args on DataObject don’t call setters
->end() | ||
->type('Page') | ||
->addFields(['BackwardsTitle']) | ||
->end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have this indented to retain some sanity ;)
Woohoo! Great to see this merged |
…der-tweaks Update code for upgrader
…ocus-transform Add an autoFocus property to the Form
Per #9