graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

On specifying ordered vs unordered enum definitions #1062

Closed cdaringe closed 1 year ago

cdaringe commented 1 year ago

Problem

There is no specification on whether EnumTypeDefinition are sorted by Name, and it causes friction for GQL clients who presume there is order.

Context

As observed from the above, ordering matters during codegen to GQL consumers. If a schema maintainer changed the enum to enum { Yy Xx Zz }, some clients would see this as a breaking change in the status quo.

Discussion

Personal recommendation: enums and tagged unions alike are Sets, thus intrinsically unordered. We should state as much. Equipped with this assertion, client libraries whom seek to do type generation should apply stable sorting.

benjie commented 1 year ago

Good catch. In my opinion, like fields, they should be in the order in which they were defined, which effectively means tools are free to make their own decisions. Either way, the order should be stable in my opinion:

  1. introspect a GraphQL schema
  2. build a schema from these introspection results
  3. introspect this new schema using the same introspection query

The results of 1 and 3 should be identical.

cdaringe commented 1 year ago

@benjie! long time, no-GitHub-see :)

Either way, the order should be stable in my opinion ...

Just to clarify, are you asserting that you think, in the spec, EnumTypeDefinition should specify that entries are ordered?

martinbonnin commented 1 year ago

We've been occasionaly hit by such sorting issues in Apollo Kotlin and the more I think about it the more I think the "good" order for tools is the schema order. Everything else is surprising and error prone.

Sorting codegen by name is dangerous because the schema author doesn't control the sorting anymore so adding an enum starting with "a" offsets all the subsequent ones. All in all, my favorite solution is to keep things as is and have tools that analyze schema changes output a warning for such order changes.

benjie commented 1 year ago

@cdaringe :wave: :grin:

Just to clarify, are you asserting that you think, in the spec, EnumTypeDefinition should specify that entries are ordered?

Yes; I was writing a reply here but figured more valuable to just make the changes I want to see... so please see explanation in https://github.com/graphql/graphql-spec/pull/1063

cdaringe commented 1 year ago

Sounds good. Let’s move subsequent convo over there