Skip to content

fix: ensure default directives do not get replaced by custom directives #27

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

Merged
merged 3 commits into from
Aug 16, 2022

Conversation

jmauerhan
Copy link
Contributor

@jmauerhan jmauerhan commented Aug 16, 2022

Proposed changes

  • This PR fixes a bug where our custom directives for federation were replacing the default directives, making queries which included include or skip not work correctly - the underlying webonyx/graphql-php library would throw this error: "Unknown directive \"include\".
  • By merging the custom directives with the standard ones, we now have all of the required directives.

How to test

  • In addition to unit tests, I tested locally with a federated schema. This fix allowed a query which used @include across a federated schema to properly resolve.

Can also confirm by running this query to see the directives:

query{
  __schema{
    directives{
      name
    }
  }
}

before:

{
  "data": {
    "__schema": {
      "directives": [
        {
          "name": "key"
        },
        {
          "name": "external"
        },
        {
          "name": "requires"
        },
        {
          "name": "provides"
        }
      ]
    }
  }
}

with the fix:

{
  "data": {
    "__schema": {
      "directives": [
        {
          "name": "key"
        },
        {
          "name": "external"
        },
        {
          "name": "requires"
        },
        {
          "name": "provides"
        },
        {
          "name": "include"
        },
        {
          "name": "skip"
        },
        {
          "name": "deprecated"
        }
      ]
    }
  }
}

Unit Tests

  • This PR has unit tests
  • This PR does not have unit tests: why?

@jmauerhan jmauerhan requested review from cberube, maccath and xiian August 16, 2022 17:21
Copy link
Contributor

@xiian xiian left a comment

Choose a reason for hiding this comment

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

👍

To provide a bit more context:

With webonyx/graphql-php, The directives are typically accessed via Schema::getDirectives(), which will return the directives from the config OR GraphQL::getStandardDirectives(), which calls Directive::getInternalDirectives() just like this PR.

And since we are defining directives on the config, we would need to include the internal directives as well.

There may be a case made for using GraphQL::getStandardDirectives() instead of Directive::getInternalDirectives(), but I don't see any value in the extra step right now.

@jmauerhan
Copy link
Contributor Author

jmauerhan commented Aug 16, 2022

There may be a case made for using GraphQL::getStandardDirectives() instead of Directive::getInternalDirectives(), but I don't see any value in the extra step right now.

I actually had that first, but since it's using array_values, we lose the key names (which are the directive names), so it doesn't work well here.

public static function getStandardDirectives() : array
{
    return array_values(Directive::getInternalDirectives());
}

@jmauerhan jmauerhan merged commit 4f958dd into main Aug 16, 2022
@jmauerhan jmauerhan deleted the fix-federated-schema-standard-directives branch August 16, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants