Improve ExecutableNormalizedField.isConditional for interface fields and fix a bug regarding covariant fields #2638

gnawf · 2021-12-09T20:30:26Z

Not done, need to review the logic, clean it up and add tests etc. Just putting up for early review.

This heavily compresses some queries described in GQLGW-1181.

It got the query I had down from 4k lines to 800

bbakerman · 2021-12-14T10:40:10Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

place this ImmutableKit say

bbakerman · 2021-12-14T10:45:38Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

Can we not work this out at ENF object creation time. eg we have the parent field in scope as we create child ones.
So iis it more efficient to know the "parent is an interface" and hence the child field is conditional?

This then becomes a boolean without having to look upwards

Thoughts??

We could but Nadel needs to manipulate and insert new objects which would need it computed for them.

An optimization for later I'd say?

gnawf · 2022-02-21T22:11:33Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

See the doco below for why this is wrong.

I would suggest removing it then ... this is not a public api and we don't need to deprecate thing we don't want

gnawf · 2022-02-21T22:12:04Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

Introspection fields only work directly under Query

gnawf · 2022-02-21T22:12:25Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

Deprecating these, we should never use them. As we discussed on Slack last week too.

same as the others: lets remove it

gnawf · 2022-02-21T22:14:14Z

src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java

Fixed a bug here where we weren't considering ALL the field definitions. There's a test case for this.

https://github.com/graphql-java/graphql-java/blob/6e2218b7fe66d9b1003e0a863b180b2ebc622786/src/test/groovy/graphql/normalized/ExecutableNormalizedOperationFactoryTest.groovy#L747-L791

Without this fix, the test would've returned

-Query.animal: Animal --[Cat, Dog].parent: Cat, Dog ---[Cat].name: String

Instead of

-Query.animal: Animal --[Cat, Dog].parent: Cat, Dog ---[Cat, Dog].name: String

gnawf · 2022-02-22T22:42:02Z

src/main/java/graphql/normalized/VariableAccumulator.java

Was deciding whether to mark the VariableAccumulator value as nullable or non-nullable in compileToDocument and decided to make it nullable with it defaulting to false.

andimarek · 2022-02-23T04:08:53Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

I would suggest removing it then ... this is not a public api and we don't need to deprecate thing we don't want

andimarek · 2022-02-23T04:10:31Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

+     * We MUST consider that the output type of the {@code parent} field is {@code Animal} and
+     * NOT {@code Cat} or {@code Dog} as their respective impls would say.
+     */
+    public boolean isConditional(GraphQLSchema schema, @Nullable String parentOutputTypeName) {


I don't think we should allow parentOutputTypeName to be null. This makes for an akward api. Or is there a strong reason for it?

Nah, we could make it non-nullable. I'll make that change.

andimarek · 2022-02-23T04:14:23Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

+        return parentOutputType != getOneObjectType(schema);
+    }
+
+    @Deprecated // using this is ALMOST always wrong


lets remove this instead of saying it is wrong to use it.

andimarek · 2022-02-23T04:14:31Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

        return getOneFieldDefinition(schema).getType();
    }

+    @Deprecated // using this is ALMOST always wrong


andimarek · 2022-02-23T04:21:38Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

+
+        GraphQLType parentOutputType = schema.getType(parentOutputTypeName);
+
+        // Is not conditional if field exists in interface type, and we have specified all impls


this is hard to read: can we handle the introspection field name explicitly first?

andimarek · 2022-02-23T04:22:43Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

