Improving GraphQL type handling and upgrading testng #288

manikandan-ravikumar · 2022-10-12T07:09:06Z

Partial update to entities

The purpose of this PR is to enable updation of selective entity fields. By introducing Optional, it also differentiates null from undefined

Let's assume an entity with 5 fields and the requirement is to update only one field to null. We can set the value of that field to null and omit other fields in a mutation query.

Changelog

Fixing import issues with javax.xml.bind.DatatypeConverter
Bump testng version from 6.9.10 to 7.4.0
New GraphQLUndefined type
Following improvements in MethodDataFetcher:
1. Improved Nullable handling using Optional
2. Handle Optional field types
3. Support RawType in additional to WrpperType
GraphQLInputTest updated to test Optional field types in GraphQL queries

manikandan-ravikumar · 2022-10-24T08:27:51Z

Hi @yrashk or anyone,
Is it possible to push this PR further. We are planning to get rid of the fork that we have been using in our project and directly include this repo as a dependency. I have another related PR here #287. I would really appreciate it if anyone could take a look 🙏

tdraier

Hi, thanks for the PR !

We're now helping on project maintenance, I have reviewed and tested your code. I was quite confused trying to understand what you were trying to achieve, the test case does not reflect any new possibility you introduced.
In the end, I think the support for Optional can be achieved by just adding a new case in buildArg.
Also we did not understand what was the GraphqlUndefined - from what I've seen the class itself is actually never really used .. ?
See comments inline

tdraier · 2023-05-11T09:32:10Z

src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java

+        Optional<Object> optionalArg = Optional.ofNullable(arg);
+        if (!optionalArg.isPresent()) {
            return null;
        }


This creation of optional is useless and makes the code much more confusing and complex to understand. You just test that optional isPresent, otherwise return null. An optional which always have a value is not an optional anymore.

tdraier · 2023-05-11T09:34:10Z

src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java

+            if (arg == null) {
+                return null;
+            } else {
+                return Optional.ofNullable(buildArg(subType, new GraphQLUndefined(), arg));


Here I think we have a major issue : the second parameter should be graphqlType, we are just creating an optional of the arg parameter. If subtype is another parameterized type (a list), it won't be correctly parsed. Anyway it's strange to use GraphqlUndefined here since the value is defined.

tdraier · 2023-05-11T09:34:59Z

src/main/java/graphql/annotations/type/GraphQLUndefined.java

+
+import java.util.List;
+
+public class GraphQLUndefined implements GraphQLType {


I don't understand the meaning or usage of this class. It's only used in buildArg, in a value that will be thrown away just after.

tdraier · 2023-05-11T09:36:27Z

src/test/java/graphql/annotations/GraphQLInputTest.java

+    public void queryWithUndefinableParameters() {
+        GraphQLSchema schema = newAnnotationsSchema().query(QueryUndefinedParameter.class).build();
+
+        GraphQL graphQL = GraphQL.newGraphQL(schema).build();
+        ExecutionResult result = graphQL.execute("{ something(code: {firstField:\"a\",secondField:\"b\"}) otherthing(code: {secondField:\"c\"}) listthings somethingWithOneField(code: {}) }", new QueryUndefinedParameter());
+        assertTrue(result.getErrors().isEmpty());
+        assertEquals(((Map<String, String>) result.getData()).get("something"), "ab");
+        assertEquals(((Map<String, String>) result.getData()).get("otherthing"), "c");
+        assertEquals(((Map<String, String>) result.getData()).get("listthings"), "was null");
+        assertEquals(((Map<String, String>) result.getData()).get("somethingWithOneField"), "was undefined");
+    }
+


Since the goal of this story (from my understanding) is to make the distinction between {} and {value: null} , we should at least have a test that shows it. The use cases described here already work without the proposed changes.

tdraier · 2023-05-11T09:36:58Z

src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java

        } else {
-            return arg;
+            return optionalArg.orElse(null);


again, return arg was much more clear

tdraier · 2023-05-11T09:44:50Z

src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java

+                if (parameters[0].getType().isAssignableFrom(Optional.class)) {
+                    return constructNewInstance(constructor, arg);
+                } else {
+                    return constructNewInstance(constructor, optionalArg.orElse(null));
+                }


I don't see how this case can work. The constructor has one single parameter which is assignable from arg.getClass() (Map, for example) and at the same time Optional.class , and in that case , you don't pass the optional but the arg ? otherwise, you keep the previous behaviour , equivalent to return constructNewInstance(constructor, arg); ? In that case, rather keep the previous code

tdraier · 2023-05-11T09:45:45Z

src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java

            } else {
                List<Object> objects = new ArrayList<>();
-                Map map = (Map) arg;
+                Map map = (Map) optionalArg.orElseGet(Collections::emptyMap);


OptionalArg isPresent (you test that at the beginning). Keep Map map = (Map) arg;

tdraier · 2023-05-11T09:50:48Z

src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java

+            if (((ParameterizedType) p).getRawType() == Optional.class) {
+                if (!optionalArg.isPresent()) {
+                    return null;
+                } else {
+                    Type subType = ((ParameterizedType) p).getActualTypeArguments()[0];
+                    return Optional.ofNullable(buildArg(subType, graphQLType, arg));
+                }
+            } else {
+                List<Object> list = new ArrayList<>();
+                Type subType = ((ParameterizedType) p).getActualTypeArguments()[0];
+                GraphQLType wrappedType = ((GraphQLList) graphQLType).getWrappedType();

-            for (Object item : ((List) arg)) {
-                list.add(buildArg(subType, wrappedType, item));
+                for (Object item : ((List) optionalArg.orElseGet(Collections::emptyList))) {
+                    list.add(buildArg(subType, wrappedType, item));
+                }
+                return list;
+            }


Keep this whole case unchanged, and move the next case (p instanceof ParameterizedType && ((ParameterizedType) p).getRawType() == Optional.class) before the list one

tdraier

Hi, thanks for the PR !

We're now helping on project maintenance, I have reviewed and tested your code. I was quite confused trying to understand what you were trying to achieve, the test case does not reflect any new possibility you introduced.
In the end, I think the support for Optional can be achieved by just adding a new case in buildArg.
Also we did not understand what was the GraphqlUndefined - from what I've seen the class itself is actually never really used .. ?
See comments inline

Fgerthoffert · 2023-08-25T08:05:59Z

Hi @manikandan-ravikumar,

We will be closing the PR for the time being, there are interesting improvements in your initial proposal but it required some work to be integrated into the codebase.

We don't have the bandwidth to do these changes ourselves for now, but if you're still interested, feel free to re-open the PR and submit the requested changes.

Thanks,

improving graphql type handling and improving testng version

961ae12

manikandan-ravikumar mentioned this pull request Oct 12, 2022

Improving GraphQL type handling and upgrading testng Nosto/graphql-java-annotations#5

Closed

tdraier requested changes May 11, 2023

View reviewed changes

Fgerthoffert closed this Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improving GraphQL type handling and upgrading testng #288

Improving GraphQL type handling and upgrading testng #288

Uh oh!

manikandan-ravikumar commented Oct 12, 2022 •

edited

Loading

Uh oh!

manikandan-ravikumar commented Oct 24, 2022

Uh oh!

tdraier left a comment

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier May 11, 2023

Uh oh!

tdraier left a comment

Uh oh!

Fgerthoffert commented Aug 25, 2023

Uh oh!

Uh oh!


		import java.util.List;

		public class GraphQLUndefined implements GraphQLType {

Improving GraphQL type handling and upgrading testng #288

Improving GraphQL type handling and upgrading testng #288

Uh oh!

Conversation

manikandan-ravikumar commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Partial update to entities

Changelog

Uh oh!

manikandan-ravikumar commented Oct 24, 2022

Uh oh!

tdraier left a comment

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!

tdraier left a comment

Choose a reason for hiding this comment

Uh oh!

Fgerthoffert commented Aug 25, 2023

Uh oh!

Uh oh!

manikandan-ravikumar commented Oct 12, 2022 •

edited

Loading