Skip to content

Fix type matching for arguments wrapped with non null.#19

Closed
Hellblazer wants to merge 1 commit into
graphql-java:masterfrom
ChiralBehaviors:fix-non-null-wrapped-input-type-match
Closed

Fix type matching for arguments wrapped with non null.#19
Hellblazer wants to merge 1 commit into
graphql-java:masterfrom
ChiralBehaviors:fix-non-null-wrapped-input-type-match

Conversation

@Hellblazer
Copy link
Copy Markdown
Contributor

If an input argument typed is wrapped with non null, it won't match
input variables defined by the query. This patch adds matching on the
unwrapped argument type.

If an input argument typed is wrapped with non null, it won't match
input variables defined by the query.  This patch adds matching on the
unwrapped argument type.
@andimarek
Copy link
Copy Markdown
Member

Could you give an example, which use-case this would solve? Thanks!

@Hellblazer
Copy link
Copy Markdown
Contributor Author

Sure. I apologize for no tests, but groovy - while something I do know about - isn't anything I am fluent enough in to produce tests. And gradle.... so sorry about that.

My test case is a query of the form

query m($id: String) { myQuery { id: $id } }

where id is constrained to be non null string.

What happens is that when the input is a variable, not explicit in the query text, the expected type is non null wrapped and the actual type is not. The way the code was written, this will return false without ever checking if the wrapped expected type matches the actual type.

@andimarek
Copy link
Copy Markdown
Member

I'm sorry, but I think the implementation is correct: When id is notNull and you declare a variable as Nullable, the query is invalid. Because you could assign null to $id and then it would crash when executing the query.

Can't you declare the variable as NonNull: m($id: String!) ?

@Hellblazer
Copy link
Copy Markdown
Contributor Author

Sorry, somehow missed this notification for the comment.

I must not have been clear. The input argument to the query is declared as NonNull. The problem with the current logic is that when I have a input argument declared as NonNull, I will get a NPE when I supply the input argument as a query variable. The NPE does not occur if this input argument is supplied in the text of the query.

For example:

query m($id: String) { myQuery { id: $id } }

The query variable "id" is not null and has value "foo". This causes a NPE at the point in the code where I fixed this.

query m { myQuery { id: "foo" } }

Will not throw an NPE.

If I was able to create a groovy test case for this, it's pretty easy to verify. Unfortunately, groovy is pretty far out of band for me. However, the test case is very simple. Create a query that has an NonNull input argument. Supply the value of that input in the variable map you pass when executing the query. NPE will happen.

I certainly understand this may in fact not the way to fix this. But this NPE pretty much prevents any use of NonNull input args when supplied by variables to the query.

@andimarek
Copy link
Copy Markdown
Member

I'm sorry that this has such a negative effect.

I looked at it again and I think your root problem seems to be that the whole query is invalid. I didn't see this before and I fixed just recently a problem where an invalid query didn't raise any error.

The problem is the { id: $id }. You can't specify a dynamic field in graphql. So if you specify an alias for an field, the field must be a constant. For example id: myField.

@Hellblazer
Copy link
Copy Markdown
Contributor Author

Well, this is what I get for not writing a test ;) So, I have corrected this. You can find the gist here.

Apologies for this. This is a completely different issue and I was confusing it with the NPE bug you already merged.

This problem is validating types and when NonNull argument, supplying argument values fails due to not navigating to the wrapped type.

@andimarek
Copy link
Copy Markdown
Member

No problem.

I took your gist and changed the query argument (in the failing section) to Non-Null: ... m(\$id : String!) ... Now it works.

And like I said, I'm sorry, but I think this is correct:

"I'm sorry, but I think the implementation is correct: When id is notNull and you declare a variable as Nullable, the query is invalid. Because you could assign null to $id and then it would crash when executing the query."

Is this helpful/possible for you to change the variable type?

@Hellblazer
Copy link
Copy Markdown
Contributor Author

omg. Yes, you're absolutely correct. I completely spaced about the !.

Well, thanks for the patience ;) Much appreciated.

@Hellblazer Hellblazer closed this Sep 18, 2015
estal011 pushed a commit to 8btc-OnePiece/graphql-java that referenced this pull request Oct 21, 2022
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.

2 participants