gmac / graphql-stitching-ruby Goto Github PK

Hello again.

When one of our location returns an error, GraphQL::Stitching::Client doesn't propagate that error and raises undefined local variable or method `index' instead at this line:

Indeed there is no such thing like index at this point.

Hello,

First, thanks for writing this, the implementation is very clean and easy to walk through.

I have a case where I'm upgrading a pretty stale version of GraphQL and I've run into an issue with Enums. The project makes use of the value property on Enums to translate between GraphQL Enum "labels" and Ruby values, e.g.:

Enum.value('UNSPECIFIED', 'Unspecified value', value: 'none') will be UNSPECIFIED in the Schema, but if passed as a query argument, will appear as "none" in Ruby.

The issue comes up during introspection, when Composer.build_enum_type attempts to build the enum types.

On this line: https://github.com/gmac/graphql-stitching-ruby/blob/main/lib/graphql/stitching/composer.rb#L238
it is constructing a new EnumValue via EnumValue.value, but it passes the value as the first argument, rather than the graphql_name.

In my case, this causes two issues:

1. Some of my Enum values are not valid EnumValue names, they are Ruby primitives or don't meet the naming validation rules
2. Those that do pass validation are still detached from their original EnumValue which links the name and value together.

I experimented pretty thoroughly with different approaches to solve this, I was hoping I could achieve it by implementing a custom enum_value_class, but the necessary values aren't passed down. For now, I've resorted to monkey-patching build_enum_type, which gets the job done for my narrow case, but there's likely a better general solution. For my Schema, I don't have overlapping types, so I can reliably pick the first location an EnumValue was seen and pull the graphql_name off of that, which I've done by simply:

enum_values_by_value_location.each do |value, enum_values_by_location|
  # Getting the first location
  location = enum_values_by_location.keys.first
  # Getting the GraphQL name off of it, or falling back to original behavior
  graphql_name = enum_values_by_location[location]&.graphql_name || value
  enum_value = value(graphql_name,
               value: value,
               description: builder.merge_descriptions(type_name, enum_values_by_location, enum_value: value),
               deprecation_reason: builder.merge_deprecations(type_name, enum_values_by_location, enum_value: value))

Any tips or thoughts appreciated, happy to offer any addition info as needed.

When one of our location returns an error, GraphQL::Stitching::Client doesn't propagate that error and raises no implicit conversion of nil into Array (TypeError) instead at this line:

This is because that extract_errors!(origin_sets_by_operation, errors) is returning nil.

Looking at the implementation,

This flatten! is the crux. It can return nil, in case the operation did not modify errors_result.

From @mikeharty:

I'm not totally clear on whether "default_value" is officially supported in the GraphQL gem, but we are using it and it does work. It appears to be left out of the implementation here: https://github.com/gmac/graphql-stitching-ruby/blob/main/lib/graphql/stitching/composer.rb#L369

I've worked around this adding a merge_default_values function into the monkey patch I mentioned above:

def merge_default_values(type_name, members_by_location, argument_name: nil, field_name: nil)
  default_values = members_by_location.values.map(&:default_value)
  
  return nil if default_values.any?(&:nil?)
  
  if default_values.uniq.length != 1
    path = [type_name, field_name, argument_name].compact.join('.')
    raise ComposerError, "Default values for `#{path}` must be the same."
  end
  
  default_values.first
end

and I updated the code at the point I linked to conditionally add the default_value via kwargs if it has a value, so that it doesn't default fields to nulls that previously had no default (caused validation issues otherwise)

type = merge_value_types(type_name, value_types, argument_name: argument_name, field_name: field_name)
default_value = merge_default_values(type_name, arguments_by_location, argument_name: argument_name, field_name: field_name)
kwargs = {}
kwargs[:default_value] = default_value unless default_value.nil? && type.non_null?
schema_argument = owner.argument(
  argument_name,
  description: merge_descriptions(type_name, arguments_by_location, argument_name: argument_name, 
                                                                    field_name: field_name),
  deprecation_reason: merge_deprecations(type_name, arguments_by_location, argument_name: argument_name, 
                                                                           field_name: field_name),
  type: GraphQL::Stitching::Util.unwrap_non_null(type),
  required: type.non_null?,
  camelize: false,
  **kwargs
)

