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

[FR] GraphQL: Customize (add/hide) available fields #4850

Open
wsydney76 opened this issue Aug 29, 2019 · 17 comments

Comments

@wsydney76
Copy link

commented Aug 29, 2019

This is a follow up of #4832

Currently the data available via GraphQL is de facto a 1:1 representation of raw db content.

There are some reasons to customize that behavior:

  • Hide fields (e.g. internal data like workflow or accounting data, or data that acts only as input for business logic)
  • Add data resulting from custom business logic
  • Add data from complex queries (like adding joins from elements to custom tables, or multi step queries)
  • Workaround for directives that will fail in static site generators? (see comments by @narration-sd )
  • Unsupported stuff (like incoming relations, if i don't miss something)
  • Performance issues (like simulating eager loading owners of matrix blocks)
  • Keeping the api stable in case of changes in your information model

An additional field could be registered with:

  • The schema(s) in which it is available
  • The type(s) it is available in
  • How it is resolved. Personally i find pointing to a Yii behavior method the most natural thing, but will be any other way (even a twig template?)
  • A schema description of the returned data (e.g array of type xxx_BlockType, nested json data). To be honest i have no idea of GraphQL internals, so i don't know what is possible here.
@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

[redone from the phone email result, so you can possibly read it. Do not email github from phones...]

  • This is a good and thoughtful list. It makes me feel better about how the
    original discussion was switched off.

  • A primary need is going to be controlling the behavior of (we could call
    them) these filtering fields.

  • I see no reason not to borrow the way CraftQL did it, introduce name: value
    argument lists inside parents on the field.

  • The arguments will end up I suspect being parsed outside of GraphQL proper, but look just like its syntax; thus comfortable, supporting users, which feels a rather substantial point.

  • Pass in the name of the transform for an image, and the most glaring
    problem I know is solved.

  • Propose that this will make a fine basis model for futures and accommodating all the
    interesting possibles you've raised.

  • It should work because it's syntax will pass through the Gridsome, Gatsby, etc.'s own GraphQL engine. I'm careful to say it, but should, as craftQl's did.

Thank you, gentlemen.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@wsydney76 @andris-sevcenko : I've spend an amount of time with this today, probing source code as it acts, and may have a nice way to approach it, staying within areas we can touch.

  • First, ok, keep it up with the directives. Because, we'll actually use them.
  • Provide, not an Event, actually, but a method, on Craft\services\Gql -- skeleton it as translateQueryArgs($query, $rules), for the moment.
  • In translateQueryArgs:
    • render the GraphQL AS tree early, using its own Parser::parse()
    • I've analyzed and code-proved the result of this can be fed to GraphQL::executeQuery() as the $source, which we'll do in this model instead of the straight-text value
    • then, parsed, we have an AST for the query, GraphQL::-style
    • we arrange our $rules to be useful for it, information to redefine nodes as required
    • we Visit the query AST, find Field nodes identified by element and field that match our $rules, and have arguments that the Schema thinks shouldn't be present, but that we recognize
    • for each relevant node found, we use our $rule content to re-arrange what the $Field node looks like, so that, wait for it, now it has a proper identified Directive form, argument names substituted per need, etc..
    • as mentioned, then, we use our full translateQueryrArgs() return, the modified AST, as the $source input to executeQuery()
    • result: Gql is happy because it received directives, and the underlying js package GraphQL is happy because it thought it only saw Field arguments, not directives which it didn't know about.

Points of design:

  • you have your directives,
  • and we can still use them all the places that Craft GraphQL isn't going to be in control
  • rules make it flexible and extensible to whatever directives want to be enthused
  • no Events, just one ordinary method call

Issues:

  • building the AST visitor, which should be made out of GraphQL parts preferably, so we would track
  • defining the rule form/s
  • proving the AST can be so-modified. I think so - it's keeping track of positions in the query text, so errors will refer back to the text people think they have, even though the substituted node/s won't internally match...
  • but we'll need to pre-exception or intercede when the error is in the translator's rule or match
  • I don't know about multiple directives cases at this point, or even if they're ever allowed in GraphQL, who we know would canonically prefer to avoid them entirely, but can't back-track. If so, another way of handling multiple Field arguments, I guess. Not interesting for now.

I suspect this would fit the considerable range Werner's excellently thought-out list opens to, and it does fit what I need to get one or more normal Craft primitives to work.

What do you think?

@brandonkelly -inclusive, of course, if he's a little busy right now.

@andris-sevcenko

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@narration-sd I mean... I'm a sucker for over-engineering myself, but I have a lot of issues with that plan.

The elegance of GraphQL is in the fact that you look at the schema and you know what you can query for.

  1. GraphQL should not ever return anything that is not defined in its schema. That means you won't be able to add new fields using this method because they wouldn't be reflected in the schema. Even if the GraphQL engine allowed it (pretty sure it wouldn't validate the query that does stuff that's not in the schema), you're just introducing some arbitrary hidden fields which doesn't sound like good practice at all.

  2. I feel that analyzing the query and then adding some rules runtime undermines the elegance of GraphQL and is not transparent at all.

  3. No part of the plan is simple. I mean, look at the issues you outlined yourself. None of that sounds simple to implement and maintain in the long run, especially if that code's job is to handle deviations and is most likely to encounter the wildest of scenarios.

Instead, it really, really, should just be an event.

Every data entity returned by GraphQL is going to be content. And we've yet to encounter where that is not an Element. So, knowing that Craft's implementation has a generator for each element type (https://github.com/craftcms/cms/tree/develop/src/gql/types/generators), it's trivial to implement an event firing before each GraphQL type for different pieces of content is created and added to the schema. At which point a plugin or a module can just remove unneeded fields or add their own fields, complete with a resolver function (https://webonyx.github.io/graphql-php/type-system/object-types/#field-configuration-options).

That would include less moving parts (a lot less), doesn't involve magically manipulating the query and ensures that schema always shows the available content.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Well, right on time as I was dropping off, but what the heck, and this time I fire up the laptop rather than try to uncoil Github's misbehavior.

Andris, I'm not quite sure I follow any of your slam about 'over-engineering'. The AST stuff is all there within webonyx's portion of your package, and in principle is simple. Rules to define what happens are also straightforward. Connecting the two so that directives with their attractions to provide functionality while serving the wider needs which can't directly call them would seem to be win-win. And slams -- that's pretty out of court, isn't it?

Now. In my brief read of your counter-proposal, no doubt humanly flawed, and skipping my own linguistic tendencies, what I don't see happening is solution to where I began. How is the query written so that it satisfies the js-package's no-custom-directives style going to specify what directive, and what the directive would do? Or if it's something other than a directive?

To whit, how do we pass the identity of a transform for an image out of that Gridsome etc. box and into also-yours Craft Asset's hands? I am probably really missing something.

Then, that you can introduce Events into your resolvers, ok. I actually began in thinking there. But to attract coding for them in addition to, duplicating directives you've now enticed just everybody to write? And again, quite possibly I mistake, but it sounds like you are talking about sprinkling these Events around in so many resolvers of types -- each Event being a code you will have to approve for internality, and then support.

Related, I was a while ago beginning to think towards how resolvers might be made externally available for other custom writing, and maybe this is what you are talking about. But again, that's kind of a support issue, would think, if the flexibility sounds attractive. Issues -- and alternatives -- seem to fill the imagination, so I'll stop with this line, just consider it needs a thoughtful balancing.

Back to go. Events to alter resolvers haven't shown me how to push modifiers on fields up through the local js-framework minimal-GraphQL stone wall. Perhaps it's in there; let's see.

But anyway. I put a lot of effort into coming up with something workable, so that this issue wouldn't get buried in the 'someday' bin as it was headed for by discussion being shut off. Backed off from whatever's appearing to tend towards sparks, I'd think you'd appreciate if not respect that, Andris.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Apologies for a couple of hopefully helpful after-edits....cheers.

And sure, I would like always for this to be more simple - no points missed there.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

And, I guess I should underline -- I'm not doing any of this, front-channel or back, to critique anyone else's thinking, design styles, etc.. I'm doing it because I have front-and-center need, that product that has waited to be fully practical. There are other issues as you know, but this is one of the fully technical ones that needs to get resolved.

And yes, also out of a tendency to want to help.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

One more thought occurs as my own keyboard cools :) and I cool by reading a photo site and seeing people with too much crabbing there as ever, the better to shake my head at self.

The thought is that you seem to be talking about adding fields etc. not in the Schema.

That's not what's happening. The Schema is always the Schema -- the only thing the query can operate on. It's the query that gets rearranged. Maybe code would be a lingua franca that helps.

Asset field specified with transform in CraftQL -- passes through js-package GraphQLs

image {
                title
                url(transform: cardImage)
            }

Asset field specified with transform in Craft GraphQL-- blocked by js-package GraphQLs

image {
                title
                url @transform(handle: "cardImage")
            }

Conversion explained would be to alter the first style so that the re-formed query, the thing your GraphQL execute sees, becomes the second. That's all.

The conversion is proposed via the AST is because a parse is necessary to discover the argument on the Field type, with a structured result able to modify it -- and webonyx provides that for us.

Another shake of the head at self: sure, I don't like to go to such an extent either, of course. And if there's a better way around it...very open.

But in any case, you probably have no idea of the wryness surrounding all of this project here anyway, none of it aimed your direction whatsoever...! Cheers, Andris, good thoughts -- Clive

@andris-sevcenko

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Clive,

Well. I had a grumpy morning, it seems, and I've come across harsher than I intended. Sorry about that - I'll try to encompass in a few sentences what I propose.

  1. I've dealt with AST stuff. Once Craft starts to resolve any query, it analyzes it (https://github.com/craftcms/cms/blob/develop/src/gql/base/Resolver.php#L85) recursively to construct all the eager-loading conditions for it. And I know I'd hate to re-visit that functionality in 6 months because it would take a while to absorb it all again and remember what it's doing. (Parsing named fragments and inline fragments don't make things easier.)

  2. I don't propose events in resolvers. What I am offering is events before an individual composite GraphQL type is created with the definition passed along with the event. Plugins/modules could then
    a) remove fields from definitions
    b) add new fields (with their own resolver functions) to the definition
    c) modify existing fields (as well as add a resolver function to them).

So, if you felt so inclined, you could modify the url field on an Asset type and make it have a transformHandle argument. You could add a resolver function to the same url field that looks for that argument and generates the transform. You could invoke whatever Yii behaviors you want or maybe even add your own layer of caching. And, best of all, the GraphQL Schema exposed to the client would show the new, updated state. And the query resolving itself would not get any more complicated, where-as pre-parsing the query would add complexity to it.

So, you could solve a Directive issue in two ways:
a) either by using a GraphQL source plugin that supports it (it's on our list, but don't have an ETA yet);
b) you could install a small plugin in Craft that would subtly tweak the schema in a way that is more appealing to Gatsby.

Everybody wins, and complexity isn't increased, no?

@wsydney76

This comment has been minimized.

Copy link
Author

commented Aug 30, 2019

Without being able to understand this discussion in detail, having an event beforehand sounds perfectly fine for me.

Wanted to add that it may be additionally possible to define schema fields in a config file, optionally including a twig template string as a resolver instead of a php function.

Just looked at real live element-api transformers, there is a lot of trivial stuff like
{{ image.license ? "Published under #{image.license}" : "Any reuse prohibited without exceptions" }} (For legal reasons you sometimes have to be very verbose even in api data, can get very expensive if you don't)

Would be great to be able to implement such easy things without writing php code.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Ok, going back to sleep for beauty (age hath its privileges), but up for just the moment to get both you guys' good thoughts, with maybe inspiration on how phone can do decent answer.

  • Andris: grumpinesses ok, we all can have have 'em.
  • I see now the crossed purposes and why you were going on about Schema mismatches: we were each focused on opposite ends of the stick.
  • same reason I didn't grok how you thought to use Events. You want to use them to change the Schema....
  • so ok, it makes sense. I see some issues, mostly practical, as you do for translation.
  • I'm fading, and need to allow it, so discuss later zzz. But yes, it can work. I may see a nice intersection...
  • Werner, vielen Dank. I woke up on that, and immediately think it's imaginative, and fine contribution. We older wolves can do much more than growl in the pack!

Ok, bis vraiment Morgen, to mix a few...

@wsydney76

This comment has been minimized.

Copy link
Author

commented Aug 30, 2019

FYI: Just played around a bit using a simple custom field type as a workaround for those trival requirements (the last thing before holidays, i'm off for the next 6 weeks. Really off.)

Just takes a twig template as a setting that is evaluated at runtime.

...
    public $template;
...
    public function normalizeValue($value, ElementInterface $element = null)
    {
        return Craft::$app->view->renderString($this->template, ['element' => $element] );
    }

(Maybe something like this already exists, but i didn't see it at first glance. Preparse plugin may also work if evaluation at runtime is not needed.)

Example settings:

{{ element.license ? "Licensed under #{element.license}" : "No license" }}

{{ element.featuredImage|one ? element.featuredImage|one.url('featuredImage') : siteInfo.defaultImage|one.url('featuredImage') }}
(Using the agnostic fetch plugin, as assets may or may not be eager loaded?)

{{ element.myBehavior }}

As 'normal' fields they perfectly show up in the schema, (checked in gridsome's playground) without any further effort. And they can be also used in twig and element-api.

Also they can be edited in the CP (ok, that may also be a disadvantage...)

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Gute Reise, Werner, and sounds a great time ahead; geniessen Sie :) oder, sie, vielleicht

Thanks for Twig example of what you're thinking - will have a close look while you're gone...appreciated.

Clive

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@andris-sevcenko I wanted to rest concerning this approach, so gave it some of my long-weekend-here time.

In fact, it's powerful, as you suggest, and very easy...once you figure out how much you don't need to do, which means digging around in the endless Type machine just as with the AST you mention, until the layering is understood.

And then figure out how this can be sensibly inserted...more of the same.

However, the coding result is indeed cleaner, once arrived, and in discovery prototype I have image transforms working nicely as field arguments via a globalizable function, more as easily to come, so that I can believe via concrete evidence. As we do for the real.

Here's a bit of how I may see the rest playing out.

  • as a universal place to put the Event, the GqlTypeTrait turned out attractive for various reasons having to do with your everything-static, and a complete system would just complete the function I added there to work in the usual way of triggering and reading back from an Event.

  • as tight as I could see to make things, you'd call this function as I do now on one, from each types\generators\ElementType::getFieldDefinitions(), wrapping the static return so this can modify it.

  • you'd pass the ElementType for the Event to let the handler know what Type we mean.

  • the Event handler merges in on each field for that Type it knows about, such things as args and arg-selective resolve(r) as I am already now.

  • the merge data for the handler would be able to be in a /config file if not hard-coded. This if so might want to just be php, as then a multilevel array with anonymous functions is straightforward.

Ok. It's a lot later, and many more hours in than I intended, but it's done as a preplay. You can see how you feel.

I suspect you'll reckon how expansion on this method could do a lot more, given that opening such would in some future be sensible. For now, looks like it will solve my popular-spas requirement...

Cheerio, and don't mind me -- it's late... 🐟

@andris-sevcenko

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

The list you just banged out, @narration-sd, seems to be pretty much what I was thinking :)

Now just to get around to implementing that.

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Ok, in some kind of exercise of solidarity or judgement(!?) it's done I think. I'll let it rest overnight, and PR it tomorrow.

I'm running it fully from an Event, within Live Vue, which only has to pass back a simple nested array of config changes for any fields per Element Type, as the code-inline doc illustrates.

Let's see if I can post that doc:

    /**
     * @event schemaDefFieldsEvent The event that is triggered when Gql Types have
     * completed defining their Craft-internally-defined fields.
     *
     * This event follows a usage pattern in Craft plugins etc. similar to others, but is
     * triggered through this proxy, SchemaDefFieldsEventProxy, so that its intended use
     * can be possible from the Gql Element interfaces, which are all defined by traits
     * and static functions, not methods of constructed objects.
     *
     * The job of the actual Event handler is simply to pass back a table, a nested array
     * which identifies any replacements or additions to properties for fields of Types.
     * A /config php file may in some cases be an attractive place to actually put this
     * config, as it can contain closures, as for the resolve(r) items when used.
     *
     * The `employ` method takes care of triggering the event, then appropriately merges
     * for targetted fields in the currently built Type, returning the upgraded field
     * definitions. It's used to complete each of the Element Interfaces for Gql inside
     * of Craft, just modifying the static lists in these before they're returned.
     * ---
     * ```php
     * use craft\gql\base\SchemaDefFieldsEventProxy;
     * use craft\events\GqlSchemaDefFieldsEvent;
     * use yii\base\Event;
     *
     * Event::on(SchemaDefFieldsEventProxy::class,
     *     SchemaDefFieldsEventProxy::EVENT_SCHEMA_DEF_FIELDS,
     *     function(GqlSchemaDefFieldsEvent $event) {
     *         $event->fieldsToChange = [
     *             'Asset' => [  // the Element Type Name (only)
     *                  'url' => [  // one of its fields
     *                      'args' => [
     *                           'transform' => Type::string()
     *                       ],
     *                       'resolve' => function ($source, $args, $context, $info) {
     *                           if (\array_key_exists('transform', $args)) {
     *                               return $source->getUrl($args['transform']);
     *                           } else { // other keys will get other resolution, after which this default
     *                               return $source->getUrl();
     *                           }
     *                       },
     *                   ]
     *               ]
     *          ];
     *     }
     * );
     * ```
     */
@narration-sd

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@andris-sevcenko @brandonkelly And, 'Monday' after 'holiday' brings the Pull Request up, now at #4879.

It's working on code here, and the principles are simple, after less-simple work to discover what those principles should be. I've added the ability for all GraphQL Interfaces in the PR, and I'm using some of them on my prototypes that you know about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.