Skip to content

Resolve "[graphql] Pull together significant changes to communicate"

Brett Walker requested to merge 363130-graphql-pull-together-significant-changes-to-communicate into master

What does this MR do and why?

Details many of the changes made when upgrading the GraphQL Ruby gem from 1.11.x to 1.13.x. The reason for so many changes is that we started using the GraphQL - Interpreter

Related to #363130 (closed)

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Minor Points of Interest

These are things that are minor and not a big deal, and most of them will cause an error during a spec if you do something that is not supported.

  • resolve procs are no longer supported by the gem. example MR

    For example, this is no longer supported

    field :project, Types::ProjectType,
      description: 'The project the snippet is associated with',
      null: true,
      authorize: :read_project
      resolve: -> (snippet, args, context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Project, snippet.project_id).find }

    Instead, use a method for resolving:

    field :project, Types::ProjectType,
      description: 'The project the snippet is associated with',
      null: true,
      authorize: :read_project

def project
  Gitlab::Graphql::Loaders::BatchModelLoader.new(Project, object.project_id).find
end

  • We should always use the newer standard types, such as GraphQL::Types::Int, instead of GraphQL::INT_TYPE. example MR. A cop was created to enforce this.

  • default values on arguments should have have the same type. For example, if an argument is of type GraphQL::Types::String, then the default_value should also be of type GraphQL::Types::String. example MR

    argument :ref, GraphQL::Types::String,
         required: false,
         required: false,
         default_value: 'some value'

  • to_graphql is deprecated and will be removed in 2.0. Do not use. Most information you need is now on the actual field, such as in this MR

  • use .to_type_signature instead of to_graphql when you need the type name. example MR

  • most errors are no longer raised as exceptions, but are bubbled up from the framework. example MR

    When testing resolvers, instead of

    expect { result }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)

    use expect_graphql_error_to_be_created instead

    expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ResourceNotAvailable) do
  result
end

  • When creating a field extension, subclass from GraphQL::Schema::FieldExtension instead of GraphQL::Schema::Field::ConnectionExtension. You can otherwise get duplicate connection methods defined. example MR

  • Use our empty_schema from GraphqlHelpers instead of trying to create your own. example MR

  • use query_double(schema: nil) from GraphqlHelpers instead of double('query', schema: nil). example MR

  • when exceptions are raised in the lower levels of the code/gem, we now add backtrace information to the error message, which can help track down some tricky bugs. MR

  • calls to GraphQL::ObjectType.accepts_definitions and GraphQL::Define.assign_metadata_key are no longer supported and have been removed. MR

  • the gem changed how input names were calculated. Having graphql_name called after any arguments were defined could cause naming issues. So we just standardized on putting it at the top. A cop was created to enforce it. See [graphql] Move graphql_name call to beginning of class and DuplicateNamesError when upgrading to 1.13

  • Gitlab::Graphql::Present was converted to a field extension, instead of our old instrumentation. MR

  • Replaced calls-gitaly instrumentation with a field extension, MR

Important

  • possible cyclic dependencies, which main be resolved once we've upgraded to 2.x. See Adding field with resolver on a Type causes "Can't determine the return type " error on a different Type and Fix unresolved name due to cyclic definition

    # Normally this wouldn't be needed and we could use
#   type Types::IssueType.connection_type, null: true
# in a resolver. However we can end up with cyclic definitions,
# which can result in errors like
#   NameError: uninitialized constant Resolvers::GroupIssuesResolver 
#
# Now we would use
#   type "Types::IssueConnection", null: true
# which gives a delayed resolution, and the proper connection type.
# See app/graphql/resolvers/base_issues_resolver.rb
# Reference: https://github.com/rmosolgo/graphql-ruby/issues/3974#issuecomment-1084444214

  • FindClosest was removed as it relied heavily on the internal node structure. MR Instead:

    • use the query context to pass information to lower-parts of the evaluation tree (using scoped_set!)
    • use parent to examine the immediate parent of the current node (no traversal is possible, but you can see through connections)
    • use context.namespace(:interpreter)[:current_path] to see the current path.
    • use statically available information on the objects

  • When testing resolvers using GraphqlHelpers#resolve, arguments for the resolver can be handled two ways.

    1. 95% of the resolver specs use arguments that are ruby objects, as opposed to when using the GraphQL API only strings and integers are used. This works fine in most cases.

    2. If your resolver takes arguments that use a prepare proc, such as a resolver that accepts timeframe arguments (TimeFrameArguments), you must pass the arg_style: :internal_prepared parameter into the resolve method. This tells the code to convert the arguments into strings and integers and pass them through regular argument handling, ensuring that the prepare proc is called correctly. For example in iterations_resolver_spec.rb :

      def resolve_group_iterations(args = {}, obj = group, context = { current_user: current_user })
  resolve(described_class, obj: obj, args: args, ctx: context, arg_style: :internal_prepared)
end

      One additional caveat is that if you are passing enums as a resolver argument, you must use the external representation of the enum, rather than the internal. For example:

      # good
resolve_group_iterations({ search: search, in: ['CADENCE_TITLE'] })

# bad
resolve_group_iterations({ search: search, in: [:cadence_title] })

      The use of :internal_prepared was added as a bridge for the GraphQL gem upgrade. Testing resolvers directly will be removed eventually via issue #363121, and writing unit tests for resolvers/mutations is already deprecated

Edited by Brett Walker

Merge request reports