GraphQL Ruby supports visibility controls for selectively hiding parts of a schema from view. Stitching should be able to piggy-back on the GraphQL Ruby implementation of the feature to allow portions of the combined schema to be hidden. Visibility controls would make sense as directives structured similar to Apollo authorizations.

There's presently one failing test involving a late-bound type when running with GraphQL v1.13. Let's assess and fix/ignore to expand gem compatibility down to GraphQL Ruby v1.13.

GraphQL foreign keys are commonly handled as Product.imageId is here:

# -- Products schema:

type Product {
  id: ID!
  imageId: ID!
}

# -- Images schema:

type Image {
  id: ID!
  url: String!
}

However, stitching wants this schema to be shaped as:

# -- Products schema:

type Product {
  id: ID!
  image: Image!
}

type Image {
  id: ID!
}

# -- Images schema:

type Image {
  id: ID!
  url: String!
}

Rather than forcing services to be reshaped (assuming we even have ownership and are able to), it would be nice if stitching would handle the transformation of key fields into typed relations, such as:

# -- Products schema:

type Product {
  id: ID!
  imageId: ID! @relation(fieldName: "image", typeName: "Image", foreignKey: "id")
  # --> image: Image! // type Image { id: ID! }
}

# -- Images schema:

type Image {
  id: ID!
  url: String!
}

In the Request class, the prepare! method applies default values to variables using conditional assignment:

operation.variables.each do |v|
  @variables[v.name] ||= v.default_value
end

Unfortunately,false is falsey (obviously), so it is always overwritten with the value in v.default_value (which could be nil or true).

This is critical, I'd think, considering it can flip false boolean values to true if the default is true.

Script to reproduce (run from project root):

require_relative 'lib/graphql/stitching'
require_relative 'lib/graphql/stitching/request'

query = <<~GRAPHQL
  query($a: Boolean, $b: Boolean, $c: Boolean = true) {
    base(a: $a, b: $b, c: $c) { id }
  }
GRAPHQL

variables = { "a" => true, "b" => false, "c" => false }
request = GraphQL::Stitching::Request.new(GraphQL.parse(query), variables: variables)
request.prepare!

puts request.variables
# {"a"=>true, "b"=>nil, "c"=>true}

Fix is simple, I'll open a PR in a moment with the fix + a test.

Summary

The Executor currently runs request executions synchronously.

def exec!
  # @todo make this async
  next_ops = @queue.select { _1[:after_key].nil? }

  while next_ops.any?
    next_ops.each do |op|
      # Each of these "next operations" should be run in parallel...
      # Also, each individual completion should trigger a next round looking for new operations
      # (we do NOT need to await all operations in this round before looking for followups)
      @status[op[:key]] = perform_operation(op)
    end

    next_ops = @queue.select do |op|
      after_key = op[:after_key]
      after_key && @status[after_key] == :completed && @status[op[:key]].nil?
    end
  end
end

We need to explore async options for running batches of requests concurrently. We'd ideally align with GraphQL Ruby's async implementation to avoid new dependencies, or match GraphQL Batch. Things to look at:

• GraphQL Ruby dataloader docs
• The GraphQL Interpreter, which is what all GraphQL::Schema.execute calls go to. All requests are multiplexed (single execution is just a wrapper for a multiplex of one)... and multiplexing uses GraphQL's built-in dataloader.
• Might want to talk to @swalkinshaw about how GraphQL Batch gets its async event reactor. Following GraphQL Batch would be a good secondary approach.

The library does some work using hashes... this is because we want to facilitate serialization and deserialization of critical data structures (delegation maps and query plans). However, right now some parts use string keys and other parts use symbol keys... this is at odds with running JSON.parse on a cached structure and being ready to go.

While symbol keys look nicer, delegation maps have to use all string keys for name matching. It probably makes the most sense to use string keys everywhere. We could also potentially wrap some common structures like boundaries in Structs, but that just adds a step and more object creation, so I'm not sure it's worth it.

At present, executor batching inlines all stitching IDs into their resolver queries:

