Validation errors and @NonNull return types, in combination with @Source

[dpolysiou]

I'm facing an issue similar to 1921, but concerning methods annotated with @Source. Suppose we have a snippet similar to the following:

@Query
public Parent getParent(...) {
    ...
}

public @NonNull Child getChild(@Source Parent parent, @Valid Input input) {
    ...
}

In case the input is invalid, the API returns, in addition to the expected validation errors, an error concerning the nullability of the field:

{
  "message": "The field at path '/parent/child' was declared as a non null type, but the code involved in retrieving data has wrongly returned a null value.  The graphql specification requires that the parent field be set to null, or if that is non nullable that it bubble up null to its parent and so on. The non-nullable type is 'Child!' within parent type 'Parent'",
  "path": [
    "parent",
    "child"
  ],
  "extensions": {
    "classification": "NullValueInNonNullableField"
  }
}

According to the error message, it seems that there is a problem with the process of bubbling-up null from the child field to the parent, until a nullable field is found. I'm not sure if this is something I should somehow handle in my own methods, or if it should be handled by smallrye-graphql.

I have uploaded a reproducer (using Quarkus version 3.14.4).

[t1]

Wow, that's an interesting edge case! The child field is declared to be non-null, but because of the validation error (could probably also be an error thrown somewhere in the code), it's null. I'm not sure, if it would be okay to set it to null and suppress that additional error. I think this is what happens at the root query, too.

[jmartisk]

The spec (https://spec.graphql.org/draft/#sel-FANTNJBABJyxE):

If the result of resolving a field is null (either because the function to resolve the field returned null or because a field error was raised), and that field is of a Non-Null type, then a field error is raised. The error must be added to the "errors" list in the response.

I think this means that the behavior of SmallRye GraphQL is correct according to the spec, isn't it?

[dpolysiou]

That part is according to the spec, but what about the following two paragraphs:

Additional errors

If the field returns null because of a field error which has already been added to the "errors" list in the response, the "errors" list must not be further affected. That is, only one error should be added to the errors list per field.

In this case, a field error has already been added to the errors list (the bean validation error), so the NullValueInNonNullableField is superfluous.

Propagation of null

Since Non-Null type fields cannot be null, field errors are propagated to be handled by the parent field. If the parent field may be null then it resolves to null, otherwise if it is a Non-Null type, the field error is further propagated to its parent field.

I'm not sure if this propagation is handled correctly. The end result certainly looks like what I would expect, i.e. {"data": {"parent": null}, errors: [...]}. However, the additional error message seems to disagree.

I suppose that if the additional error was suppressed, everything would be fine.

[jmartisk]

Hmmm... It may be argued that the two errors do not belong to the same field, the nullability error a path ["parent", "child"] and the validation error has ["child", "input"] (btw. shouldn't this rather be ["parent","child","input"]???), because it belongs to the particular field of the Input type that was invalid.

[jmartisk]

You are right it seems a bit odd that the nullability error is there. It's added by graphql-java though, and I would guess it's exactly because the paths of the two errors are different, so that graphql-java decides to append this error after all. On the SmallRye side I think we don't have control over this

[dpolysiou]

It may also be argued that the input is a "field argument", not a "field", and therefore any field errors belong to the field, not the argument. I'm trying to find some clarification in the spec, but I'm having a hard time. A relevant part is found in the coercion of field arguments*:

i. Otherwise if argumentType is a Non-Nullable type, and either hasValue is not true or value is null, raise a field error.

I might be reading this wrong, but to me it suggests that this field error belongs to the field, not the field argument.

[dpolysiou]

I experimented a little bit with the path constructed in BeanValidationError. This is an implementation of a method found in the GraphQLError interface, which states:

The graphql spec says that the (optional) path field of any error must be a list of path entries starting at the root of the response and ending with the field associated with the error https://spec.graphql.org/draft/#sec-Errors.Error-Result-Format

Indeed, the spec says:

If an error can be associated to a particular field in the GraphQL result, it must contain an entry with the key path that details the path of the response field which experienced the error. This allows clients to identify whether a null result is intentional or caused by a runtime error. (emphasis mine)

So, arguably, this is a (minor) spec-compliance issue.

If I use the ResultPath, as expected by GraphQLError, the validation error's path becomes ["parent", "child"] and the nullability error goes away. If I additionally append "input", the error re-appears.

[jmartisk]

Ok, so you think the problem is that we generate a wrong path for the error and it should be ["parent", "child"]? Then yeah, we could probably fix that (PRs are welcome!)

[dpolysiou]

I'd be happy to provide a PR. However, one issue that needs discussion is what to do with all the constraint violations. Right now, an error is created for each violation, and since each error has a different path, they all show up in the response. If I change the path, only a single violation will show up (since they would all have the same path).

One option would be to gather all violations in a single field error. So, there would be a single error with path ["parent", "child"] and the individual violations could be included in the error extensions, as a violations array. Does that sound like a good idea?

[jmartisk]

Yeah but we should make sure we don't just 'drop' the information about which argument was invalid, we still should convey that somewhere, if it's no longer going to be in the path.

So, I could imagine that all validation errors for the same field would be merged together into a single io.smallrye.graphql.validation.BeanValidationError, and its extensions would contain an entry named violations with an array where each violation still has the subentries message, propertyPath, invalidValue and constraint like now... The propertyPath, I think, contains the name of the argument, if the violation was related to an argument, so that would cover the requirement to convey the name of the argument.

Does this make sense?

[dpolysiou]

Makes perfect sense. Additionally, there is a list of source locations pointing to the errors (eg. line X, column Y). I guess this list should contain the locations of all arguments that had violations.

[jmartisk]

Yes. It won't contain the explicit mapping between locations and particular constraint violations, but following the required response format, I guess it's the best we can do

[jmartisk]

Fixed via #2201