READY- Applied Directives #2562

bbakerman · 2021-09-26T07:43:42Z

GraphQLDirective has been designed wrong. It's really the representation of the directive definition. Yet it and GraphqlArgument have been shoe horned into representing the instances of a directives applied to schema elements.

So this PR is s start to support for this. For backwards compat reasons the old information will be present and side by side GraphQLAppliedDirectives will be made available along with old wrong GraphQLDirectives

This is not done yet but I am putting up a PR for early feedback.

What needs to be done

✅ means its done
👍 means its QA checked
👎 means tis not right

Go through all the GraphqlDirectiveContainers and fix them up
- Make sure they use the common builder ✅
- Make sure they have the getAppliedDirectives methods ✅
- Make sure the SchemaElement copying support code works - eg named children ✅
- Make sure the common builder works via trick ✅
Make sure the schema validation catches non repeatable directive violations ✅ 👍
Go to schema gen and put in duplicate generation for applied directives along side the current directive code ✅
Put in a SchemaGen option that will NOT put in any old school objects - this will be opt in. ✅
Make sure the Schema Visitor code works and visits these elements ✅
Make sure the SchemaPrinter is working ✅
Make sure the Introspection gets the new applied directives ✅
Make sure graphql.execution.directives.QueryDirectives is working ✅

bbakerman · 2021-09-26T08:03:22Z

src/main/java/graphql/schema/GraphQLAppliedDirectiveArgument.java

+ * However with directives on SDL elements, the value is specified in AST only and transferred into the GraphQLArgument object and the
+ * 'defaultValue' comes instead from the directive definition elsewhere in the SDL.  You can think of them as 'instances' of arguments, their shape and their
+ * specific value on that directive.
+ */


TODO - fix up JavaDoc - currently copied

bbakerman · 2021-09-26T08:03:48Z

src/main/java/graphql/schema/GraphQLAppliedDirectiveArgument.java

