Pass down ast.Path of current field, argument etc.. inside validator.Events.OnField

[shotmk]

GraphQL errors have a path field that helps clients to locate the path in graph where error happened.

I am writing some custom directive implementation on top of validator.Walk and validator.Events APIs inside gqlparser.

I was surprised to find out that i do not have access to current field ast.Path inside the OnField() callback.

Looking on a walker implementation it feels like it is pretty easy to calculate path as we are doing a straightforward tree traversal.

Can ast.Path info be added to the OnField Events listener of validator.Events ?

[StevenACoffman]

@shotmk I would always be very open to a PR that helps clients to locate the path in graph where error happened, especially if it did not break backwards compatibility or cause a big performance regression.

Are you suggesting that the ast.Path would be on the walker or the Events, or on the Field or somewhere else?

It's currently only in the varValidator

[shotmk]

@StevenACoffman
I think ast.Path fits to be inside ast.Field next to Position field. (as position and path logically have the same purpose: locating the field in document / schema).

[StevenACoffman]

Sure, that makes sense. Are you able to make a PR for that? I'm swamped at work for the foreseeable future, but would like to see this happen.

[shotmk]

@StevenACoffman
I will try to find time for a PR in near future ;)

[shotmk]

@StevenACoffman
I am working on the PR and noticed a weird behavior of OnField observer.
I have a schema like this:

type Query {
 topLevelField: TopLevelField
}

type TopLevelField {
 nestedField: NestedField
}

type NestedField {
 deeplyNestedField: String
}

schema { query: Query}

and i am doing the following query:

{
 topLevelField {
   nestedField {
     ...NestedFieldFragment
   }
 }
}

fragment NestedFieldFragment on NestedField {
 deeplyNestedField
}

In my test i am doing :

called := 0
observers := &Events{}
observers.OnField(func(walker *Walker, field *ast.Field) {
  called++
})
Walk(schema, query, observers)
require.Equal(t,3, called)

Expecting the OnField to be called 3 times, but actually it is called 4 times.
This happens because how walk() is being implemented in walk.go

	for _, child := range w.Document.Operations {
		w.validatedFragmentSpreads = make(map[string]bool)
		w.walkOperation(child) 
		// AT THIS POINT OnField is called 3 times 
	}
	for _, child := range w.Document.Fragments {
		w.validatedFragmentSpreads = make(map[string]bool)
		w.walkFragment(child)
		// This additional walkFragment causes the additional OnField call
	}

Is it expected can this be changed ?
Now when i am adding field paths, i can keep track on called unique paths inside the walker and make sure to trigger OnField for each unique field path of the operation exactly once.

WDYT?

[StevenACoffman]

That might be great! I remember there were some tricky corner cases where it was better to call some observers twice to avoid missing calling other observers at all. I can't think of why we would need to call the observers twice.

• w.Observers.fragment calling v(w, it),
• w.Observers.directive calling v(w, dir),
• w.Observers.directiveList calling v(w, directives),
• w.Observers.value calling v(w, value),
• w.Observers.field calling v(w, it),
• w.Observers.inlineFragment calling v(w, it),
• w.Observers.fragmentSpread calling v(w, it),

Looking at this on my phone on a bus makes me think something else might also be incorrect.

For example, In the walk()
it looks like we should maybe not reset the validatedFragmentSpreads and instead have walk() be:

	w.validatedFragmentSpreads = make(map[string]bool)
	for _, child := range w.Document.Operations {
		w.walkOperation(child)
	}
	for _, child := range w.Document.Fragments {
		w.walkFragment(child)
	}

WDYT?

I'd need to look at the side effect differences between walkFragment and walkOperation.
I seem to recall that walkOperation would do w.walkDirectives(def, operation.Directives, loc) with loc being a ast.LocationQuery, ast.LocationMutation, or ast.LocationSubscription, but that walkFragment instead calls w.walkDirectives(def, it.Directives, ast.LocationFragmentDefinition). I forget what practical difference that makes and how the validatedFragmentSpreads interacts.

Sorry, this is my bus stop! :)

[StevenACoffman]

Hey, if you get stuck, or need to set this work aside, push up your current work as a branch somewhere and I'm happy to take a look.

[shotmk]

i had a pretty simple implementation ready, and when i was covering it with tests i noticed this fragments thingy...
I didn't have time to get back to it and do a proper investigation, but i do plan to do it at some near future.
I've pushed my progress. You can take a look here:
master...shotmk:gqlparser:add-path-to-fields

I even have a workaround in place, but wanted maybe to fix the issue in it's root by not walking fragment fields or smth..

[StevenACoffman]

Oh, now I see. Yeah, that's awkward. Ok, thanks for letting me peek! I'll let you think of a more elegant solution. If I think of anything before then I'll tell you.