query MyOperation_2 {
  _0_result: widgets(ids:["a","b","c"]) { ... }
  _1_0_result: sprocket(id:"x") { ... }
  _1_1_result: sprocket(id:"y") { ... }
}

This is not ideal because it creates high request cardinality, which may defeat some backend caches. It would be generally better if requests stayed consistent when possible and submitted keys as request variables:

query MyOperation_2($_0_key: [ID!]!, $_1_0_key: ID!, $_1_1_key: ID!) {
  _0_result: widgets(ids: $_0_key) { ... }
  _1_0_result: sprocket(id: $_1_0_key) { ... }
  _1_1_result: sprocket(id: $_1_1_key) { ... }
}

# variables: { "_0_key": ["a","b","c"], "_1_0_key": "x", "_1_1_key": "y" }

From @mikeharty:

# Getting double args sometimes... why?
return if owner.arguments.any? { _1.first == argument_name }

This line causes arguments to be dropped entirely - I haven't dug into the root of why the double args, but if I change that return to be next, my arguments stop being dropped.

I'm testing this against a reasonably substantial (~400 distinct types) schema and with these changes am generating identical schema to without stitching.

Summary

Requests are statically planned up front, which means that fragment selections may generate operations forking from types that are not actually resolved. We need to perform a type check after each execution and (recursively?) eliminate child operations that don't actually apply to the resolved type.

Summary

The library is organized around composable pieces:

Composer -> Supergraph -> Planner -> Executor -> Shaper

These are intentionally discrete so that parts and pieces of the stitching workflow can be mixed and matched (ie: precompose and caching of supergraph, caching and restoration of query plans, etc). However, this makes the library difficult to use quickly out of the box. We need a Gateway component that rolls up a boilerplate workflow of all the parts and pieces into one unit.

Desired API

Should be easy to build a stitched gateway and use it to execute requests:

gateway = GraphQL::Stitching::Gateway.new({
  products: {
    schema: GraphQL::Schema.from_definition(movies_schema),
    client: GraphQL::Stitching::RemoteClient.new(url: "http://localhost:3000"),
  },
  showtimes: {
    schema: GraphQL::Schema.from_definition(showtimes_schema),
    client: GraphQL::Stitching::RemoteClient.new(url: "http://localhost:3001"),
  },
  local: {
    schema: MyLocal::GraphQL::Schema
  },
})

gateway.cache_read do |key|
  $redis.get(key) # << 3P code
end

gateway.cache_write do |key, payload|
  $redis.set(key, payload) # << 3P code
end

result = gateway.execute(
  # Same basic arguments as GraphQL Ruby (https://graphql-ruby.org/queries/executing_queries)
  query: ,
  document: ,
  variables: ,
)

Steps

This general workflow is mostly laid out in example/gateway.rb. Use that for reference...

1. Run Composer during gateway initialization – make sure all location names are input as strings.
2. Also during initialization, add provided clients to the composed Supergraph. A client is anything that responds to .call(). We should consolidate Supergraph's assign_location_url and assign_location_handler into one "assign_location_client" method that accepts any call-able object.
3. The Gateway cache_read and cache_write methods should stash the provided procs as instance variables. These are not required to be set.
4. Add an execute method, the signature should be a subset of the GraphQL::Schema.execute method. When invoking execute:

• Generate a Document (stitching lib) from the provided document or query
• Validate the document AST against the Supergraph schema. Format and return any validation errors.
• Bonus: add a variable hosting routine to normalize the document.
• If there's a cache reader, then generate a document SHA and request it from the cache accessor. Parse any returned results (currently requires JSON to parse with symbolized keys). If we have a cached plan, we can skip the next step.
• Plan the submitted request document unless a cached plan was found.
  • If there's a cache writer, then provide the generated plan and the request SHA to the cache writer.
• Execute the plan with provided query variables.
• Pass the raw execution result to the Shaper (in progress).

5. Needs tests.

Hello!

I'm considering using this gem for a graphql-ruby / packwerk project. The idea of having in-process graphql stitching is compelling. I'm curious if you have thoughts about how I could avoid n+1 problems. I don't have a great understanding about how gateways / schema stitching tools handle this in general.

Summary

