rmosolgo / graphql-ruby

Ruby implementation of GraphQL
http://graphql-ruby.org
MIT License
5.36k stars 1.37k forks source link

Values set via context.scoped_set! not always propagating correctly to children #3899

Closed srgoldman closed 2 years ago

srgoldman commented 2 years ago

Describe the bug

Running into a weird issue with scoped context. In my type, I'm calling context.scoped_set!(key, val) . Depending on the order of the fields in my query, the scoped_context gets wiped out. to be specific:

{
  collections(handle: "fionlite-fabric-womens") {
    nodes {
      productHandles
      productGroups {
        handle
        products {
          handle
        }
      }
    }
  }

inside the def product_groups method on the collection type, I call scoped_set. if productHandles is above productGroups, then I get the correct results (the scoped_context has the expected value). If productHandles is below productGroups, then scoped_context is blank when I reach the products field in productGroup. product_handles is simply an attribute on the collection object so there's no code for it at all, just an activerecord model attribute.

I was able to workaround the issue for now by overriding the initializer and calling scoped_set! there after super but that seems a bit janky.

Versions

graphql version: 1.13.8 rails (or other framework): rails other applicable versions (graphql-batch, etc)

GraphQL schema

Include relevant types and fields (in Ruby is best, in GraphQL IDL is ok). Any custom extensions, etc?

class Collection < GraphQL::Schema::Object
  field :product_groups, [ProductGroup]

  def product_groups
    context.scoped_set!(:collection, object)
   …
end

GraphQL query

See above

Steps to reproduce

  1. Create a simple schema and type
  2. Add a couple of fields to the type
  3. In one of the field definitions that resolves to some other type, call context.scoped_set!
  4. In the field definitions for the other type, check the value of context (:collection in my case). Depending on the order of fields in the query, it will or won't have a value.

Expected behavior

I would expect the value of context[:collection] to always be present in the child objects after it has been set and not depend on the order of fields in the query.

Actual behavior

I could see the value getting set but sometime between it being set and the child object fields getting resolved, it was wiped out. I could not identify exactly where that was happening.

Additional context

Definitely not an urgent issue since a workaround was readily available in this situation but it'd be nice to understand what's going on in case we need different values for the same context key for different children at some point in the future.

With these details, we can efficiently hunt down the bug!

rmosolgo commented 2 years ago

Thanks for the detailed report... I definitely agree that it should work how you expect!

rmosolgo commented 2 years ago

Ok, taking a closer look, I have a clarifying question. It sounds like you want to:

  1. Assign scoped_set!(:collection, object) inside Collection#product_groups;
  2. Then, access context[:collection] inside Collection#product_handles

Is that right?

The flow described above is not supposed to work. Since the fields are siblings (they appear at the same depth), they aren't supposed to share a scoped context. Scoped context is supposed to flow "down" the query (to child fields) only, not to sibling fields.

However, when I sketch out a replication of this issue locally, I do see some values leaking from child to another. But in my replication, the "wrong" value is leaking -- that is, the context[:collection] from the previous def product_groups is showing up in the next def product_handles. Here's that replication script:

Testing scoped_context between sibling fields in a list

```ruby require "bundler/inline" gemfile do gem "graphql", "2.0" end class Schema < GraphQL::Schema class Product < GraphQL::Schema::Object field :handle, String end class Group < GraphQL::Schema::Object field :handle, String field :products, [Product] end class Collection < GraphQL::Schema::Object field :handles, [String] def handles p [:handles, object[:handle], context[:current_path], context[:collection]] object[:groups].map { |g| g[:handle] } end field :groups, [Group] def groups context.scoped_set!(:collection, object[:handle]) object[:groups] end end class Query < GraphQL::Schema::Object field :collections, Collection.connection_type def collections [ { handle: "collection-1", groups: [ { handle: "group-1", products: [{ handle: "product-1" }, {handle: "product-2"}] }, { handle: "group-2", products: [{ handle: "product-3" }, {handle: "product-4"}] }, ] }, { handle: "collection-2", groups: [ { handle: "group-3", products: [{ handle: "product-5" }, {handle: "product-6"}] }, { handle: "group-4", products: [{ handle: "product-7" }, {handle: "product-8"}] }, ] } ] end end query(Query) end pp Schema.execute(<<-GRAPHQL).to_h { collections { nodes { handles groups { handle products { handle } } } } } GRAPHQL # $ ruby scoped_context_test.rb # [:handles, "collection-1", ["collections", "nodes", 0, "handles"], nil] # # Uh oh, this is wrong: `object` is `"collection-2"`, but `context[:collection]` still has `"collection-1"`: # # [:handles, "collection-2", ["collections", "nodes", 1, "handles"], "collection-1"] # {"data"=> # {"collections"=> # {"nodes"=> # [{"handles"=>["group-1", "group-2"], # "groups"=> # [{"handle"=>"group-1", # "products"=>[{"handle"=>"product-1"}, {"handle"=>"product-2"}]}, # {"handle"=>"group-2", # "products"=>[{"handle"=>"product-3"}, {"handle"=>"product-4"}]}]}, # {"handles"=>["group-3", "group-4"], # "groups"=> # [{"handle"=>"group-3", # "products"=>[{"handle"=>"product-5"}, {"handle"=>"product-6"}]}, # {"handle"=>"group-4", # "products"=> # [{"handle"=>"product-7"}, {"handle"=>"product-8"}]}]}]}}} ```

Anyhow, that's a bug -- context[:collection] should be nil in def product_handles since it was assigned in def product_groups. I'll take a look soon and make sure that scoped context isn't leaking anymore.

Did I understand your question correctly? I also see mention of the products field having the wrong scoped context value, but I wasn't able to replicate any issues on that field. If you expect something different, could you please share the def products method that expect a context value to be present? Maybe some more context would help me replicate the issue.

srgoldman commented 2 years ago

Thanks for diving in. So you found a different issue as it turns out. I understand that it doesn't make sense for it to work for siblings. The situation I have is that inside the product_groups type, I'm referencing context[:collection], expecting it to have been set when the product_group was instantiated via the product_groups field in collection. If you look at the query above, products is a field on product_group. When a product_group is referenced directly, I want that products field to return all the products in the group, however, when the path is: collection { productGroups { products { ... }}}, I want to constraint the set of products to those that are both in the product group and the collection. It's inside the products field that I'm seeing the context not coming through.

Does that clarify things?

class ProductGroup < GraphQL::Schema::Object
  field :products, [Product]

  def products
    if context[:collection]
       object.products.where(collection: context[:collection])
   else
      object.products
   end
end
rmosolgo commented 2 years ago

Hmm, interesting... If I modify my replication script to print context[:collection] inside def products, it seems to work fine:

Checking scoped context inside a child field

```ruby require "bundler/inline" gemfile do gem "graphql", "2.0" end class Schema < GraphQL::Schema class Product < GraphQL::Schema::Object field :handle, String end class Group < GraphQL::Schema::Object field :handle, String field :products, [Product] def products p [:products, context[:current_path], context[:collection]] object[:products] end end class Collection < GraphQL::Schema::Object field :groups, [Group] def groups context.scoped_set!(:collection, object[:handle]) object[:groups] end end class Query < GraphQL::Schema::Object field :collections, Collection.connection_type def collections [ { handle: "collection-1", groups: [ { handle: "group-1", products: [{ handle: "product-1" }, {handle: "product-2"}] }, { handle: "group-2", products: [{ handle: "product-3" }, {handle: "product-4"}] }, ] }, { handle: "collection-2", groups: [ { handle: "group-3", products: [{ handle: "product-5" }, {handle: "product-6"}] }, { handle: "group-4", products: [{ handle: "product-7" }, {handle: "product-8"}] }, ] } ] end end query(Query) end pp Schema.execute(<<-GRAPHQL).to_h { collections { nodes { groups { handle products { handle } } } } } GRAPHQL # $ ruby scoped_context_test.rb # # Looks good, all `products` calls have a collection set: # # [:products, ["collections", "nodes", 0, "groups", 0, "products"], "collection-1"] # [:products, ["collections", "nodes", 0, "groups", 1, "products"], "collection-1"] # [:products, ["collections", "nodes", 1, "groups", 0, "products"], "collection-2"] # [:products, ["collections", "nodes", 1, "groups", 1, "products"], "collection-2"] # # {"data"=> # {"collections"=> # {"nodes"=> # [{"groups"=> # [{"handle"=>"group-1", # "products"=>[{"handle"=>"product-1"}, {"handle"=>"product-2"}]}, # {"handle"=>"group-2", # "products"=>[{"handle"=>"product-3"}, {"handle"=>"product-4"}]}]}, # {"groups"=> # [{"handle"=>"group-3", # "products"=>[{"handle"=>"product-5"}, {"handle"=>"product-6"}]}, # {"handle"=>"group-4", # "products"=> # [{"handle"=>"product-7"}, {"handle"=>"product-8"}]}]}]}}} ```

So, there must be something else different... What could it be 🤔

srgoldman commented 2 years ago

Hmmm... so we are using both GraphQL::Dataloader and GraphQL::Batch. The product groups for the collection are actually loaded via Dataloader.

    def product_groups
      dataloader.with(Sources::ProductGroupsByCollection, include_unavailable?).load(object.id).then do |pgs|
        # sort the product groups by the product_group_sort_order on the collection
        # need to dynamically build the sort order when include_unavailable? is true
        pgs.sort do |pg1, pg2|
          ordering = product_group_sort_order
          if ordering
            (ordering[pg1.handle] || 999) <=> (ordering[pg2.handle] || 999)
          else
            pg1.handle <=> pg2.handle
          end
        end
      end
    end
class Sources::ProductGroupsByCollection < GraphQL::Dataloader::Source
  def initialize(include_unavailable)
    @include_unavailable = include_unavailable
  end

  def fetch(collection_ids)
    available_for_sale_values = @include_unavailable ? [false, true] : [true]
    collection_ids.map do |col_id|
      pg_ids = Collection.find(col_id).products.where(available_for_sale: available_for_sale_values).pluck(:product_group_id).uniq
      ProductGroup.where(id: pg_ids)
    end
  end
end

It feels like the order of the fields isn't the cause of the issue but whatever impact it has on the execution plan/order exposes the issue. Here we're just using Dataloader but we have both in our schema since we ran into a different issue with Dataloader that was handled properly by Batch (I don't recall the details at the moment but can get them for you).

rmosolgo commented 2 years ago

Thanks for sharing that context -- I bet it's something to do with Dataloader 😖 . I was expecting to see a context.set_scoped! in there somewhere, but I don't ... where does that happen? Inside def product_groups? Inside the .then?

srgoldman commented 2 years ago

Ah, of course you were, but I pulled it out because it wasn't working as expected and instead have:

    def initialize(object, context)
      super
      context.scoped_set!(:collection, object)
    end

on Types::Collection but previously, yes, the first line of the product_groups method did the context.scoped_set!(:collection, object) call.

rmosolgo commented 2 years ago

Would you mind giving this branch a try? #3950 You could add it to your project with:

gem "graphql", github: "rmosolgo/graphql-ruby", ref: "scoped-context-refactor"

I basically re-wrote scoped context to use context[:current_path], which is known to be reliable all the time. It also simplified the runtime code a lot. After that refactor, I couldn't find any wackiness with dataloader... but I might have missed a spot.

If you're able to give it a shot, let me know what you find. If not, that's alright, I'll merge it soon and release it in 2.0.1.

rmosolgo commented 2 years ago

Oh, of course -- I backported my tests without the refactor, and I confirmed that they were very broken: 66f3ee0fd03a5b3b066540f8ab3484a768b4bb81 I expect #3950 will do the job 🍻

srgoldman commented 2 years ago

Hi Robert - sorry for the delay in responding, I was away for the long weekend (+ Friday). I've gotten my code back to the point where I can repro the issue but am stuck with upgrading to the latest version of graphql due to the gem dependencies. If you wouldn't mind directing me around this roadblock, I can test your fix in our scenario.

I think what's blocking the gem update is the shopify_api gem which depends on the graphql-client gem which is restricting the version of your gem.

Bundler could not find compatible versions for gem "graphql":
  In Gemfile:
    graphql (~> 2.0.0)

    shopify_api (~> 9.5) was resolved to 9.5.1, which depends on
      graphql-client was resolved to 0.17.0, which depends on
        graphql (~> 1.10)
rmosolgo commented 2 years ago

Ick, yup: https://github.com/github/graphql-client/blob/d19333d9587d7b187bba1f2339dc09a52067f8be/graphql-client.gemspec#L13

srgoldman commented 2 years ago

Thought I had commented here. I tested using the steps above and everything seemed to work as expected. I did open a PR against the graphql-client repo but am unclear on what to expect. Should I just proceed and use my local fork or wait for the PR to be merged?

Thanks again for the quick turnaround!