POC: allow users to get requested fields from context

[nicksrandall]

This POC could solve #17

This PR gives resolvers access to get the list of field names that have been requested for the resolver of that type and any sub-types recursively.

Example

Assume a user executes the following query

{
  character(id: "1000") {
    id
    ...comparisonFields
  }
}

fragment comparisonFields on Character {
  name
  appearsIn
  friends {
    name
  }
}

The resolver (optionally) has access to the fields requested like so:

func (r *Resolver) Character(ctx context.Context, args *struct{ ID graphql.ID }) *characterResolver {
	fields := graphql.GetFieldsFromContext(ctx)
	log.Print(fields) // => logs: exec.FieldList{"id":exec.FieldList{}, "name":exec.FieldList{}, "appearsIn":exec.FieldList{}, "friends":exec.FieldList{"name":exec.FieldList{}}}
	// ... implementation
}

The field information is useful because now I can potentially optimize my data fetching mechanism based on what was requested in the query (like avoiding an expensive SQL join).

@neelance Thoughts?

@tj would this solve your issue?

[nicksrandall]

@jargv We've discussed something like this before, what do you think of this implementation?

[dmitshur]

This identifier name does not follow Go style. See https://github.com/golang/go/wiki/CodeReviewComments#initialisms.

[nicksrandall]

One question I have is what should the type be of the list of fields. I started by making it a []string with dot-notation for recursive fields.

However, to get constant lookup times, I changes it to a recursive map.

type FieldList map[string]FieldList

Do you think that is the right approach?

[tonyghita]

We already have something like this structure in the introspection package. Maybe we could leverage that?

[nicksrandall]

@tonyghita is there a specific part of the introspection that you think I could integrate with? From what I can tell, it is mostly for introspecting schemas and not (necessarily) a query.

[tonyghita]

Hmm you're right, but the query does have to match the schema for it to be valid, right? I'll think about this a bit more and get back to you tomorrow.

[nicksrandall]

Is it worth it for me to refactor this PR to resolve conflicts or should I abandon it? @neelance if we got this right, would you merge?

[neelance]

Sorry, but I'm not fully convinced about this yet. Please give me some more time, I haven't forgotten about this.

[theduke]

@neelance @nicksrandall I would really need this feature.

It's quite essential for properly optimizing DB queries.

[arnos]

Hey I agree @nicksrandall and @tj that the intent could solve this but I feel it's at the wrong level.

We've been using neelance/graphql for the last 3 mo (thanks guys) to build our MVP. One element we did do is adapt Uncle Bob's clean architecture.

The major change it made us do is split business logic away from the resolvers (resolvers are in the adapter layer but are mostly stubs that relay the call to the business logic functions).

The aggregation of multiple results is already taken care by graphql package, adding custom logic there to get types and sub-types recursively (IMHO) to avoid complex joins seems counter-intuitive.

An alternate approach to #17 n+1 issue I'm looking currently into is looking into channels to act as js promises and aggregate calls to the same type into a single DB query vs multiple (aka debounce the query calls or Facebooks' dataloader).

[tj]

The problem right now is that some queries are basically impossible with this package as-is, you can't possibly fetch/process all the data, in all ways, just to filter on it later. You have to construct a query based on what the user asks for. That said it's not like it has to go into this package, easy enough to fork of course :D, just a different use-case than the typical fetch/filter.

[tonyghita]

I think this is a problem that dataloader can solve. Have you tried adapting https://github.com/nicksrandall/dataloader for your usecase?

[tj]

@tonyghita I'll check that out, I already hacked the field stuff into a fork. If it's not something the maintainers would use or want that's no biggie, just mentioning the context.

[tonyghita]

@nicksrandall what do you think of #169 compared to these changes? I know it's been a while unfortunately.

[nicksrandall]

I think that we can do better than my implementation here. As I recall, this was a quick and dirty proof of concept.

The JavaScript implementation of Graphql has a "context" argument that gets passed to every resolver. One of the fields in that context is an AST of the current query which can be introspected to get a list of all the requested fields.

I think the idiomatic go way to provide a list of the currently selected fields would be to leverage the context variable that we already have. That way we're not adding yet another argument for every feature we'd like to provide the resolvers.

It would be nice if the implementation we come up with could lazily evaluate the requested fields so that resolvers that don't ask for it don't have to pay the price. Once evaluated, we could store the result in the context so that downstream resolvers have less work to do.

Then again, calculating the requested fields might be so trivial that such optimizations would be unnecessary.