This condition is different for SDL-based vs programmatic:

if (schemaDefault.isPresent() && schemaDefault.get().isLiteral()) { //true for SDL
    schemaDefaultValue = (Value) schemaDefault.get().getValue();
} else if (schemaDefault.isPresent() && schemaDefault.get().isSet()) { //valueToLiteral explodes for programmatic 
    schemaDefaultValue = ValuesResolver.valueToLiteral(schemaDefault.get(), expectedType, getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale());
}

The interesting part is that an object {limit: 10, offset: 0} is (wrongly) deduced as the default value for the variable in both cases. But the problem manifests differently in SDL-based vs programmatic. Since the default value here is merely used to decide nullness, and for SDL-based the value is taken as-is, the only issue is potentially wrong nullness. For the programmatic case though the default value first has to be converted, and this is what explodes, as the types don't match.

But the root cause in all cases is that the query tree isn't properly traversed apparently, and the argument and its default value are in the context when a variable is being validated. Seems like the traverser doesn't descent deeper into the input object itself in this case.

But the root cause in all cases is that the query tree isn't properly traversed apparently, and the argument and its default value are in the context when a variable is being validated. Seems like the traverser doesn't descent deeper into the input object itself in this case.

Actually I suspect i was a copy and paste bug. When we call ValuesResolver.valueToLiteral on the argument default value we need to pass in the argument type - but we didnt - we passed in the variable reference type. Which is plain wrong

Bah, I hate to be harping on this but... this method seems to be using the default value of the whole input object while claiming to be using the default value of the input object field where the variable is used instead:

// variableType is Int, variableDefaultValue is object??
public boolean doesVariableTypesMatch(GraphQLType variableType, Value variableDefaultValue, GraphQLType expectedType) {
    return checkType(effectiveType(variableType, variableDefaultValue), expectedType);
}

Is that... surely ok? It feels like it's comparing apples to oranges 😐

E.g. if I were to add the default values to limit and offset:

type Query {
    items(pagination: Pagination = {limit: 10, offset: 0}): [String]
}
input Pagination {
    limit: Int = 10    #now has a default
    offset: Int = 0
}

and query:

query Items( $limit: Int, $offset: Int) {
    items(
      pagination: {limit: $limit, offset: $offset}
    )
}

shouldn't variableDefaultValue from above be 10 and not {limit: 10, offset: 0}?

Sorry if I'm confused and just pestering you at this point 😳

I think I managed to break the nullness deduction (exploiting the thing I explained above):

Schema:

type Query {
    items(pagination: Pagination = {limit: 1, offset: 1}): [String]
}
input Pagination {
    limit: Int! #non-null this time, no default value
    offset: Int! #non-null this time, no default value
}

Query (same as before):

query Items( $limit: Int, $offset: Int) {
  items(
    pagination: {limit: $limit, offset: $offset} 
  )
}

Variables:

Map<String, Object> vars = new HashMap<>();
vars.put("limit", 5);
vars.put("offset", null); //explicit null for a non-null input field

This validates (and should't)!

Calling env.getArguments() inside the items fetcher throws:

AssertException: Internal error: should never happen: Should have been validated before

I will look into this new bug condition when I can...

Seem 309fb9e1747f05ce0f3c52c6aa92570a11144dab is when this code was last changed. It was a bug fix around nullness

I think we need to pull on that string to see its correct or not - it feels wrong based on the above. I have created a repro and it does blow up as stated

So having looked into this more it seems that the fix created to adress #2573 is not correct enough,.

As @kaqqao has said it does not cover cases where the variable reference is down in part of the input type. It only covers the case where a $variable reference is the whole input type.

I think this fix needs to be reconsidered from first principles. eg a variable reference can point to part of an arguments structure and the null / non null overrride rules from the spec need to somehow descend into the structure OR be checked in some other manner

I am reading the spec here now

A notable exception to typical variable type compatibility is allowing a variable definition with a nullable type to be provided to a non‐null location as long as either that variable or that location provides a default value.

One line of thought is that we don't have to check that innards of the default value are ok but rather that the argument has a default value at all.

HOWEVER - I took the graphql-js reference implementation and added a new test to it using our examples

in harness.ts

  input Pagination {
      limit: Int! #non-null this time, no default value
      offset: Int! #non-null this time, no default value
  }

  type QueryRoot {
    items(pagination: Pagination = {limit: 1, offset: 1}): [String]

and src/validation/__tests__/VariablesInAllowedPositionRule-test.ts

  it('Pagination => Pagination in field position', () => {
    expectValid(`
      query Items( $limit: Int, $offset: Int) {
                 items(
                    pagination: {limit: $limit, offset: $offset} 
                )
      }
    `);
  });

and this shows the query as invalid.

[
  {
    "locations": [
      {
        "column": 20
        "line": 2
      }
      {
        "column": 41
        "line": 4
      }
    ]
    "message": "Variable \"$limit\" of type \"Int\" used in position expecting type \"Int!\"."
  }
  {
    "locations": [
      {
        "column": 33
        "line": 2
      }
      {
        "column": 57
        "line": 4
      }
    ]
    "message": "Variable \"$offset\" of type \"Int\" used in position expecting type \"Int!\"."
  }
]

So I think we need to go back and deeply read the spec here because my initial thought is that the defaulting on the argument above would allow it to proceed BUT the reference implementation does not allow it

Are we reading the spec wrong OR has the reference implementation got the same bug as we do?

Hmmm more investigation needed

FWIW, graphql-js behavior looks unambiguously correct to me, spec-wise.

allowing a variable definition with a nullable type to be provided to a non‐null location as long as either that variable or that location provides a default value.

limit and offset fields (locations) are both non-null and without defaults in the schema, and the corresponding variable definitions are also without defaults yet nullable. That is definitely not valid according to the text above, so the types must match (and don't, so the validation fails).

graphql-java's behavior is very different. It instead validated the query, because it looked at the default value of pagination, which should not have come into the picture here at all, since the value for pagination argument was explicitly provided by the client. Me supplying null for offset was then merely exploiting this validation bug in order to sneak an invalid value in.

One line of thought is that we don't have to check that innards of the default value are ok but rather that the argument has a default value at all.

True, but it's the location where the variable is used that matters, and the locations in question (limit and offset) have no default value! The pagination default value is merely there to trip graphql-java into comparing wrong things. In graphql-js, it doesn't serve any purpose in this case (it's a default value overridden by an explicit value and thus ignored completely).

we don't have to check that innards of the default value

Not at query execution time, no. I reckon that's already done once during schema-building, and that's all that's needed. The spec text above deals with query execution time only.

True, but the locations in question (limit and offset) have no default value! The pagination default value is merely there to trip graphql-java into comparing wrong things. In graphql-js, it doesn't serve any purpose in this case (it's a default value overridden by an explicit value and thus ignored completely).

limit and offset fields (locations) are both non-null and without defaults in the schema, and the corresponding variable definitions are also without defaults yet nullable.

Yes I have been rereading the spec and the reference implementation and I think I have come to the same conclusion. The example IS invalid based on spec even if it could be argued that code COULD somehow work out how to default it (eg some how look upwards to the argument defaults and smoosh the values in).

We do indeed still have a bug in graphql-java that allows this to be considered valid and then fail later BUT I was trying to work out the desired validation behavior.

@kaqqao - I have implemented a fix for this problem. I went back to the graphql-js implementation for inspiration and to check expected behavior

If you can please have a look this will help us.

Not at query execution time, no. I reckon that's already done once during schema-building, and that's all that's needed.

I think this is an improvement we could add - eg at schema build time we validate that the default values are in fact valid for that location

Hi!
Thank you both for looking into this issue.
Just wanted to add that I noticed this bug when trying to upgrade from v16.2 which was released Feb 2, 2021. On v16.2 the query worked fine.

So the commit @bbakerman linked, which was merged Dec 27, 2021, definitely sounds like it could have introduced this issue. I assume it was released with the v18.0 release Mar 15, 2022.

@kaqqao - I have implemented a fix for this problem. I went back to the graphql-js implementation for inspiration and to check expected behavior

If you can please have a look this will help us.

This now looks right to my eyes, and I confirm that I can no longer break it 😄 Thanks a bunch for this!