+     *
+     * @return a value of type T which is the java value of the argument
+     */
+    public static <T> T getArgumentValue(GraphQLInputType argumentType, GraphQLAppliedDirectiveArgument argument) {


Help needed here @andimarek

andimarek · 2021-09-27T01:25:09Z

src/main/java/graphql/schema/GraphQLAppliedDirectiveArgument.java

+ * specific value on that directive.
+ */
+@PublicApi
+public class GraphQLAppliedDirectiveArgument implements GraphQLNamedSchemaElement {


regarding naming: so the question is: has this actually anything todo with directives? How about GraphQLArgumentValue?

In some ways they dont have anything to do with directives - HOWEVER the only place where we have a "name" and value is in directives in SDL and "args" in queries

We never created a GraphQLArgumentValue in queries (just map<string,object> but yeah we could.

So I like GraphQLArgumentValue or maybe GraphqlAppliedArgument

andimarek · 2021-09-27T01:28:31Z

src/main/java/graphql/schema/GraphQLAppliedDirective.java

+        return name;
+    }
+
+    @Override


does an applied directive actually have a description? Not 100% sure out of my head.

They do no have descriptions and the grammar does not allow it

However out interfaces call for it

public interface GraphQLNamedSchemaElement extends GraphQLSchemaElement { String getName(); String getDescription(); Node getDefinition(); }

probably should add Nullable then to it or add a comment

andimarek · 2021-09-27T01:30:19Z

src/main/java/graphql/schema/GraphQLAppliedDirectiveArgument.java

+    public String getDescription() {
+        return null;
+    }
+


This is @nullable

andimarek · 2021-09-27T01:30:43Z

src/main/java/graphql/schema/GraphQLAppliedDirectiveArgument.java

+    public static <T> T getArgumentValue(GraphQLInputType argumentType, GraphQLAppliedDirectiveArgument argument) {
+        return getInputValueImpl(argumentType, argument.getArgumentValue());
+    }
+


some question as above: do we actually have a Description?

See above - no

# Conflicts: # src/main/java/graphql/DirectivesUtil.java # src/main/java/graphql/schema/GraphQLArgument.java # src/main/java/graphql/schema/GraphQLEnumType.java # src/main/java/graphql/schema/GraphQLEnumValueDefinition.java # src/main/java/graphql/schema/GraphQLFieldDefinition.java # src/main/java/graphql/schema/GraphQLInputObjectField.java # src/main/java/graphql/schema/GraphQLInputObjectType.java # src/main/java/graphql/schema/GraphQLInterfaceType.java # src/main/java/graphql/schema/GraphQLObjectType.java # src/main/java/graphql/schema/GraphQLScalarType.java # src/main/java/graphql/schema/GraphQLUnionType.java

bbakerman · 2021-12-13T01:31:32Z

src/main/java/graphql/introspection/IntrospectionWithDirectivesSupport.java

         * @return the directive to be included
         */
-        GraphQLDirective getDirective();
+        String getDirectiveName();


BREAKING CHANGE!!

We could put in a shimand defaulted method but do we want to? I dont think its use enough to warrant it

Thoughts?

bbakerman · 2021-12-13T01:33:03Z

src/main/java/graphql/schema/GraphQLEnumValueDefinition.java

-            directives.clear();
-            return this;
-        }
-


All these are moved to a common Builder that is type safe

bbakerman · 2021-12-13T01:34:23Z

src/main/java/graphql/schema/GraphqlDirectivesContainerTypeBuilder.java

+
+@SuppressWarnings("unchecked")
+@Internal
+public abstract class GraphqlDirectivesContainerTypeBuilder<B extends GraphqlDirectivesContainerTypeBuilder<B, BASE>, BASE extends GraphqlTypeBuilder<BASE>> extends GraphqlTypeBuilder<BASE> {


Cool generics tricks to make sure they return the right type

bbakerman · 2021-12-13T01:37:15Z

src/main/java/graphql/util/Pair.java

+import graphql.Internal;
+
+@Internal
+public final class Pair<T, U> {


A simple Pair class

think about guava code instead of home grown

bbakerman · 2021-12-13T01:39:25Z

src/main/java/graphql/schema/idl/SchemaPrinter.java

         * @return new instance of options
         */
-        public Options includeDirectives(Predicate<GraphQLDirective> includeDirective) {
-            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeSchemaDefinition, this.includeDirectiveDefinitions, this.useAstDefinitions, this.descriptionsAsHashComments, includeDirective, this.includeSchemaElement, this.comparatorRegistry);


BREAKING CHANGE - again we could do work to make this NOT a breaking change BUT I dont think its worth it.

Thoughts?

bbakerman · 2021-12-13T01:40:37Z

src/main/java/graphql/util/Anonymizer.java

-                directivesOriginalToNewNameMap.put(directiveName, newName);
-                return CONTINUE;
-            }
-


moved upwards and made common

andimarek · 2022-02-05T09:29:04Z

src/main/java/graphql/execution/directives/QueryDirectives.java

+     *
+     * @return a map of all the applied directives immediately on this merged field
+     */
+    Map<String, List<GraphQLAppliedDirective>> getImmediateAppliedDirectivesByName();


so here the order of the keys matter, right? We should communicate this clearly that the Map keys order reflects the order of the directives found.

Mh ... actually is that true with a merged field?
Maybe it is time to make this more explicit with a Field -> AppliedDirectives result value or so

Field -> AppliedDirectives is super weird in another way. That is we are kinda saying - we can't decide so.... here you go.

I mean we are more correct - only because we are no overly useful.

The code is

final Map<Field, List<GraphQLDirective>> byField = new LinkedHashMap<>(); final Map<Field, List<GraphQLAppliedDirective>> byFieldApplied = new LinkedHashMap<>(); mergedField.getFields().forEach(field -> { List<Directive> directives = field.getDirectives(); ImmutableList<GraphQLDirective> resolvedDirectives = ImmutableList.copyOf( directivesResolver .resolveDirectives(directives, schema, variables) .values() ); byField.put(field, resolvedDirectives); // at some point we will only use applied byFieldApplied.put(field, ImmutableKit.map(resolvedDirectives, GraphQLDirective::toAppliedDirective)); });

So its in Merged field order - but that in itself might not be meaningful.

I guess this is NO worse than before in directive terms - so we can improve it later with a Field -> AppliedDirectives if we choose

andimarek · 2022-02-15T23:35:19Z

src/main/java/graphql/schema/GraphQLAppliedDirective.java

+        return name;
+    }
+
+    @Override


probably should add Nullable then to it or add a comment

andimarek · 2022-02-15T23:35:44Z

src/main/java/graphql/schema/GraphQLAppliedDirective.java

+        return null;
+    }
+
+    public List<GraphQLAppliedArgument> getArguments() {


calling it getAppliedArguments maybe?

andimarek · 2022-02-15T23:39:25Z

src/main/java/graphql/schema/GraphQLAppliedArgument.java

+import static graphql.execution.ValuesResolver.getInputValueImpl;
+
+/**
+ * This defines an argument and its value that can be supplied to a graphql field (via {@link GraphQLFieldDefinition} during


I don't think this is a true this is used for field argument values. conceptually this might be true, but a GraphQLSchemaElement is not concerned with queries/

andimarek · 2022-02-15T23:40:14Z

src/main/java/graphql/schema/GraphQLAppliedArgument.java

+        return null;
+    }
+
+    public Argument getDefinition() {


Can be null

andimarek · 2022-02-15T23:44:34Z

src/main/java/graphql/execution/directives/QueryDirectives.java

+     *
+     * @return a map of all the applied directives immediately on this merged field
+     */
+    Map<String, List<GraphQLAppliedDirective>> getImmediateAppliedDirectivesByName();


I think the bigger question is actually if applied directives should be used for queries AND schemas? conceptually they are the same, but code wise GraphQLApplieDirective is a schema element and not a query element

… version

…finitions never need this

New behavior introduced in graphql-java/graphql-java#2562

* Better union exclusion * Fix tests * Ignore applied directives New behavior introduced in graphql-java/graphql-java#2562 * Fix counter * Add tests

A start on applied directives

0dae6b5

bbakerman commented Sep 26, 2021

View reviewed changes

andimarek reviewed Sep 27, 2021

View reviewed changes

bbakerman added 3 commits October 4, 2021 12:04

More work on getting applied schemas duplicated

aca111e

Now with T extends T

7eafd24

Renamed to directivesHolder and also fixed up and extra SchemaElement

f00e244

bbakerman mentioned this pull request Oct 9, 2021

Directive validation is now done on schema build post validation #2576

Merged

bbakerman added 15 commits December 6, 2021 12:31

Put back in the ease of use builder functions

c9df4d0

Ooops - didnt know my own clever work

e3dd53b

Small tweak

975c8d2

Merge remote-tracking branch 'origin/master' into applied_directives

51fdc32

SchemaGenerator work on getting applied directives in place

f17ba1c

Added SchemaGeneratorAppliedDirectiveHelper.java to split code

fc843de

Amde the applied directive to be consistent wirth GraphQLArgument

db2c7c2

more tests

b645ea7

tests in directive wiring

986cb86

Test for schema visiting and transform - not passing

e2cc582

More work on schema printing

ae520e6

More work on introspection

e5f057b

Added Anonymizer support

39513a8

Added QueryDirectives support

b7377c8

bbakerman changed the title ~~MAJOR WIP - Applied Directives~~ READY- Applied Directives Dec 13, 2021

bbakerman commented Dec 13, 2021

View reviewed changes

graphql-java deleted a comment from andimarek Dec 13, 2021

bbakerman commented Dec 13, 2021

View reviewed changes

andimarek added this to the 18.0 milestone Feb 1, 2022

Merge remote-tracking branch 'origin/master' into applied_directives

00f6c09

andimarek reviewed Feb 5, 2022

View reviewed changes

andimarek reviewed Feb 15, 2022

View reviewed changes

bbakerman added 7 commits February 17, 2022 10:31

Merge remote-tracking branch 'origin/master' into applied_directives

033d796

Renamed GraphQLAppliedArgument to GraphQLAppliedDirectiveArgument

e69cbd4

Put in deprecation notices

a6dd683

Query applied directives added instead of re-using the scheme element…

ef4db61

… version

Removing schema repeated list methods on getDirectives - directive de…

ade00b9

…finitions never need this

deprecated value methods

5a551be

Fix ups based on Andis final review

b83014b

bbakerman merged commit a2d81a8 into master Feb 24, 2022

olim7t mentioned this pull request Jun 29, 2022

Reduce GraphQLJava induced log noise stargate/stargate#1903

Merged

4 tasks

nagliyvred mentioned this pull request Jul 11, 2022

use GraphQLAppliedDirective instead of GraphQLDirective for printing FederationSDL apollographql/federation-jvm#223

Closed

renovate bot mentioned this pull request Dec 7, 2022

Update dependency com.graphql-java:graphql-java from v11.0 to v230521 - autoclosed entur/uttu#106

Closed

1 task

renovate bot mentioned this pull request Jan 11, 2023

Update dependency com.graphql-java:graphql-java to v230521 - autoclosed grails/grails-data-graphql#98

Closed

1 task

renovate bot mentioned this pull request Jan 28, 2023

fix(deps): update dependency com.graphql-java:graphql-java from v11.0 to v230521 - autoclosed entur/uttu#153

Closed

1 task

gnawf added a commit to atlassian-labs/nadel that referenced this pull request Jun 21, 2023

Ignore applied directives

0d08a6b

New behavior introduced in graphql-java/graphql-java#2562

gnawf added a commit to atlassian-labs/nadel that referenced this pull request Jun 21, 2023

Better union exclusion (#455)

b115c98

* Better union exclusion * Fix tests * Ignore applied directives New behavior introduced in graphql-java/graphql-java#2562 * Fix counter * Add tests

READY- Applied Directives #2562

READY- Applied Directives #2562

Uh oh!

Conversation

bbakerman commented Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andimarek Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbakerman commented Sep 26, 2021 •

edited

Loading

andimarek Feb 5, 2022 •

edited

Loading