Fix type matching for arguments wrapped with non null.#19
Conversation
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.
|
Could you give an example, which use-case this would solve? Thanks! |
|
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. |
|
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: |
|
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: 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. 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. |
|
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 |
|
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. |
|
No problem. I took your gist and changed the query argument (in the failing section) to Non-Null: 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? |
|
omg. Yes, you're absolutely correct. I completely spaced about the !. Well, thanks for the patience ;) Much appreciated. |
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.