-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add filter types to schema #21
Conversation
|
||
GraphQLFieldDefinition fieldDefinition = objectType.getFieldDefinition("customers"); | ||
Assertions.assertEquals("page",fieldDefinition.getArguments().get(0).getName()); | ||
Assertions.assertEquals("filter", fieldDefinition.getArguments().get(1).getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you expand the test to show to full filter and its arguments
.field(GraphQLInputObjectField.newInputObjectField().name("offset").type(Scalars.GraphQLInt)); | ||
} | ||
|
||
public static GraphQLInputObjectType.Builder filterInputBuilder() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this some form of sandbox. It contains business model where it should be generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the schema we read from the Database we are applying the where filters and page limits apply automatically.
@@ -61,9 +53,19 @@ public GraphQLSchema buildSchema(Schema schema) { | |||
GraphQLObjectType.Builder queryTypeBuilder = GraphQLObjectType.newObject(); | |||
queryTypeBuilder.name("QueryType"); | |||
|
|||
//add filters for queries | |||
builder.additionalType(Filters.pageInputBuilder().build()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I think it will be amazing to use https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/schema/idl/SchemaPrinter.java
to print schema for us to verify this under some different model use cases (we can use snapshot testing for this or anything else to validate correctness and call it a day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are printing the schema using SchemaPrinter
, once you run you can look for the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this now! thank you
Amazing work! Really great use case how easy is to extend the GraphQLJava. |
return GraphQLInputObjectType.newInputObject().name("QueryFilter") | ||
.field(GraphQLInputObjectField.newInputObjectField().name("id").type(GraphQLTypeReference.typeRef("IDInput"))) | ||
.field(GraphQLInputObjectField.newInputObjectField().name("title").type(GraphQLTypeReference.typeRef("StringInput"))) | ||
.field(GraphQLInputObjectField.newInputObjectField().name("clickCount").type(GraphQLTypeReference.typeRef("IntInput"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now using these types add FilterInput on Customer like
CustomerFilterInput {
SSN StringInput,
ID IDInput,
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding it additionally or modifying the existing type Customer
to type CustomerFilterInput
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a separate type, see https://graphqlcrud.org/docs/next/find and example of NoteFilter
and also see at the end of the page
Assertions.assertTrue(child.toString().contains("originalType=[QueryFilter]")); | ||
}); | ||
} | ||
else if(parent.getKey().equals("FloatInput")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expand the test to each individual input type you added. Break them into different methods be more readable.
Assertions.assertTrue(fieldDefinition.getArgument("filter").getType().getChildren().toString().contains("PHONE")); | ||
Assertions.assertTrue(fieldDefinition.getArgument("filter").getType().getChildren().toString().contains("STATE")); | ||
Assertions.assertTrue(fieldDefinition.getArgument("filter").getType().getChildren().toString().contains("SSN")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant for BoolenInput, IntInput, StringInput.. here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't fetch the types here, as they are TypeReferences
to StringInput
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean write test cases for BoolenInput, IntInput, StringInput itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look here - engine/src/test/java/io/graphqlcrud/FilterInputTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, sorry I missed that some how looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@wtrocki is it good to land on master? |
All good to merge. I did not checked last commit. I will run this in the morning from master. |
okay sure! |
Adds types for pagination, ordering and filtering in the schema for #16