Add failing test for macro deprecated field in SDL render

[maartenvanvliet]

A field that is deprecated in the macro defined schema's is not rendered as deprecated in SDL. See this failing test case. This is a regression from Absinthe 1.5.5

In 1352d8f the deprecation directive was no longer special-cased, which works for SDL defined schema's, but not in macro defined schema's. The difference being that the macro schema's don't add a deprecated directive toAbsinthe.Blueprint.Schema.FieldDefinition.directives, so it only relies on the FieldDefinition.deprecation field.

SDL schema's directives do add the deprecated directive to the directives field, so they are rendered correctly in SDL. They rely on this directive expansion to set the deprecation field.

We could either special case the deprecated directive again in the SDL renderer, or use the macro schema to add a deprecated directive when deprecating a field (perhaps through a schema phase)

[binaryseed]

Thanks, good find. It seems like adding the directive is moving in the right direction...

[aerosol]

I started looking at this, but it's quite difficult to figure out what needs to be done for someone unfamiliar with the internals. Is the idea to introduce a Phase that'd extend relevant nodes' directives with Directive instances in case the deprecation value is present? How would one go about instantiating the Directive struct given the blueprint input? It's also unclear to me how to provide the argument (reason) for later expansion there.

[zoldar]

FWIW here's the workaround we have used to re-add directives back at the time https://gist.github.com/zoldar/d617f2037854ccfc26cd428e11df6ed7

[chakeresa]

We are also seeing this issue (upgrading from 1.5.5 to 1.6.5 removed all the deprecation warnings from our introspected schema). Are there any updates on fixing this bug? I am hesitant to update, for this reason :-/.

[ymtszw]

@binaryseed Hi, let me clarify the situation so that I could potentially tackle on the issue:

• Currently Absinthe DOES support "defining" directives in macro schema, but DOES NOT support "calling/applying/using" defined directives as per https://elixirforum.com/t/how-to-apply-directives-on-fields-in-absinthe/20089/2
  Is this still correct?
  • @deprecated default directive is special-cased, has its special "applying" notation in Absinthe right now.
• If the above holds, the most "proper" way to solve this issue is:
  • (1) introduce the whole directive mechanisms to support "applying" any (including user-defined) directives to various GQL locations as per https://spec.graphql.org/draft/#DirectiveLocation
  • and (2) render SDLs based on actually applied directives in our schema
  • then (3) with (2), @deprecated and maybe-future-coming other "default directives" (such as @skip) are mere directives inside, so they are rendered alongside with user-defined directives
    • Maybe then we can remove special-casing of @deprecated, like :deprecation in type structs
• "The whole directive mechanisms" is obviously a significant undertaking. My gut feeling says for now we should keep special-casing @deprecated and use :deprecation in SDL rendering. What do you think on this?
  • Since deprecation mechanism is crucial for incremental type-safe development, I foresee many users will benefit from this.
  • For example, in our workflow we utilize GraphQL Inspector for detecting deprecations/breaking changes, so properly rendering @deprecated in SDL greatly contributes to our type-based confidence!

---

BTW, I have found that @deprecated on args and input fields were rather new and still not merged to the official spec, to my surprise.
graphql/graphql-spec#805

[benwilson512]

@ymtszw that forum post is out of date. Absinthe does support calling directives in SDL.

see: https://github.com/absinthe-graphql/absinthe/blob/master/test/absinthe/schema/notation/experimental/import_sdl_test.exs#L567-L592

[ymtszw]

@benwilson512 Thanks, now it became clearer. Shame I haven't caught up with the latest features.

So the missing part is: in macro-defined schema, collaterally add "deprecated" directives in nodes when they have deprecate/1 macro or :deprecate attr attached, right? (Is this what @binaryseed meant in his post?)

I could possibly investigate this field in coming weekend(s). Any other guidance would be welcome if any.

[ymtszw]

I would still really love to tackle on this but couldn't have taken my time. Maybe soon...

FWIW our use case:

• We are auto-generating schema.graphql and committing it to git
• On GitHub Actions we call graphql-inspector to detect intentional/unintentional schema changes
• graphql-inspector can report removals of non-deprecated types and fields as errors

For this workflow, we compare schema.graphql in a PR branch against one from its base branch. But due to the issue, it cannot track deprecation => removal flow.
The inspector can be manually suppressed using predefined labels (such as approved-breaking-changes) so it isn't a show-stopper. But proper detection of removal-after-deprecation is definitely an improvement.

[ymtszw]

Related attempt: #1110

[maartenvanvliet]

I've created a PR to apply directives at the schema which also covers deprecations. This would also cover the @deprecated directive

[chakeresa]

Awesome! Here’s the PR if anyone else is curious: #1140