absinthe-graphql / absinthe

The GraphQL toolkit for Elixir
http://absinthe-graphql.org
Other
4.28k stars 524 forks source link

Resolvers do not receive enough AST Context to resolve Fragments #122

Closed dpehrson closed 7 years ago

dpehrson commented 8 years ago

Note: I have discussed this issue with @benwilson512 on IRC

First-off, absinthe is fantastic, one of the coolest pieces of tech I've had the pleasure of working with in a long time, so thanks for all the hard work.

I have wired up my schema that for now consists of a simple model: User has_many Packs and a Pack belongs_to User which represents the different Backpack configurations a user might take on various hiking/camping trips.

I implemented a dynamic resolution system that walks the incoming AST and recursively figures out the preloads needed to resolve the incoming GraphQL query to an Ecto query.

This worked great until I wired it up to Relay which quickly introduced Query Fragments. Unfortunately the AST object passed to resolvers is scoped to the node being resolved, so while the it will include a FragmentSpread letting me know that I need to look at "F1" to continue processing the resolution, there's no way to actually get access to the AST node that represents "F1" and see what fields are in it.

Some potential solutions I see:

  1. Inline Fragments into the Node AST. Pro: Overall structure doesn't change. Con: Lots of duplicated data.
  2. Pass a third argument to resolvers which is a Map of Fragments keyed on the Fragment Name. Pro: The map could be shared globally by all involved resolvers, easy to process a FragmentSpread in a node AST and quickly look up it's name in the Map. Con: Changes the signature of resolvers.
  3. Provide a helper function that would return a Fragment AST given it's name. Pro: No backwards-comparability issues. Con: Might not even be possible? That context would need to be available somewhere.
bruce commented 8 years ago

@dpehrson Thanks for the kind words about Absinthe!

I feel your pain on this issue. @benwilson512 and I have been discussing this recently. Some brief comments, off the cuff:

My feeling on this is that your first recommendation is the closest to what we want to achieve. The duplicated data isn't much of a concern, given some other benefits. I'd argue instead of "inlining" into an AST, we create a new tree type that models the final structure to run through execution. This has the benefit of also significantly simplifying execution. Incidently, this approach is fairly close to Sangria's concept of projection, which has been on our radar for awhile.

I'll start looking at this approach this weekend.

dpehrson commented 8 years ago

@bruce Thanks for the quick reply and openness to digging into a solution. I am really excited about the possibilities with Elixir/Phoenix/React/GraphQL and I'm pretty sure if I can get Fragments resolved, my path will be cleared to start building the app I'm going for. I originally sat down to try and resolve this on my own but quickly realized I was in over my head which is why I reached back out to Ben on IRC.

Please let me know if there is any other information/help I can provide such as more detail about my use cases, etc.

The similarities between the high-level design of GraphQL queries/mutations vs Ecto queries/changesets seems like such a great fit, I'm curious to see how transparent we can make the resolutions.

This could be an immensely powerful pairing of technologies for the future of web APIs/apps.

dpehrson commented 8 years ago

As a followup, after spending the rest of the day digging further in, I came across something we might be able to leverage.

In dealing with how to respond to root-level node queries that relay requires, I ran across this query syntax:

query {
  node(id: "UGFjazo0OQ==") {
    id,
    ... on Pack {
      id,
      name,
      user {
        id,
        ... on User {
          username
        }
      }
    }
  }
}

When executed my abstract resolver hits upon Absinthe.Language.InlineFragment containing the fragment spread. which can be handled by simply peeling off it's selection_set and recursing further.

If you're already thinking about going route number 1 and flattening named fragments, a completely transparent way to do so would be to simply replace all Absinthe.Language.FragmentSpread objects with Absinthe.Language.InlineFragment objects taken from the root query.

Not sure how feasible this is for all use cases, but it seems like it would work for mine.

bruce commented 8 years ago

@dpehrson Just a note to let you know we haven't forgotten about this. We're working on an "intermediate representation" approach in #124 that involves flattening fragments and will help us break validation and execution into more flexible, discrete pieces, while giving us an avenue to do some long-overdue cleanup on some of the gnarlier pieces of logic, especially in execution.

Not only will this provide a more consistent data structure for this "hack" approach to use, but it should give us the bits we need to build much better approaches to replace it.

dpehrson commented 8 years ago

@bruce Thanks so much for the followup, I'll be watching both of these issues for progress. Again, let me know if there's any way I can help. Starting next week I will be working full time on my startup @trailpost which is what I am hoping to use absinthe to power.

bruce commented 8 years ago

(Adding this to the v1.2 milestone as a reminder. The resolution via flattening is going well, and we need to make sure the subselection fields are provided to resolvers in our new Info struct -- preferably filtered for the appropriate type condition or supporting projection.)

bruce commented 7 years ago

Changes in v1.2 have changed the landscape significantly with this issue:

dpehrson commented 7 years ago

:heart: you guys, I just started working on my system again yesterday, and now I get this notification, what rad timing.