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

Apply type system directives in macro based schemas #1140

Conversation

maartenvanvliet
Copy link
Contributor

This PR allows schemas to apply type system directives in macro based schemas. This was already possible for SDL based schemas.
It makes deprecations work through the deprecated type system directive. This directive is defined on the default prototype schema and this led to a problem with the several deprecated introspection fields. These were extracted to a separate schema phase so the prototype schema can skip them. In Absinthe 2.0 this phase can be removed altogether but I've kept it for now for backwards compatibility.

  • Some work for a followup is to validate the arguments passed into applied type system directives. This is currently also not done for SDL based schema afaict.

  • Schema level directives also are not possible yet e.g.

schema @feature(name: ":schema") {
  query: Query
}

Since macro based deprecations are no longer special cased it will also fix #1045

This change was necessary to to implement macro-based directive
handling.

Because these fields are deprecated, they would use the new `deprecated`
directive. This directive definition is taken from the prototype schema.
However, during compilation of the prototype schema it would also invoke
the introspection builtins with 'deprecated' directive, which is not yet
available.

This new phase extracts the deprecated fields, thus allowing the
prototype phase to skip it in its schema pipeline to avoid the problem.

In Absinthe 2.0 the entire phase can be removed but as of now it's
a backwards incompatible change.
This also handles deprecation at the directive level, so it works
similar to SDL schema's.
@@ -310,6 +310,9 @@ defmodule Absinthe.Middleware do
[{:ref, Absinthe.Type.BuiltIns.Introspection, _}] ->
expanded

[{:ref, Absinthe.Phase.Schema.DeprecatedDirectiveFields, _}] ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really happy with this, but can be removed when these fields are no longer necessary.

end
```
"""
defmacro directive(identifier, attrs \\ [], do: block) do
defmacro directive(identifier, attrs, do: block) when is_list(attrs) when not is_nil(block) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

directive/1,2,3 can be used for both defining directives as applying them to types in the schema.

I think this is clear for the users but perhaps there's another function name we can use for applying the directive.

Copy link
Contributor

@ymtszw ymtszw left a comment

Choose a reason for hiding this comment

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

Amazing! As commented in #1045 I was also looking into the issue and saw it a huge undertaking to properly support type system directives in macro schema by my hand, even for only supporting deprecated.

As a workaround I was to adopt @zoldar's gist on userland (which works for now, at least for deprecated) but this patch should nicely remove the extra work. Love it!

lib/absinthe/schema/notation.ex Outdated Show resolved Hide resolved
Co-authored-by: Yu Matsuzawa <ymtszw@gmail.com>
@ymtszw
Copy link
Contributor

ymtszw commented Jan 9, 2022

A step to move #1003 forward (by a lot!)


query do
field :post, :post do
directive :feature, name: ":field_definition"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused about these strings that look like atoms? are they arbitrary strings and you just happened to use strings that look like atoms or is there some significance to this format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's arbitrary, in the same test file there's an SDL schema that follows the same format (https://github.com/absinthe-graphql/absinthe/blob/2390500573c08d882143b48bc02f455ae5d0fd86/test/absinthe/schema/type_system_directive_test.exs). The macro-schema is a near copy of that.

@kdawgwilk
Copy link
Contributor

❤️ Really looking forward to this since it will greatly simplify the https://github.com/DivvyPayHQ/absinthe_federation library

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.

4 participants