Input values embedded into a GraphQL document make that document unique and prevent it from hashing consistently when looking for a cached query plan:

query { 
  product(id: "1") { name }
}

A nice add-on would be a utility that traverses a request document and extracts input literals and hoists them up to document variables. Then no matter how the request was submitted, it will be subject to plan caching with a normalized body:

query($_hoist_0: ID!){ 
  product(id: $_hoist_0) { name }
}

# variables { "_hoist_0": "1" }

This normalization would be appropriate to happen in the Document object.

Similar to other composer libraries, there should be some controls for hiding elements in a schema. Specifically:

• @private: field/argument is treated like it doesn't exist in a location schema.
• @inaccessible: eliminates the field/argument entirely from the combined schema.
• @override: prioritize an element from a given location.

I recently added support for multipart form file uploads in the project where I've implemented this library. I've been debating whether or not it's something that would make sense to incorporate here.

My implementation has deviated pretty far in this particular area; I implemented a custom HttpExecutable in order to swap the client to RestClient (needed a proxy), and to add pre/post-request hook points to manipulate queries and responses. This is also where the multipart form handling happens, I'm using apollo_upload_server-ruby, which does follow the spec, but is a Rails specific implementation.

I say all of that essentially to say, maybe that's where this belongs, in a custom implementation, if needed. I figured I'd get your take before making that assumption. The thought also crossed my mind that maybe there's a plugin-style approach that would make sense. Let me know if there's any interest here, I'm happy to more formally propose a solution if there's an appetite for it, just wanted to gauge whether this feels messy/out of scope first.

It'd be nice to formally support field-level authorization through the query planner, similar to other federation libraries. A few specs:

• Unauthorized fields are simply filtered out of the request by default.
• A setting opts requests with unauthorized fields into returning immediately with an error.

It looks like @mikeharty has been doing some auth work in his custom executor. Mike – any chance you could elaborate here with more on how the feature could/should work with what you're already doing?

Right now, many operations may have the same after_key yet all target the same location on behalf of different insertion paths. This results in several requests made to the same service during the same execution frame. We should expand batching to write a single query for all of the different operations being delegated during a given frame.

At present, runtime directives are completely ignored by the planner. Need to:

• Extract variables from all runtime directives while planning.
• Confirm that runtime directives are passed through in subqueries.
• Make planning operations aware of runtime directives on their root scope, and conditionally run operations when an entire operation can be ignored.

The @boundary directive is terminology borrowed from Bramble, and while I like it, it's not necessary to carry it over and is potentially confusing given that Bramble's boundary annotations work differently and are a lot more confusing.

Summary

Right now, a raw execution result is returned directly. This raw result has many possible inaccuracies:

• Might contain stitching keys that were automatically added.
• Might be missing requested fields rather than providing the requested field with a null value.
• Needs to apply schema nullability constraints to the resolved payload.

We need a final-pass algorithm that traverses the original request, prunes extra payload fields, adds missing payload fields as null, and then bubbles nullability constraints up through the document tree. Same basic idea as the Apollo resultsShaper or Bramble bubbleUpNullValuesInPlace.

There's a dev branch setup for this work here: https://github.com/gmac/graphql-stitching-ruby/compare/dev_shaper?expand=1

Tests run using:

bundle exec rake test TEST=test/graphql/stitching/shaper_test.rb

Example

The user requested this:

query {
  storefront(id: "1") {
    id
    products {
      upc
      name
      price
      nullableField
    }
  }
}

But the raw execution result looks like this:

{
  "data": {
    "storefront": {
      "id": "1",
      "products": [
        {
          "upc": "1",
          "_STITCH_upc": "1",
          "_STITCH_typename": "Product",
          "name": "iPhone",
          "price": 699.99,
          "nullableField": 1
        },
        {
          "upc": "2",
          "_STITCH_upc": "2",
          "_STITCH_typename": "Product",
          "name": "Apple Watch",
          "price": 399.99
        }
      ]
    }
  }
}

In the above, the user didn't request the stitching keys, so they should be removed. They did request nullableField but a value only came back for one record, so the field is missing from the second and should be added as "nullableField": null. Lastly, we'd need to bubble up errors in place based on schema null constraints.