Add JSON-path to execution errors #259

josh · 2016-09-19T23:35:06Z

Implements path on error messages as seen in graphql-js.

https://github.com/graphql/graphql-js/blob/89f3f1a319bb913616f2910e8a806170155ffaf0/src/error/GraphQLError.js#L42-L48
graphql/graphql-js#396

To: @rmosolgo
CC: @kdaigle

josh · 2016-09-19T23:35:45Z

Hmm, I should probably an add array example. It should look something like ["user", "repositories", "edges", 42, "node"].

rmosolgo · 2016-09-19T23:44:08Z

👍 with a test for array indices

I'm searching for a centralized approach while rewriting StaticValidation, I wonder if I can steal this!

josh · 2016-09-20T00:14:30Z

lib/graphql/query/serial_execution/field_resolution.rb

@@ -35,6 +35,7 @@ def result
        def get_finished_value(raw_value)
          if raw_value.is_a?(GraphQL::ExecutionError)
            raw_value.ast_node = irep_node.ast_node
+            raw_value.path = irep_node.path


@rmosolgo I'm a little stumped on how to get the current collection index context here. Just want to double check its not available somehow that I'm just missing. Otherwise I can try to hack some extra state into the execution context.

😭 sadly it's totally not available.

Here's the .map where the index would come from: https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/query/serial_execution/value_resolution.rb#L48

This feature is a hard requirement for DeferredExecution and you can see the implementation here:

graphql-ruby/lib/graphql/execution/deferred_execution.rb

Lines 386 to 391 in 8e7700c

inner_frame = ExecFrame.new({

node: frame.node,

path: frame.path + [idx],

type: wrapped_type,

value: item,

})

Although ... it doesn't really apply here :S I guess we have our pick among so-so options:

mutate the irep_node for each loop in the map

copy the irep_node and give the copy a new path for each loop in the map (irep_node.dup is supported -- I think it's used for fragment inlining)

add some kind of proxy object to wrap the irep node & send it into each loop of the map

????

josh · 2016-09-20T00:14:54Z

👍 with a test for array indices

Well, added a failing test 😁

josh · 2016-09-20T18:28:38Z

@rmosolgo So I think we should actually consider reverting #198, or at least renaming that "path" key.

An array describing the JSON-path into the execution response which corresponds to this error. Only included for errors during execution.

https://github.com/graphql/graphql-js/blob/89f3f1a319bb913616f2910e8a806170155ffaf0/src/error/GraphQLError.js#L42-L48

#198 defines a path format for validation errors. However, with the semantics defined in graphql-js, this should only be used as a path into the JSON result data. Which is there none in a validation error.

josh · 2016-09-20T18:38:18Z

Don't love the collection index implementation, but it appears to work. Any suggestions for test cases that would probably break this?

rmosolgo · 2016-09-20T18:42:26Z

Ugh, are either of these covered by the spec? All I can find is:

Every error must contain an entry with the key message with a string description of the error intended for the developer as a guide to understand and correct the error.

If an error can be associated to a particular point in the requested GraphQL document, it should contain an entry with the key locations with a list of locations, where each location is a map with the keys line and column, both positive numbers starting from 1 which describe the beginning of an associated syntax element.

GraphQL servers may provide additional entries to error as they choose to produce more helpful or machine‐readable errors, however future versions of the spec may describe additional entries to errors.

It looks like everybody is trying to be "more helpful", but in conflicting ways.

There is a way that they can be disambiguated: if data is null, then the path describes a location in the document; if data is not null, the path describes a location in the response.

Anyways, I don't know that validation-error-path has any tooling built on it, so we can probably rename it safely. I added it because #160 ...

josh · 2016-09-21T22:36:00Z

Anyways, I don't know that validation-error-path has any tooling built on it, so we can probably rename it safely.

Proposed renaming in #264

rmosolgo

Looks good to me, except maybe that dup we can spare. Do you want to wait for #264 or can this be merged independently?

rmosolgo · 2016-09-22T01:31:45Z

lib/graphql/internal_representation/node.rb

@@ -72,6 +72,15 @@ def owner
        end
      end

+      def path
+        path = parent ? parent.path.dup : []


Why .dup, doesn't the method return a new array each time?

rmosolgo · 2016-09-22T01:41:22Z

lib/graphql/internal_representation/node.rb

+      def path
+        path = parent ? parent.path.dup : []
+        path << name if name
+        path << @index if @index


Just to make sure I understand, name and @index aren't mutually exclusive, right? An IRep node that represents a selection on a list type will have both name (the field name) and @index (the current iteration number)?

Correct.

In the case of the root IRep, theres no name or @index.

rmosolgo · 2016-09-22T02:09:17Z

lib/graphql/query/serial_execution/value_resolution.rb

@@ -45,7 +45,8 @@ class ListResolution < BaseResolution
          def non_null_result
            wrapped_type = field_type.of_type
            strategy_class = get_strategy_for_kind(wrapped_type.kind)
-            value.map do |item|
+            value.each_with_index.map do |item, index|
+              irep_node.index = index


Mutation is generally error-prone, but I'm open to it here, since it's a hot path and not used intensively. Maybe one good thing would be to put it back to nil when we're done, just in case? I can't think of why it would matter though. :S

An alternative implementation would be to define a delegator, something like

inner_node = IndexedNode.new(irep_node, index)

where IndexedNode is like

class IndexedNode extend Forwardable def initialize(irep_node, index) @irep_node = irep_node @index = index end def path @irep_node.path + [@index] end def_delegators :@irep_node, :name, :definitions, :directives, # ?? Lots of stuff end

I find this implementation less scary, since you never mess with the original irep_node, but it's also less efficient. I'm happy to leave it as is and refactor to something like ☝️ that if we actually have issues.

Maybe one good thing would be to put it back to nil when we're done, just in case?

Reseting index back to nil in adf0c0f

josh · 2016-09-23T20:33:49Z

Do you want to wait for #264 or can this be merged independently?

Cool. Appears to have no conflicting files either.

rmosolgo · 2016-09-23T21:57:10Z

Nice, thanks for making this happen! Looking forward to see what it can be used for :)

josh commented Sep 20, 2016

View reviewed changes

josh added 4 commits September 20, 2016 10:41

Add JSON-path to execution errors

4b59727

Add failing test for collection index

4a25240

Copy current execution index to irep_node

8a31721

Test nested collection error

cd22a4d

rmosolgo reviewed Sep 22, 2016

View reviewed changes

josh added 3 commits September 21, 2016 22:18

Remove dup

053867c

Reset index after list iteration

adf0c0f

Merge remote-tracking branch 'upstream/master' into execution-error-path

b10f40d

rmosolgo merged commit a36fead into rmosolgo:master Sep 23, 2016

josh deleted the execution-error-path branch October 31, 2016 20:37

	inner_frame = ExecFrame.new({
	node: frame.node,
	path: frame.path + [idx],
	type: wrapped_type,
	value: item,
	})

Add JSON-path to execution errors #259

Add JSON-path to execution errors #259

Uh oh!

Conversation

josh commented Sep 19, 2016

Uh oh!

josh commented Sep 19, 2016

Uh oh!

rmosolgo commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josh commented Sep 20, 2016

Uh oh!

josh commented Sep 20, 2016

Uh oh!

josh commented Sep 20, 2016

Uh oh!

rmosolgo commented Sep 20, 2016

Uh oh!

josh commented Sep 21, 2016

Uh oh!

rmosolgo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josh commented Sep 23, 2016

Uh oh!

rmosolgo commented Sep 23, 2016

Uh oh!

Uh oh!