New validation rules about type, field, argument by dugenkui03 · Pull Request #1955 · graphql-java/graphql-java

dugenkui03 · 2020-06-13T03:14:55Z

The validation about GraphQLObjectType, GraphQLInterfaceType, GraphQLUnionType, GraphQLEnumType, GraphQLInputObjectType, GraphQLScalarType.

Types must define one or more fields;
Enum type must define one or more enum values;
Union type must include one or more unique member types;
The member types of a Union type must all be Object base types;
Non‐Null type must not wrap another Non‐Null type;
Invalid name begin with "__" (two underscores).

Details in specification: TypeSystem

andimarek

Thanks a lot for these new rules.

Please have a look at my comments.

andimarek · 2020-06-19T06:14:40Z

+
+    private boolean isScalarOrIntrospectionTypes(GraphQLNamedType type) {
+        if (type instanceof GraphQLScalarType && (
+                type.getName().equals("Int") || type.getName().equals("Float") || type.getName().equals("String") ||


we should only treat the build-in scalar types (Int, Float, String, Boolean and ID) in a special way. The others are not really special and should be treated as every other custom scalar.

We also have a helper method in Scalars (I believe) to determine if the type is a build-in one or not

Thanks for your suggestion.

andimarek · 2020-06-19T06:17:37Z

+            return;
+        }
+
+        List<Type> memberTypes = definition.getMemberTypes();


we should not need to check the AST definition here. Just getting the members of the type should be enough.

types in GraphQLUnionType.Builder is a Map(code link), which will lost the type with duplicated name.

Maybe it will be better to move unique member check to SchemaTypeChecker.

I will followed your suggestion instead of removed it, because types definied in GraphQLUnionType is list after all.

Then I will add unique member check to SchemaTypeChecker in another PR.

Is that appropriate?

andimarek · 2020-06-19T06:18:48Z

+        }
+    }
+
+    private void validateObject(GraphQLObjectType type, SchemaValidationErrorCollector errorCollector) {


can we combine validateObject and validateInterface?
I would also suggest a more prescriptive name like validateContainsField or so.

Thanks for your suggestion.

andimarek · 2020-06-19T06:19:20Z

+            return true;
+        }
+
+        if (type instanceof GraphQLObjectType && (


IntrospectionQuery is not a special type at all (it is just a name).

graphql-java framework contains a type named IntrospectionQuery, which contains fields start with '__', and is used for introspection.

I add a method named isIntrospectionTypes(), which is used in filterBuiltInTypes(). details in new commit.

andimarek · 2020-06-19T06:19:51Z

+        return graphQLNamedTypes.stream().filter(type -> !isScalarOrIntrospectionTypes(type)).collect(Collectors.toList());
+    }
+
+    private boolean isScalarOrIntrospectionTypes(GraphQLNamedType type) {


could this whole method just check for __ prefix?

Replace it by filterBuiltInTypes, which aimed to filter Built-In Types.

andimarek · 2020-06-19T06:21:21Z

+            return Collections.emptyList();
+        }
+
+        return graphQLNamedTypes.stream().filter(type -> !isScalarOrIntrospectionTypes(type)).collect(Collectors.toList());


We have static util methods (in FpKit I believe), please use them instead of using directly filter etc.

Thanks for your suggestion.

andimarek · 2020-06-19T06:21:52Z

+class TypeAndFieldRuleTest extends Specification {
+
+
+    def "type must define one or more fields."() {


we should add tests for Interfaces here as well.

Thanks for your suggestion.

andimarek · 2020-06-19T06:23:37Z

+            errorCollector.addError(validationError);
+        }
+
+        List<String> typeNames = new ArrayList<>();


I think a Set is better here as track just the names we already saw.

Thanks for your suggestion.

dugenkui03 · 2020-09-13T16:30:12Z

hi @andimarek , appreciate your time for this. is there any thing else I need to do for this PR?

I think is is pretty important to improve schema validation in future release version.

tfij · 2020-10-21T07:49:35Z

Hi, is there any way to help in review of this PR?
Correct schema validation is very much expected by my.

…graphql-java v16 since graphql-java/graphql-java#1955

* Fixed test to include a field because validation in graphql now requires ypes to have one or more fields ( graphql-java/graphql-java#1955 ) * Removed references to arguments when recreation ExecutionStrategyParameters because arguments field was dropped in graphql-java ( https://github.com/graphql-java/graphql-java/pull/2023/files#diff-d31b54bc13a6051c43027e76627c6e99de28f64adfcfcfb275856ec00890cc4fL44 ) * Updated tests to work with immutable structures that graphql-java has moved to ( https://github.com/graphql-java/graphql-java/pull/2086/files#diff-9fed7fa709720e6a47a80119bb991def6fa752f8933d9f5e3dfcede5520c07d6R134 )

add some schema validation rules: type, field, argument.

a90e4e3

dugenkui03 force-pushed the addSchemaValidationRules branch from 256cb9a to a90e4e3 Compare June 13, 2020 06:55

dugenkui03 marked this pull request as ready for review June 13, 2020 07:02

dugenkui03 changed the title ~~add new schema validation rules~~ New schema validation rules Jun 13, 2020

dugenkui03 changed the title ~~New schema validation rules~~ New validation rules about type, field, argument Jun 13, 2020

dugenkui03 mentioned this pull request Jun 18, 2020

Issue 1882: Refactor the module of schema validation, added some schema validation rules which defined in GraphQL specification #1881

Closed

andimarek requested changes Jun 19, 2020

View reviewed changes

dugenkui03 requested a review from andimarek June 19, 2020 19:17

dugenkui03 force-pushed the addSchemaValidationRules branch from a23d7e4 to 59a1377 Compare June 20, 2020 11:54

add isIntrospectionTypes method in Introspection.

9ff53d2

dugenkui03 force-pushed the addSchemaValidationRules branch from 59a1377 to 9ff53d2 Compare June 21, 2020 04:28

dugenkui03 added 2 commits June 25, 2020 15:52

rules about input object type

982cd29

merge master and fix conflict; update unit test for GraphQLUnionTypy

9365b90

dugenkui03 mentioned this pull request Oct 15, 2020

Add schema validator to check if object type is not empty #2076

Closed

bbakerman added this to the 16.0 milestone Nov 10, 2020

bbakerman merged commit c801fd8 into graphql-java:master Nov 10, 2020

hrkfdn mentioned this pull request Nov 25, 2020

Update to graphql-java v16.1 apollographql/federation-jvm#84

Closed

sachindshinde mentioned this pull request Dec 9, 2020

graphql-java-support: Bump graphql-java to v16.1 apollographql/federation-jvm#85

Merged

sachindshinde added a commit to apollographql/federation-jvm that referenced this pull request Jan 19, 2021

Remove testPrinterEmpty() since empty types are no longer allowed in …

636e915

…graphql-java v16 since graphql-java/graphql-java#1955

anttip mentioned this pull request May 19, 2021

Updating to use latest graphql-java:16.2 library Enigmatis/graphql-java-annotations#270

Merged

		class TypeAndFieldRuleTest extends Specification {


		def "type must define one or more fields."() {

Conversation

dugenkui03 commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andimarek 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!

dugenkui03 Jun 19, 2020 • 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!

dugenkui03 Jun 19, 2020 • 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!

dugenkui03 Jun 19, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

dugenkui03 commented Sep 13, 2020

Uh oh!

tfij commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dugenkui03 commented Jun 13, 2020 •

edited

Loading

dugenkui03 Jun 19, 2020 •

edited

Loading

dugenkui03 Jun 19, 2020 •

edited

Loading

dugenkui03 Jun 19, 2020 •

edited

Loading