GeezerVison is showing github.com — Did PR #4229 go too far? · Issue #4259 · graphql-java/graphql-java
Did PR #4229 go too far? #4259DescriptionPR #4229 unifies the behavior of all built-in directives, where in the past skip and include was treated differently from other built ins. In the process, the GraphQLSchema.Builder.clearDirectives method was removed, because this was one of the places where different built-ins were being treated differently.
However, we have a use-case where we use GraphQLSchema.transform to transform all of the non-built-in directives in a schema. This works by taking all the existing non-graphql-builtin directives from the source schema, then calling clearDirectives on the builder for the new schema, then adding transformations of the original directives into the new schema. So a semantics of clearDirectives which is "clear all non-graphql-builtin directives" would be helpful. So perhaps the change in #4229 should've been: "clearDirectives clears non-graphql built-in directives, but treats all graphql built-ins uniformly (contra the old behavior)" -- rather than getting rid of it altogether. Activityandimarek commented on Feb 23, 2026 Yes I think we can add it again but preventing built in scalars to be removed. rstata commented on Feb 24, 2026 i'm thinking that it should delete all directives including built-ins. users are then free to add in their own version of the built-ins, and ensureBuiltInDirectives will add in any that they miss. this would mimic the behavior they experience with a newly-created Builder. andimarek commented on Feb 24, 2026 I am not sure about allowing users to remove built in directives.
I don't think we really support that intentionally, maybe by accident.
Do you have a specific reason for removing built ins? rstata commented on Feb 24, 2026 as i understand it, the way a "fresh" schema builder works is that the builder starts with no directive definitions, and developers can add definitions for built-in directives if they want. at build-time buildImpl calls ensureBuiltInDirectives which will add definitions for built-in directives that have not already been added by developers.
i'm suggesting here the same behavior for clearDirectives: it resets the state to what you would've gotten with a fresh builder (ie, no directives), and again we rely on the build process to ensure that all built-in directives are defined.
but since we're talking about this behavior, i don't really like it, because (again as i understand it) the developer can add a definition of skip for example that doesn't have an if argument, which would cause chaos. so what we could do here is to always initialize the directives list of a builder to have our pre-defined built-in directive definitions -- this is what you'd get with a fresh builder, and this is what you'd get with clearDirectives -- and we throw an error if you attempt to redefine a built-in directive. alternatively, ensureBuiltinDirectives could check that the def's provided for built-ins conform to the spec.
in any event, i think that clearDirectives should reset the directive-definitions state of a Builder to the state you get from a fresh builder, no matter what we think that state should be...
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|