+            GraphQLInterfaceType parentOutputTypeAsInterface = (GraphQLInterfaceType) parentOutputType;
+            List<GraphQLObjectType> interfaceImpls = schema.getImplementations(parentOutputTypeAsInterface);
+            if (interfaceImpls.size() == objectTypeNames.size()) {
+                return parentOutputTypeAsInterface.getField(this.fieldName) == null


why do we need check for field name existing in the paren type? If it doesn't exist it would imply that the schema doesn't match the ENF and I would throw an Exception normally.

Nah this is checking whether the field exists in the interface type e.g.

type Query { pet: Pet } interface Pet { name: String } type Dog implements Pet { name: String collar: String }

So the a tree can go

- [Query].pet: Pet -- [Dog].name -- [Dog].collar

We need an inline fragment for collar because it's not in Pet but we don't need one for name.

I think checking number of impls and object type names is not enough? Actually now that I think about it… maybe you're right and we don't nee to check. I'll confirm tomorrow.

andimarek · 2022-02-23T04:27:10Z

src/main/java/graphql/normalized/ExecutableNormalizedField.java

same as the others: lets remove it

src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java

andimarek · 2022-02-23T04:41:32Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

+                    ... on Dog {
+                        isGoodBoy
+                    }
+                    ... on Node {


this is a great test. We should probably have another one where this is a standalone fragment and not inlined and used in another context where ...on Node actually can include File

andimarek · 2022-02-23T04:45:31Z

src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy

+        def printed = AstPrinter.printAst(new AstSorter().sort(result.document))
+
+        then:
+        // Perhaps the typename should be hoisted out of the fragments, but the impl currently generates


thinking about the __typename hanlding: I think we need to improve this
one fundamental principle of the ENF is that regardless of the query the ENF should be same if it executes the same.
Here we can generate two different ENOperation when we place __typename directly inside the Union or inside all type conditions for all types. Both of them must produce the same ENOperation.

This is not directly related to printing, but in this test it becomes obvious.

gnawf · 2022-02-24T01:50:49Z

Fixed all comments, but please don't merge yet. Need to consider how this change affects Nadel.

gnawf · 2022-02-27T23:36:49Z

Ok ready to merge

See graphql-java/graphql-java#2638

…t gen

bbakerman reviewed Dec 14, 2021

View reviewed changes

src/main/java/graphql/normalized/ExecutableNormalizedField.java Outdated

Copy link

Member

bbakerman Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

place this ImmutableKit say

bbakerman reviewed Dec 14, 2021

View reviewed changes

gnawf force-pushed the fwang/improve-isConditional branch from 2cabee4 to 6e2218b Compare February 21, 2022 22:09

gnawf commented Feb 21, 2022

View reviewed changes

gnawf marked this pull request as ready for review February 21, 2022 22:16

bbakerman approved these changes Feb 22, 2022

View reviewed changes

Fix bug when a field from an interface is typed differently on its impls

e0c7a03

gnawf force-pushed the fwang/improve-isConditional branch from 6e2218b to 40c30d8 Compare February 22, 2022 22:37

Improve ExecutableNormalizedField.isConditional for interface fields

1a54d01

gnawf force-pushed the fwang/improve-isConditional branch from 40c30d8 to 918cc71 Compare February 22, 2022 22:40

Run AstSorter on other documents in test

d24b0d0

gnawf force-pushed the fwang/improve-isConditional branch from 918cc71 to d24b0d0 Compare February 22, 2022 22:43

gnawf commented Feb 22, 2022

View reviewed changes

andimarek requested changes Feb 23, 2022

View reviewed changes

gnawf added 2 commits February 24, 2022 14:35

Dedup some code and add comment explaining some isConditional code

14ca8c5

Add support for introspection fields

4579ec7

andimarek changed the title ~~Improve ExecutableNormalizedField.isConditional for interface fields~~ Improve ExecutableNormalizedField.isConditional for interface fields and fix a bug regarding covariant fields Feb 24, 2022

andimarek mentioned this pull request Feb 24, 2022

Adapt DataFetchingFieldSelectionSet to new ENF #2736

Closed

gnawf added a commit to atlassian-labs/nadel that referenced this pull request Feb 27, 2022

Update tests in preparation for new GraphQL Java isConditional

9cd05aa

See graphql-java/graphql-java#2638

gnawf mentioned this pull request Feb 27, 2022

Improve isConditional for ENFs atlassian-labs/nadel#384

Merged

5 tasks

undo moving getOperationTpype ... we don't want to rely on common nex…

263bbd0

…t gen

andimarek merged commit 7fe0d84 into graphql-java:master Feb 28, 2022

andimarek added this to the 18.0 milestone Feb 28, 2022


		GraphQLType parentOutputType = schema.getType(parentOutputTypeName);

		// Is not conditional if field exists in interface type, and we have specified all impls

Improve ExecutableNormalizedField.isConditional for interface fields and fix a bug regarding covariant fields #2638

Improve ExecutableNormalizedField.isConditional for interface fields and fix a bug regarding covariant fields #2638

Uh oh!

Conversation

gnawf commented Dec 9, 2021

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnawf commented Feb 24, 2022

Uh oh!

gnawf commented Feb 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants