apollographql / federation-jvm

JVM support for Apollo Federation
https://www.apollographql.com/docs/federation/
MIT License
245 stars 65 forks source link

Federation scalars declared in the schema file don't get wired up by the transformer #380

Closed craig-day closed 6 months ago

craig-day commented 6 months ago

I believe that #352 introduced a bug in the federation v2 transformation. I have a schema that includes some federation scalar definitions, so I can use them elsewhere in the schema. For example, here is a snippet of the relevant parts:

scalar federation__FieldSet
scalar link__Import

directive @link(as: String, import: [link__Import], url: String!) repeatable on SCHEMA
directive @key(fields: federation__FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE

extend schema
@link(
    url: "https://specs.apollo.dev/federation/v2.3",
    import: ["@key", "@shareable", "@provides", "@external", "@tag", "@extends", "@override", "@inaccessible", "@composeDirective"]
)

type Post @key(fields: "id") {
  id: ID!
}

query {
  getPost(id: ID!): Post
}

Because I declare the scalar in my schema, it will be part of the TypeDefinitionRegistry that gets created from parsing the SDL. With the change from #352, specifically this line: https://github.com/apollographql/federation-jvm/blob/149e4c533d6300fc8b14d8325bd1729b59c82438/graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/Federation.java#L167

The transformer no longer wires it up for me. I think it was more correct in the previous code to check if the RuntimeWiring contained wiring for the scalar. This way I can declare and use the federation scalars in my schema, but don't have to be responsible for wiring them up and the transformer can do that for me.

craig-day commented 6 months ago

I am aware I can remove all the manual definitions from my own schema file, but then I have to fight with my linter and my IDE to get them to understand what it's happening, which I didn't have to do in the past.

craig-day commented 6 months ago

I opened #382 with one approach to resolve this.

dariuszkuc commented 6 months ago

Thanks for the fix! I released v4.4.1 with the changes.