Skip to content

Apply directives and @specifiedBy to scalars - #894

Open
oryan-block wants to merge 1 commit into
masterfrom
bugfix/864
Open

oryan-block wants to merge 1 commit into
masterfrom
bugfix/864

Conversation

@oryan-block

Copy link
Copy Markdown
Collaborator

Fixes #864

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Description

SchemaClassScanner rebuilt every custom scalar from the provided GraphQLScalarType with only its name, description and coercing. So directives from the SDL never made it, on the scalar or on extend scalar, and neither did @specifiedBy, the provided scalar's specifiedByUrl or SchemaDirectiveWiring.onScalar:

scalar Date @specifiedBy(url: "https://example.com/date")

printed as scalar Date, and introspection returned specifiedByURL: null.

Now SchemaParser builds scalars like the other types: directivesOf, buildAppliedDirectives/buildDirectives at SCALAR, then the new DirectiveWiringHelper#wireScalar. The scanner only checks that each scalar the schema uses was provided.

Main issue is that graphql-java compares types by identity, so every field, argument, input field and directive argument has to hold the same scalar instance. And a scalar's directives can take that scalar as an argument (directive @since(date: Date) + scalar Date @since(date: "2020-01-01")). So:

  • SchemaParser#scalarType builds a scalar the first time it's looked up and keeps it. It's the only way to get one now.
  • It builds the directives the scalar applies first, so no directive definition gets cached with a type reference to the scalar, whatever order they're declared in.
  • A directive's first build in SchemaDirectives builds the scalars that apply no directives and uses a type reference for the rest.
  • The one place a reference is left is a directive applied to a scalar whose argument uses a scalar that's still being built, e.g. Date's own @since(date:). graphql-java replaces it when it builds the schema, but inside onScalar getValue() on that argument returns null.

What wins when both set something: the SDL description, then the provided one (same as before). The SDL @specifiedBy, then the provided specifiedByUrl. The provided scalar's own directives are dropped, like graphql-java's SchemaGenerator does and like before this change. Copying them meant fixing up their argument types and dealing with undeclared ones, for something nobody asked for.

Release notes:

  • Scalars get the directives of their definition and extensions, @specifiedBy and onScalar wiring. Static directive wirings and a WiringFactory now get onScalar calls for custom scalars too.
  • The provided scalar's specifiedByUrl (e.g. from extended scalars) now shows up in introspection, and in the printed schema with graphql-java 26.
  • ⚠️ An undeclared directive or an unknown argument on a scalar logs a warning and the scalar is built without its directives from the schema. It'll throw in 15.0.0 (Reject invalid directives on scalars in 15.0.0 #893).
  • ⚠️ A directive argument on a scalar whose value doesn't fit its type (@specifiedBy(url: 5)) now fails the build in graphql-java's schema validation, like on every other type. Before it built because the directive was ignored.

Tests: scalar cases in TypeExtensionTest (directives of the scalar and its extension, wired) and DirectiveTest (SDL vs provided specifiedByUrl and description through introspection, the cycle above in both declaration orders, the undeclared directive warning, and a custom scalar argument read during wiring when its directive is applied inside its own argument type). They all fail on master except the last one, which passes on master and guards a regression an earlier version of this change had. mvn clean verify passes on JDK 25 against graphql-java 26.1 and 25.1.

Also checked through the public API with a probe app (SDL file, .scalars(...), a wiring per directive). On master the scalars print bare and every specifiedByURL is null. On this branch:

"A calendar date, wired with @since(null), wired with @unit(day)"
scalar Date @since(date : "2020-01-01") @unit(name : "day") @specifiedBy(url : "https://example.com/date")

"An RFC-3339 date-time"
scalar DateTime @specifiedBy(url : "https://scalars.graphql.org/andimarek/date-time")

DateTime's URL comes from the provided scalar. The @since(null) is the reference case above. Same results with graphql-java 25.1 at runtime, except its printer doesn't show a URL that only the provided scalar sets (introspection does).

Scalars were rebuilt in SchemaClassScanner from the provided GraphQLScalarType with only its name, description and coercing, so
directives from the SDL, @specifiedBy, the provided specifiedByUrl and onScalar wiring were all lost.

Co-Authored-By: Claude <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

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.

Scalars lose their directives, including @specifiedBy

1 participant