Skip to content

Conversation

yarinvak
Copy link
Member

@yarinvak yarinvak commented Oct 10, 2017

fixes the bug of unsupported list of input types as an argument
fixing issue #78
fixing issue #90

@yarinvak yarinvak requested a review from guy120494 October 10, 2017 12:46

private graphql.schema.GraphQLType getOrPutInRegistry(graphql.schema.GraphQLType graphQLType) {
graphql.schema.GraphQLType typeFromRegistry = typeRegistry.get(graphQLType.getName());
if (typeFromRegistry!=null)
Copy link
Contributor

Choose a reason for hiding this comment

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

please use braces in the if and else

Stack<graphql.schema.GraphQLType> typesStack= new Stack<>();
typesStack = getWrappedTypesStack(graphQLType, typesStack);
graphql.schema.GraphQLType wrappedType = typesStack.pop();
if (wrappedType instanceof GraphQLObjectType){
Copy link
Contributor

Choose a reason for hiding this comment

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

What if I have an interface? will this still work? (I don't remember if an interface is also an objectType)

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't, but Im not sure if we should handle it the same way

@guy120494
Copy link
Contributor

@tdraier claims that he solved these problems in #94. I think it is better to code review and merge that PR because it fixes other problems as well

@yarinvak
Copy link
Member Author

He doesn't solve issue #78 entirely. This PR allows List of List of List (...) of Input Type (it handles it recursively), while his PR does not.
This PR also fixes tests that did not pass in the master branch.
And it also does not change the code drastically compared to the PR you referenced.

@tdraier
Copy link
Collaborator

tdraier commented Oct 13, 2017

Actually List of List of List of Input types work in #94 , I've just added a unit test. It also allows you to use constructor with typed fields instead of simple map, which makes possible to use more complex type ( in the unit test, an input containing a list of other objects )

collect(Collectors.toList()));
}

private GraphQLInputType handleGraphQLList(graphql.schema.GraphQLType type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better that the function get a GraphQLList as an input

@DoctorVoid
Copy link
Contributor

Do you have tests that check list of inputs?

}
}

private graphql.schema.GraphQLType getOrPutInRegistry(graphql.schema.GraphQLType graphQLType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename method

GraphQLInputObjectType inputObject = getInputObject((GraphQLObjectType) wrappedType, "input");
inputObject = (GraphQLInputObjectType) getOrPutInRegistry(inputObject);
inputType = inputObject;
while (!typesStack.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

extract to different method

@guy120494
Copy link
Contributor

We just merged @tdraier PR (#94 ), so I'm closing this PR

@guy120494 guy120494 closed this Oct 18, 2017
@yarinvak yarinvak deleted the fix-list-input-type-query branch November 1, 2017 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants