[4.x] Allow altering of schemas

akhomy commented 2 years ago

When there is a parent contrib module/profile and you need to adjust the schema data there is no way to do it in the child schema file, meanwhile, in the backend, you can change the field structure according to your needs. Checking the https://github.com/webonyx/graphql-php/issues/451 and related topics it was resolved in some GraphQL JS libraries by using imports and directives, still, imports are not a part of the GraphQL spec https://github.com/graphql/graphql-spec/issues/343.

Valid examples:

Change the field in the GraphQL from required to non-required.
Change the field type in the GraphQL to your own implementation.

After investigating the Drupal GraphQL module I came up with the idea to introduce two events after collecting schema data. It will allow adjusting data before parsing and here you can rely on the schema keys in order to know which part to adjust. E.g.:

protected function getSchemaDocument(array $extensions = []) {
    ...
    $schema = array_merge([$this->getSchemaDefinition()], $extensions);
    // @todo Introduce AlterSchemaDocumentEvent

and

protected function getExtensionDocument(array $extensions = []) {
    ...
    $extensionSchema = implode("\n\n", $extensions);
    // @todo introduce AlterExtensionDocumentEvent

I'll try to implement this in the related PR.

akhomy commented 2 years ago

Here is PR with possible implementation - https://github.com/drupal-graphql/graphql/pull/1302

Kingdutch commented 2 years ago

Schema's in the GraphQL module are already composable and extendable. For this the module uses the concept of a base Schema and schema Extensions.

Could you outline what you were trying to achieve that could not be done with this system?

For an example of a module providing a "schema plugin" take a look at graphql_oauth. As outlined in the README a user will still need to add this extension to their own schema to enable it. However, from a maintainability perspective I would say this is desired since it means the end-developer keeps control over what modules can affect their GrahpQL schema to avoid unexpected breaking changes.

akhomy commented 2 years ago

Okay, let me explain the situation. Yes, we can achieve a similar approach by overriding in our own class getSchemaDocument, getExtensionDocument functions or even by using them and altering the AST tree elements. Still, you need to implement overrides for this, why not provide it out of the box?

Here is a practical case:

I have a parent module, which I cannot modify, and it provides a schema file:
```
type Example {
name: String!
}
```
It provides Drupal field - field_name.
You can change this field in Drupal from required to non-required.
You cannot change the schema. It is a parent module to which you don't have access.
You need to adjust your API in order to reflect the configuration changes. This is a perfect case where alterations could be used and, in your child module, you can modify data. Also, we can disable the parent module schema file and use our own, and it is much easier to use the above events. And then it is up to you what to do - use in your child module schema file, use alter event and replace some parts of it. It is totally the responsibility of the developer, but as a developer, I want to modify API in an easy way if it is possible and would like to have support for this out of the box.

There are a lot of JS libs that allow you to do so:

klausi commented 2 years ago

Discussed this with Andriy what options we have:

Alter schema strings before parsing (current approach): Simple, but a bit ugly since you need to parse schema strings in the alter event yourself.
Alter parsed webonyx schema: Complicated, parsed schema tree is hard to navigate and change
Introduce alter events in the parent module, so it builds the schema based off dynamic values (if a field is required or not): Complicated, would mean a lot of alter events in different parent module places.

So we opted to move along with the current approach to have something simple and expand on that if it does not satisfy our needs.

Kingdutch commented 2 years ago

The biggest thing that worries me about the current proposal for alterability is that it breaks one of best features of the GraphQL module: controllability; As a developer who creates a schema for their platform/site/product, I am in complete control of how that schema ends up. This proposal opens the door for other modules to alter my schema without me being able to do anything besides disable that module.

One of my hopes is for the popularity of GraphQL within the Drupal community to increase. This may encourage people to provide default schemas around the data that their module makes available. The current great way of "Schema Extensions" provides a good opt-in way to use (or ignore) those freebies. With the introduction of the alter events, unknowing people may be tempted to just write an alter hook, rather than walk the proper route of a schema extension.

Additionally, while I realise the proposed example use-case is just an example, I do take issue with it. The proposed change would be a breaking change in your GraphQL API. Additionally we may need to change the field mapping in addition to just the SDL, which is not possible with the proposed change.

I understand that I can not only complain so if this use-case is truly needed I'll try to propose an alternative solution that can be done without changes to the graphql module:

// my.module
use Drupal\my\Plugin\GraphQL\Schema\OverriddenSchema;

my_graphql_schema_alter(&$definitions) : void {
  if (isset($definitions['some_schema'])) {
    $definitions['some_schema']['class'] = OverriddenSchema::class;
  }
}

// src/Plugin/GraphQL/Schema/OverriddenSchema.php
namespace Drupal\my\Plugin\GraphQL\Schema;

use Drupal\graphql\Plugin\GraphQL\Schema\SomeSchema;

/**
 * Changes field in some schema.
 */
class OverriddenSchema extends SomeSchema {

  /**
   * {@inheritdoc}
   *
   */
  protected function getSchemaDefinition() {
    $schema = parent::getSchemaDefinition();
    if (!is_null($schema) {
      $schema = str_replace("name: String!", "name: String", $schema);
    }
    return $schema;
  }
}

// Alternative src/Plugin/GraphQL/Schema/OverriddenSchema.php if you do want to 
// modify the AST rather than strings.
//
// The GraphQL AST is not meant to be traversed manually but there are tools in the
// library to do this. A downside here is that the this change wouldn't be cached, so
// we may want to introduce a `loadSchemaDocument` which is called for cache
// misses, rather than calculating in `getSchemaDocument` directly. That would allow
// cached AST transforms.
namespace Drupal\my\Plugin\GraphQL\Schema;

use Drupal\graphql\Plugin\GraphQL\Schema\SomeSchema;
use GraphQL\Language\AST\NodeKind;
use GraphQL\Language\Visitor;

/**
 * Changes field in some schema.
 */
class OverriddenSchema extends SomeSchema {

  /**
   * {@inheritdoc}
   *
   */
  protected function getSchemaDocument(array $extensions = []) {
    $ast = parent::getSchemaDocument($extensions);

    $ast = Visitor::visit($ast, [
      NodeKind::OBJECT_TYPE_DEFINITION => function ($node) {
        if ($node->name->value !== "Example") {
          return NULL;
        }

        return Visitor::visit($node, [
          NodeKind::FIELD_DEFINITION => function ($field) {
            if ($field->name->value !== "name") {
              return NULL;
            }

            // Turn a non-nullable field into a nullable field.
            return $field->type;
          }
        ]);
      }
    ]);

    return $ast;
  }
}

The benefit here is that we still encourage people to write schema's and schema extensions in a way they normally would. Additionally if a rogue module decides to use this to apply an override that I don't want in my project (while keeping the rest of the module's functionality) I can easily reset the plugin definition override, without having to know (and keep up with) what was actually being overridden in the schema.

The above example is for a root schema plugin but would work similarly for a schema extension (by using the applicable alter hook).

Is the ergonomics of the alter hook really worth the extra maintenance, run-time cost and unpredictability over using Drupal's existing plugin system to achieve the same goal?

akhomy commented 2 years ago

Thank you Alexander for the alternative solution. I appreciate it and it is a pleasure to have a discussion with you.

Additionally we may need to change the field mapping in addition to just the SDL, which is not possible with the proposed change.

Well, looks like it isn't resolved yet, is it? Or it can be in your child module resolved the same way - by adding a resolver with the same params, which isn't perfect in both cases. And it seems it could be checked (topic for another discussion).

The benefit here is that we still encourage people to write schema's and schema extensions in a way they normally would. Additionally if a rogue module decides to use this to apply an override that I don't want in my project (while keeping the rest of the module's functionality) I can easily reset the plugin definition override, without having to know (and keep up with) what was actually being overridden in the schema.

Still, I don't see the difference between using alter event and the above solution for the reset. The benefit looks the same. It is much easier to write a proper schema file than to use events to provide your data. Also, you can disable/skip/reset rogue module event logic whenever you need and do not care about event adjustments.

Is the ergonomics of the alter hook really worth the extra maintenance, run-time cost and unpredictability over using Drupal's existing plugin system to achieve the same goal?

With this part, I agree, but altering definitions and extending parent class seem the same but longer way. I can understand why you don't like the opportunity to alter schema file data. In this case, it can be documented why and when it should be used. It sounds to me good to have this opportunity from the base module. As it was described before in JS libs you could do it.

klausi commented 2 years ago

Thanks, I appreciate the insights of you @Kingdutch ! As @akhomy said an alter event gives us a bit more flexibility where multiple event implementers can change the schema.

I would also like to avoid defining the schema and changing it in multiple places, but small changes like making a field required or not in the schema needs to be dynamically defined in our setup. We want to leverage full schema validation through the API, so we want to change the schema based on configuration for example if the field should be required or not. Then all the helpfulness of the GraphQL library kicks in and we can always ensure that the field is present, or loosen that on sites where it is not.

There should also be no runtime cost since the generated schema is cached at the end.

So I would continue on this alter schema event path, it should be harmless for developers that don't need it and more flexible for advanced use cases.

Kingdutch commented 2 years ago

it should be harmless for developers that don't need

That's my worry though. The current path provides no way (other than re-implementing the schema base class) for developers who don't want this to opt-out. If a misbehaving module implements the alter hook then the developer now has to go through a lot of effort to undo those changes (and actively keep up with the underlying module).

You first response may be to "not use that module" but if that's a maintainer of "Paragraphs" or some other high-usage module trying to be helpful then that's not an option.

I'm missing that part of my concerns being addressed anywhere.

We also already have ComposableSchema which I don't think we've discussed but could maybe also fill the proposed use-case. (alternatively making that more into a base-class than a completed schema). That class makes it easy to opt-in to having changes from other modules.

Two packages were linked as examples of how the JavaScript community implements this.

https://www.npmjs.com/package/graphql-override - which seems most closed to what is being proposed here is deprecated because "There are better ideas around :)" https://www.apollographql.com/docs/federation/federation-spec/#override - uses a directive, which would maybe even provide more control than the proposed implementation. However it also solves a very different problem, because it deals with schema stitching and multiple executing servers.

I would also like to avoid defining the schema and changing it in multiple places, but small changes like making a field required or not in the schema needs to be dynamically defined in our setup. We want to leverage full schema validation through the API, so we want to change the schema based on configuration for example if the field should be required or not. Then all the helpfulness of the GraphQL library kicks in and we can always ensure that the field is present, or loosen that on sites where it is not.

I'm not entirely sure I follow here. You're saying it's already possible, but then that it isn't possible?

There's also e.g. the https://git.drupalcode.org/project/graphql_compose which already does a lot of dynamic schema definition. I don't quite understand how what we're trying to do here is different? What level of control is missing in the scenario outlined in this issue that makes it impractical to use a similar solution?

If we must add the alter-hook into the schema plugin class in a very specific place and non of the other suggested options are acceptable, perhaps we can at least do so in an AlterableSdlSchemaBaseClass so that people who implement their own schemas can still decide whether they want to enable or disable this functionality without any performance cost.

akhomy commented 2 years ago

@Kingdutch , thank you for the discussion.

That's my worry though. The current path provides no way (other than re-implementing the schema base class) for developers who don't want this to opt-out. If a misbehaving module implements the alter hook then the developer now has to go through a lot of effort to undo those changes (and actively keep up with the underlying module).

With the above, I do not agree. If one of the modules alters definitions and you don't want to use it, you need again to alter it, don't you? Or just do not use this module. Same for the event, you can remove events that you don't like and have clean logic again. Could you describe more reasons why it worries you? What kind of effort worries you?

There's also e.g. the https://git.drupalcode.org/project/graphql_compose which already does a lot of dynamic schema definition. I don't quite understand how what we're trying to do here is different? What level of control is missing in the scenario outlined in this issue that makes it impractical to use a similar solution?

It generates schema files, but you need again to do definitions alter in order to replace the parent implementation, which isn't flexible. What if you need to define/override dynamic parts of the schema? Yes - you can say - don't do this, you can always use altered definitions, extend the class and implement it on your own, or better specify this in the schema file. But what if I need to change the part of parent behavior and reuse part of it? In the above case, definitions is almost equal to the event with more effort. Where controlability looks more like a point of view. What about having access to parent schemas and changing them on rare demand in child modules?

Kingdutch commented 2 years ago

If one of the modules alters definitions and you don't want to use it, you need again to alter it, don't you? Or just do not use this module.

That's the case with your proposed implementation and also exactly what worries me. From a software engineering perspective and for maintenance on large projects this is a really bad idea ™️ .

We can not predict now, what modules will try to do smart things and implement schema altering. It's possible that such a module lives deeply in your dependency tree and "just do not use this module" is not an option. Similarly it's very costly to expect developers to "you don't want to use it, you need again to alter it". This means that I need to spend time (and thus money) to actively keep up with a module and undo the changes it has (making sure every release that it didn't change its alter so that my own un-alter needs to change).

Setting up the system that way has the potential to create a huge burden for people using the GraphQL module. Ideally we should implement a better system (I'd be happy with any of my proposals) or at least provide a kill-switch (e.g. by adding the alter functionality only in AlterableSdlSchemaBaseClass).

It should be difficult for modules to do the wrong thing. It should not be difficult for project developers to get around a module doing the wrong thing anyway.

But what if I need to change the part of parent behavior and reuse part of it?

This is exactly where proper OOP design and functional composition comes into play. A better step would be to ask the module to break up the functionality so you can more granularly override things. That would benefit all users of the module.

klausi commented 2 years ago

Hm, I don't see the danger of a contrib module like Paragraphs using this alter hook. Contrib modules will never be able to alter the schema because they know nothing about it as a schema is site-specific. The use case is for internal modules that are enabled in different scenarios or different site setups and then have to influence core parts of the schema (like making a field required, which you cannot do with a schema extension).

Anyway - I think the idea from @Kingdutch is good to just have another AlterableComposableSchema class that extends and lives next to ComposableSchema in graphql module. Then our JobiqoGraphqlSchema extends AlterableComposableSchema and we can switch to that. So we offer the developers the choice which schema plugin to use, either composable or alter_composable or custom as we do with our own plugin jobiqo that inherits from either one of those 2. The standard documentation can stick with composable as it does now.

Advantages:

We can open source our schema alter event work within the graphql module
Existing sites are not affected by this change at all since most of them will not need the alter hook. They will stick to the standard composable schema.

Downside:

The UI will be a bit more confusing in exposing yet another schema plugin.

What do you think on that compromise?

akhomy commented 2 years ago

@klausi , thanks. Sounds good to me. The downside is that it either requires reworking of the protected methods protected function getSchemaDocument, protected function getExtensionDocument or makes alters after schema data is cached. I'll try to provide changes in the related PR.

akhomy commented 1 year ago

Reworked PR https://github.com/drupal-graphql/graphql/pull/1302 based on the comment https://github.com/drupal-graphql/graphql/issues/1301#issuecomment-1243882663 Will continue with tests

akhomy commented 1 year ago

Implemented tests, I think documentation can be implemented in another PR when someone else demands a similar feature.

klausi commented 1 year ago

Merged the PR, thanks!

drupal-graphql / graphql

[4.x] Allow altering of schemas #1301