Validation - more data in errors

[krider2010]

This work addresses #1962 (for 1.9 and onwards). It also enhances the messages returned from static validation to contain the rule applied and some data. This is duping some of the information in the message string intended to help developers/app users. This is intentional. It is done so that this data hash can easily be examined/stored in analytics services or logging without the need to try and break apart the message string.

Note that the only reliable key in this extensions hash is that of rule. Others are provided on an as-needed basis. The idea is that these keys are rule specific. One of the things I'd like to see in this review is if what each rule populates is adequate and also adequately named. I'm not in favour of documenting all the possible keys as they could change, but to set the expectation that the GraphQL spec itself has:

GraphQL services may provide an additional entry to errors with key extensions. This entry, if set, must have a map as its value. This entry is reserved for implementors to add additional information to errors however they see fit, and there are no additional restrictions on its contents.

The solution of using this hash is also to be consistent with the specification. Including in the latest (at the time of this work and PR) the following comment:

Note: Previous versions of this spec did not describe the extensions entry for error formatting. While non-specified entries are not violations, they are still discouraged.

Commits have been separated between correcting fields to path, the extensions hash changes, and then the test fixes for adding the extra hash. It is hoped that this makes it a bit easier to review.

[cjoudrey]

add_error("Field '#{node.name}' doesn't exist on type '#{parent_type.name}'", node, extensions: {
  "rule": "StaticValidation::FieldsAreDefinedOnType",
  "field": node.name,
  "type": parent_type.name
})

I don't think this is necessarily wrong, but I'm not sure about the rule part. I fear that we are leaking implementation details to the outside world by naming it after the Ruby class.

Have you considered something along the lines of:

class StaticValidation::FieldNotDefinedError < StaticValidation::Error
  attr_reader :field_name
  attr_reader :type_name

  def initialize(field_name:, type_name:)
    # ...
  end

  def code
    "fieldNotDefined"
  end

  def to_h
    # add `fieldName` and `typeName` to hash
    # something like: super.merge({ ... })
  end
end

Then when serializing the error it would become:

{
  "message": "Field '#{node.name}' doesn't exist on type '#{parent_type.name}'",
  "extensions": {
    "code": "fieldNotDefined",
    "fieldName": "...",
    "typeName": "..."
  }
}

This would enable people to distinguish errors both in Ruby-land and in GraphQL-land.

Moreover, I think it's unfortunate that the spec and reference implementation do not define anything besides a message for these errors, but Sangria (the Scala GraphQL library) started attributing codes to these errors in order to make compliance testing with graphql-cats easier. I wonder if we should follow Sangria and use the same codes and args as it uses.

[krider2010]

I did indeed consider the solution of subclasses but it basically results in 30 classes with effectively just a to_h in them. Which seemed to add confusion to the codebase for me. Indirection taken too far is a hindrance to code readability. That said I’m probably scarred by architects early in my career who insisted on factories of factories of factories.

I totally agree the naming should be improved. This was a first pass to get information on what people would like to see there. I also see value in keeping static validation in there for now (in the rule) because it allows higher level pattern matching to be applied in analytics/error logs.

[xuorig]

Interesting. I really like @cjoudrey's suggestion of adopting some convention for the rule name, sangria might be a good choice here 👍. Since this is returned to clients that don't actually care if the query was resolved by graphql-ruby or another library, I think it's important to stay away from returning the class name here.

I don't have a super strong opinion about classes vs the hashes, but I generally like the class approach a little better I think. The goal is for graphql-ruby users to also be able to act on these errors and it seems a bit less fun to use hashes and try to match strings vs having objects representing the possible errors.

Having classes for these errors would help us document them a bit better, and make them easier to use for integrators. My initial goal was to be able to do something like:

fields_not_found = validation_error.select { |error| error.is_a?(FieldNotFoundError) }
field_names = fields_not_found.map(&:field)
# Do something ...

I think it would be a bit more awkward to see what data is available in the different hashes depending on the rule 🤔 I do get where you're coming from though, but if we collocate the errors with the validation rules, they would be easy to find, and the fact that they're actual class instances would make them easier to use I think.

WDYT @krider2010 ?

[krider2010]

I don't have a super strong opinion about classes vs the hashes, but I generally like the class approach a little better I think. The goal is for graphql-ruby users to also be able to act on these errors and it seems a bit less fun to use hashes and try to match strings vs having objects representing the possible errors.

@xuorig I don’t think I understand this. Because to adhere to the GraphQL spec, we must return a hash named extensions in the error response. Maybe I’m missing how this will be used? In which case could I have an idiots guide? 🙏

Edit
Hmmm... Are you saying within ruby code we can ignore the serialised version? In which case I do totally get the need for (sub)classes. I was struggling with it all just being cut back to JSON hence found it overkill.

[cjoudrey]

Are you saying within ruby code we can ignore the serialised version?

There's two ways to consume GraphQL errors.

One way is to turn the response into a hash which can then be serialized to JSON. This is what gets sent over the wire to clients.

For example:

require "graphql"

class Query < GraphQL::Schema::Object
  field :hello, String, null: true

  def hello
  end
end

class Schema < GraphQL::Schema
  query Query
end

puts Schema.execute("{ something }").to_h

# {"errors"=>[{"message"=>"Field 'something' doesn't exist on type 'Query'", "locations"=>[{"line"=>1, "column"=>3}], "fields"=>["query", "something"]}]}

Another way is to obtain the Ruby objects by accessing the validation_errors method on the Query object:

require "graphql"

class Query < GraphQL::Schema::Object
  field :hello, String, null: true

  def hello
  end
end

class Schema < GraphQL::Schema
  query Query
end

errors = Schema.execute("{ something }").query.validation_errors
puts errors.first

# #<GraphQL::StaticValidation::Message:0x00007fe3d83d8f10>

Notice how the error is an instance of GraphQL::StaticValidation::Message. All static validation errors are an instance of that class. This isn't great because it means we cannot easily do type comparisons on them as shown in #1970 (comment).

Without having classes for each type of possible static validation error, we would have to turn the instance into a hash and then dig into the extensions key, i.e:

errors = Schema.execute("{ something }").query.validation_errors
errors.select { |error| error.to_h["extensions"]["code"] == "FieldNotFound" }

We also lose out on some advantages like adding structure to what the validation error contains. For example, the FieldNotFound error class could have attr_reader :field_name, :type_name.

I believe there are uses for both cases, that's why I'm advocating for us using Ruby classes to represent each static validation error AND also output this information in the extensions key following Sangria as an example for the error codes and what to include.

I hope this clears things up for you.

[krider2010]

@rmosolgo @xuorig @cjoudrey here is the rubified version, with a slimmed down add_error to avoid some nasty metaprogramming.

For reference for anyone discovering this later, and/or wanting a summary, here are the codes used in the error extensions hash.

Class	code
ArgumentLiteralsAreCompatibleError	argumentLiteralsIncompatible
ArgumentNamesAreUnique	argumentNotUnique
ArgumentsAreDefined	argumentNotAccepted
DirectivesAreDefined	undefinedDirective
DirectivesAreInValidLocations	directiveCannotBeApplied
FieldsAreDefinedOnType	undefinedField
FieldsHaveAppropriateSelections	selectionMismatch
FieldsWillMerge	fieldConflict
FragmentNamesAreUnique	fragmentNotUnique
FragmentSpreadsArePossible	cannotSpreadFragment
FragmentTypesExist	undefinedType
FragmentsAreFinite	infiniteLoop
FragmentsAreNamed	anonymousFragment
FragmentsAreOnCompositeTypes	fragmentOnNonCompositeType
FragmentsAreUsed	useAndDefineFragment
MutationRootExists	missingMutationConfiguration
NoDefinitionsArePresent	queryContainsSchemeDefinitions
OperationNamesAreValid	uniquelyNamedOperations
RequiredArgumentsArePresent	missingRequiredArguments
SubscriptionRootExists	missingSubscriptionConfiguration
UniqueDirectivesPerLocation	directiveNotUniqueForLocation
VariableDefaultValuesAreCorrectlyTyped	defaultValueInvalidOnNonNullVariable, defaultValueInvalidType
VariableNamesAreUnique	variableNotUnique
VariableUsagesAreAllowed	variableMismatch
VariablesAreInputTypes	variableRequiresValidType
VariablesAreUsedAndDefined	variableNotUsed, variableNotDeclared

[rmosolgo]

I think it turned out great! I'm looking forward to seeing how we can leverage the flexibility from the new specialized classes. @xuorig, @cjoudrey anything else?

[cjoudrey]

This seems like a step in the right direction. I'm happy to see us add structure to static validation errors. Good work! 👍 😄

I'm sure we can argue about the error codes, but I think they are good enough. 😄

[xuorig]

This is great 👏 Thanks @krider2010