diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index d66a2333d119a936869572cd4b5addf915eb8c65..7ab5dc36e4a446f6e185d8dd35d3028a75fefddc 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -13,8 +13,6 @@ class GitlabSchema < GraphQL::Schema use GraphQL::Pagination::Connections use BatchLoader::GraphQL use Gitlab::Graphql::Authorize - use Gitlab::Graphql::Present - use Gitlab::Graphql::CallsGitaly use Gitlab::Graphql::Pagination::Connections use Gitlab::Graphql::GenericTracing use Gitlab::Graphql::Timeout, max_seconds: Gitlab.config.gitlab.graphql_timeout diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb index 5db618254cbbfd513ae498090920395e07d80fc7..67bba079512cb906d0c13af9bb42036c0c3cfcf0 100644 --- a/app/graphql/resolvers/base_resolver.rb +++ b/app/graphql/resolvers/base_resolver.rb @@ -12,8 +12,17 @@ def self.requires_argument! @requires_argument = true end + def self.calls_gitaly! + @calls_gitaly = true + end + def self.field_options - super.merge(requires_argument: @requires_argument) + extra_options = { + requires_argument: @requires_argument, + calls_gitaly: @calls_gitaly + }.compact + + super.merge(extra_options) end def self.singular_type diff --git a/app/graphql/resolvers/last_commit_resolver.rb b/app/graphql/resolvers/last_commit_resolver.rb index dd89c322617043e2dfe796fac0460be0cfabc28c..00c43bdfee647e61b462d185fcbe2a53932e3275 100644 --- a/app/graphql/resolvers/last_commit_resolver.rb +++ b/app/graphql/resolvers/last_commit_resolver.rb @@ -4,6 +4,8 @@ module Resolvers class LastCommitResolver < BaseResolver type Types::CommitType, null: true + calls_gitaly! + alias_method :tree, :object def resolve(**args) diff --git a/app/graphql/resolvers/metrics/dashboard_resolver.rb b/app/graphql/resolvers/metrics/dashboard_resolver.rb index f569cb0b2c3f7803f608eda2ecc91c0b4fa41562..a82a4a952545dd513e36bee84664d5394291ecab 100644 --- a/app/graphql/resolvers/metrics/dashboard_resolver.rb +++ b/app/graphql/resolvers/metrics/dashboard_resolver.rb @@ -3,19 +3,30 @@ module Resolvers module Metrics class DashboardResolver < Resolvers::BaseResolver + type Types::Metrics::DashboardType, null: true + calls_gitaly! + argument :path, GraphQL::STRING_TYPE, required: true, - description: "Path to a file which defines metrics dashboard eg: 'config/prometheus/common_metrics.yml'." - - type Types::Metrics::DashboardType, null: true + description: "Path to a file which defines metrics dashboard " \ + "eg: 'config/prometheus/common_metrics.yml'." alias_method :environment, :object def resolve(**args) return unless environment - ::PerformanceMonitoring::PrometheusDashboard - .find_for(project: environment.project, user: context[:current_user], path: args[:path], options: { environment: environment }) + ::PerformanceMonitoring::PrometheusDashboard.find_for(**args, **service_params) + end + + private + + def service_params + { + project: environment.project, + user: current_user, + options: { environment: environment } + } end end end diff --git a/app/graphql/resolvers/snippets/blobs_resolver.rb b/app/graphql/resolvers/snippets/blobs_resolver.rb index 672214df7d5b0d49d22da2aeaeecb20e29299e47..569b82149d3d47e2200e36d92a4172da0d70f080 100644 --- a/app/graphql/resolvers/snippets/blobs_resolver.rb +++ b/app/graphql/resolvers/snippets/blobs_resolver.rb @@ -8,6 +8,7 @@ class BlobsResolver < BaseResolver type Types::Snippets::BlobType.connection_type, null: true authorize :read_snippet + calls_gitaly! alias_method :snippet, :object diff --git a/app/graphql/resolvers/tree_resolver.rb b/app/graphql/resolvers/tree_resolver.rb index 7a70c35897d0e8b175c2ec4c1fdc5af383bc4b67..c07d9187d4d8106b48bf3c279afc928d58a6b1a6 100644 --- a/app/graphql/resolvers/tree_resolver.rb +++ b/app/graphql/resolvers/tree_resolver.rb @@ -4,6 +4,8 @@ module Resolvers class TreeResolver < BaseResolver type Types::Tree::TreeType, null: true + calls_gitaly! + argument :path, GraphQL::STRING_TYPE, required: false, default_value: '', diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index f85675f72f265d0412cff8cfedf1b25a321edeab..78ab689092336ae8eea6076749cb7ac4c326832f 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -9,16 +9,25 @@ class BaseField < GraphQL::Schema::Field DEFAULT_COMPLEXITY = 1 - def initialize(*args, **kwargs, &block) + def initialize(**kwargs, &block) @calls_gitaly = !!kwargs.delete(:calls_gitaly) - @constant_complexity = !!kwargs[:complexity] + @constant_complexity = kwargs[:complexity].is_a?(Integer) && kwargs[:complexity] > 0 @requires_argument = !!kwargs.delete(:requires_argument) kwargs[:complexity] = field_complexity(kwargs[:resolver_class], kwargs[:complexity]) @feature_flag = kwargs[:feature_flag] kwargs = check_feature_flag(kwargs) kwargs = gitlab_deprecation(kwargs) - super(*args, **kwargs, &block) + super(**kwargs, &block) + + # We want to avoid the overhead of this in prod + extension ::Gitlab::Graphql::CallsGitaly::FieldExtension if Gitlab.dev_or_test_env? + + extension ::Gitlab::Graphql::Present::FieldExtension + end + + def may_call_gitaly? + @constant_complexity || @calls_gitaly end def requires_argument? @@ -54,8 +63,10 @@ def feature_documentation_message(key, description) end def check_feature_flag(args) - args[:description] = feature_documentation_message(args[:feature_flag], args[:description]) if args[:feature_flag].present? - args.delete(:feature_flag) + ff = args.delete(:feature_flag) + return args unless ff.present? + + args[:description] = feature_documentation_message(ff, args[:description]) args end @@ -78,7 +89,9 @@ def field_resolver_complexity # items which can be loaded. proc do |ctx, args, child_complexity| # Resolvers may add extra complexity depending on used arguments - complexity = child_complexity + self.resolver&.try(:resolver_complexity, args, child_complexity: child_complexity).to_i + complexity = child_complexity + resolver&.try( + :resolver_complexity, args, child_complexity: child_complexity + ).to_i complexity += 1 if calls_gitaly? complexity += complexity * connection_complexity_multiplier(ctx, args) @@ -93,7 +106,7 @@ def connection_complexity_multiplier(ctx, args) page_size = field_defn.connection_max_page_size || ctx.schema.default_max_page_size limit_value = [args[:first], args[:last], page_size].compact.min - multiplier = self.resolver&.try(:complexity_multiplier, args).to_f + multiplier = resolver&.try(:complexity_multiplier, args).to_f limit_value * multiplier end end diff --git a/app/graphql/types/current_user_todos.rb b/app/graphql/types/current_user_todos.rb index 79a430af1d7e635e545e4f39fe66ddc4ee6551b6..2551db875b0fb684799165fac00c918fb7b84e62 100644 --- a/app/graphql/types/current_user_todos.rb +++ b/app/graphql/types/current_user_todos.rb @@ -16,9 +16,10 @@ module CurrentUserTodos end def current_user_todos(state: nil) - state ||= %i(done pending) # TodosFinder treats a `nil` state param as `pending` + state ||= %i[done pending] # TodosFinder treats a `nil` state param as `pending` + klass = unpresented.class - TodosFinder.new(current_user, state: state, type: object.class.name, target_id: object.id).execute + TodosFinder.new(current_user, state: state, type: klass.name, target_id: object.id).execute end end end diff --git a/app/graphql/types/snippets/blob_type.rb b/app/graphql/types/snippets/blob_type.rb index fb0c1d9409b70cd51644348141c1659b1d8a7ffd..fb9ee3807054ffd4d3444dec679d446f087eb992 100644 --- a/app/graphql/types/snippets/blob_type.rb +++ b/app/graphql/types/snippets/blob_type.rb @@ -14,7 +14,6 @@ class BlobType < BaseObject field :plain_data, GraphQL::STRING_TYPE, description: 'Blob plain highlighted data.', - calls_gitaly: true, null: true field :raw_path, GraphQL::STRING_TYPE, diff --git a/app/graphql/types/tree/blob_type.rb b/app/graphql/types/tree/blob_type.rb index 3823bd94083229f5d1c60bbb9e06da374adc599f..d192c8d3c57224c3b5a96786ad992fa98c9d5020 100644 --- a/app/graphql/types/tree/blob_type.rb +++ b/app/graphql/types/tree/blob_type.rb @@ -15,6 +15,7 @@ class BlobType < BaseObject field :web_path, GraphQL::STRING_TYPE, null: true, description: 'Web path of the blob.' field :lfs_oid, GraphQL::STRING_TYPE, null: true, + calls_gitaly: true, description: 'LFS ID of the blob.' field :mode, GraphQL::STRING_TYPE, null: true, description: 'Blob mode in numeric format.' diff --git a/lib/gitlab/graphql/calls_gitaly.rb b/lib/gitlab/graphql/calls_gitaly.rb deleted file mode 100644 index 40cd74a34f2ef2a76baa80a6e8154a4f0960ab83..0000000000000000000000000000000000000000 --- a/lib/gitlab/graphql/calls_gitaly.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - # Wraps the field resolution to count Gitaly calls before and after. - # Raises an error if the field calls Gitaly but hadn't declared such. - module CallsGitaly - extend ActiveSupport::Concern - - def self.use(schema_definition) - schema_definition.instrument(:field, Gitlab::Graphql::CallsGitaly::Instrumentation.new, after_built_ins: true) - end - end - end -end diff --git a/lib/gitlab/graphql/calls_gitaly/field_extension.rb b/lib/gitlab/graphql/calls_gitaly/field_extension.rb new file mode 100644 index 0000000000000000000000000000000000000000..32530b47ce3737e9a0a57ebba92edfbbd7e957c5 --- /dev/null +++ b/lib/gitlab/graphql/calls_gitaly/field_extension.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module CallsGitaly + # Check if any `calls_gitaly: true` declarations need to be added + # + # See BaseField: this extension is not applied if the field does not + # need it (i.e. it has a constant complexity or knows that it calls + # gitaly) + class FieldExtension < ::GraphQL::Schema::FieldExtension + include Laziness + + def resolve(object:, arguments:, **rest) + yield(object, arguments, [current_gitaly_call_count, accounted_for]) + end + + def after_resolve(value:, memo:, **rest) + (value, count) = value_with_count(value, memo) + calls_gitaly_check(count) + accounted_for(count) + + value + end + + private + + # Resolutions are not nested nicely (due to laziness), so we have to + # know not just how many calls were made before resolution started, but + # also how many were accounted for by fields with the correct settings + # in between. + # + # e.g. the following is not just plausible, but common: + # + # enter A.user (lazy) + # enter A.x + # leave A.x + # enter A.calls_gitaly + # leave A.calls_gitaly (accounts for 1 call) + # leave A.user + # + # In this circumstance we need to mark the calls made by A.calls_gitaly + # as accounted for, even though they were made after we yielded + # in A.user + def value_with_count(value, (previous_count, previous_accounted_for)) + newly_accounted_for = accounted_for - previous_accounted_for + value = force(value) + count = [current_gitaly_call_count - (previous_count + newly_accounted_for), 0].max + + [value, count] + end + + def current_gitaly_call_count + Gitlab::GitalyClient.get_request_count || 0 + end + + def calls_gitaly_check(calls) + return if calls < 1 || field.may_call_gitaly? + + error = RuntimeError.new(<<~ERROR) + #{field_name} unexpectedly calls Gitaly! + + Please either specify a constant complexity or add `calls_gitaly: true` + to the field declaration + ERROR + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error) + end + + def accounted_for(count = nil) + return 0 unless Gitlab::SafeRequestStore.active? + + Gitlab::SafeRequestStore["graphql_gitaly_accounted_for"] ||= 0 + + if count.nil? + Gitlab::SafeRequestStore["graphql_gitaly_accounted_for"] + else + Gitlab::SafeRequestStore["graphql_gitaly_accounted_for"] += count + end + end + + def field_name + "#{field.owner.graphql_name}.#{field.graphql_name}" + end + end + end + end +end diff --git a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb deleted file mode 100644 index 11d3c50e093337c8209d950606cecb7454a2e08f..0000000000000000000000000000000000000000 --- a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module CallsGitaly - class Instrumentation - # Check if any `calls_gitaly: true` declarations need to be added - # Do nothing if a constant complexity was provided - def instrument(_type, field) - type_object = field.metadata[:type_class] - return field unless type_object.respond_to?(:calls_gitaly?) - return field if type_object.constant_complexity? || type_object.calls_gitaly? - - old_resolver_proc = field.resolve_proc - - gitaly_wrapped_resolve = -> (typed_object, args, ctx) do - previous_gitaly_call_count = Gitlab::GitalyClient.get_request_count - result = old_resolver_proc.call(typed_object, args, ctx) - current_gitaly_call_count = Gitlab::GitalyClient.get_request_count - calls_gitaly_check(type_object, current_gitaly_call_count - previous_gitaly_call_count) - result - end - - field.redefine do - resolve(gitaly_wrapped_resolve) - end - end - - def calls_gitaly_check(type_object, calls) - return if calls < 1 - - # Will inform you if there needs to be `calls_gitaly: true` as a kwarg in the field declaration - # if there is at least 1 Gitaly call involved with the field resolution. - error = RuntimeError.new("Gitaly is called for field '#{type_object.name}' on #{type_object.owner.try(:name)} - please either specify a constant complexity or add `calls_gitaly: true` to the field declaration") - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error) - end - end - end - end -end diff --git a/lib/gitlab/graphql/present.rb b/lib/gitlab/graphql/present.rb index 6d86d632ab44551288691c42169d81d49e9e3e69..fdaf075eb256d7d5409ef0db494f232614ed11ef 100644 --- a/lib/gitlab/graphql/present.rb +++ b/lib/gitlab/graphql/present.rb @@ -12,11 +12,30 @@ def self.present_using(kls) def self.presenter_class @presenter_class end + + def self.present(object, attrs) + klass = @presenter_class + return object if !klass || object.is_a?(klass) + + @presenter_class.new(object, **attrs) + end + end + + def unpresented + unwrapped || object end - def self.use(schema_definition) - schema_definition.instrument(:field, ::Gitlab::Graphql::Present::Instrumentation.new) + def present(object_type, attrs) + return unless object_type.respond_to?(:present) + + self.unwrapped ||= object + # @object belongs to Schema::Object, which does not expose a writer. + @object = object_type.present(unwrapped, attrs) # rubocop: disable Gitlab/ModuleWithInstanceVariables end + + private + + attr_accessor :unwrapped end end end diff --git a/lib/gitlab/graphql/present/field_extension.rb b/lib/gitlab/graphql/present/field_extension.rb new file mode 100644 index 0000000000000000000000000000000000000000..2e211b70d35f471c2cc4160826cdc9194c1c5f4d --- /dev/null +++ b/lib/gitlab/graphql/present/field_extension.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module Present + class FieldExtension < ::GraphQL::Schema::FieldExtension + SAFE_CONTEXT_KEYS = %i[current_user].freeze + + def resolve(object:, arguments:, context:) + attrs = safe_context_values(context) + + # We need to handle the object being either a Schema::Object or an + # inner Schema::Object#object. This depends on whether the field + # has a @resolver_proc or not. + if object.is_a?(::Types::BaseObject) + object.present(field.owner, attrs) + yield(object, arguments) + else + # This is the legacy code-path, hit if the field has a @resolver_proc + # TODO: remove this when resolve procs are removed from the + # graphql-ruby library, and all field instrumentation is removed. + # See: https://github.com/rmosolgo/graphql-ruby/issues/3385 + presented = field.owner.try(:present, object, attrs) || object + yield(presented, arguments) + end + end + + private + + def safe_context_values(context) + context.to_h.slice(*SAFE_CONTEXT_KEYS) + end + end + end + end +end diff --git a/lib/gitlab/graphql/present/instrumentation.rb b/lib/gitlab/graphql/present/instrumentation.rb deleted file mode 100644 index b8535575da5d8e5b15b9652e899ab9feb104ce8c..0000000000000000000000000000000000000000 --- a/lib/gitlab/graphql/present/instrumentation.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Graphql - module Present - class Instrumentation - SAFE_CONTEXT_KEYS = %i[current_user].freeze - - def instrument(type, field) - return field unless field.metadata[:type_class] - - presented_in = field.metadata[:type_class].owner - return field unless presented_in.respond_to?(:presenter_class) - return field unless presented_in.presenter_class - - old_resolver = field.resolve_proc - - resolve_with_presenter = -> (presented_type, args, context) do - # We need to wrap the original presentation type into a type that - # uses the presenter as an object. - object = presented_type.object - - if object.is_a?(presented_in.presenter_class) - next old_resolver.call(presented_type, args, context) - end - - attrs = safe_context_values(context) - presenter = presented_in.presenter_class.new(object, **attrs) - - # we have to use the new `authorized_new` method, as `new` is protected - wrapped = presented_type.class.authorized_new(presenter, context) - - old_resolver.call(wrapped, args, context) - end - - field.redefine do - resolve(resolve_with_presenter) - end - end - - private - - def safe_context_values(context) - context.to_h.slice(*SAFE_CONTEXT_KEYS) - end - end - end - end -end diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index 4db1264306923ff0a573eebbd00724a4e50cff1a..cb2bb25b098c2237888ea30984a61c366b8816b5 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -18,14 +18,6 @@ expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Authorize::Instrumentation)) end - it 'enables using presenters' do - expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Present::Instrumentation)) - end - - it 'enables using gitaly call checker' do - expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::CallsGitaly::Instrumentation)) - end - it 'has the base mutation' do expect(described_class.mutation).to eq(::Types::MutationType) end @@ -47,7 +39,7 @@ end describe '.execute' do - context 'for different types of users' do + context 'with different types of users' do context 'when no context' do it 'returns DEFAULT_MAX_COMPLEXITY' do expect(GraphQL::Schema) @@ -78,13 +70,15 @@ context 'when a logged in user' do it 'returns AUTHENTICATED_COMPLEXITY' do - expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::AUTHENTICATED_COMPLEXITY)) + expect(GraphQL::Schema).to receive(:execute) + .with('query', hash_including(max_complexity: GitlabSchema::AUTHENTICATED_COMPLEXITY)) described_class.execute('query', context: { current_user: user }) end it 'returns AUTHENTICATED_MAX_DEPTH' do - expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_depth: GitlabSchema::AUTHENTICATED_MAX_DEPTH)) + expect(GraphQL::Schema).to receive(:execute) + .with('query', hash_including(max_depth: GitlabSchema::AUTHENTICATED_MAX_DEPTH)) described_class.execute('query', context: { current_user: user }) end @@ -94,7 +88,8 @@ it 'returns ADMIN_COMPLEXITY' do user = build :user, :admin - expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::ADMIN_COMPLEXITY)) + expect(GraphQL::Schema).to receive(:execute) + .with('query', hash_including(max_complexity: GitlabSchema::ADMIN_COMPLEXITY)) described_class.execute('query', context: { current_user: user }) end @@ -130,7 +125,7 @@ end describe '.object_from_id' do - context 'for subclasses of `ApplicationRecord`' do + context 'with subclasses of `ApplicationRecord`' do let_it_be(:user) { create(:user) } it 'returns the correct record' do @@ -162,7 +157,7 @@ end end - context 'for classes that are not ActiveRecord subclasses and have implemented .lazy_find' do + context 'with classes that are not ActiveRecord subclasses and have implemented .lazy_find' do it 'returns the correct record' do note = create(:discussion_note_on_merge_request) @@ -182,7 +177,7 @@ end end - context 'for other classes' do + context 'with other classes' do # We cannot use an anonymous class here as `GlobalID` expects `.name` not # to return `nil` before do diff --git a/spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb b/spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1d8849f7e38d5926d830fe973979035507d046c7 --- /dev/null +++ b/spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Graphql::CallsGitaly::FieldExtension, :request_store do + include GraphqlHelpers + + let(:field_args) { {} } + let(:owner) { fresh_object_type } + let(:field) do + ::Types::BaseField.new(name: 'value', type: GraphQL::STRING_TYPE, null: true, owner: owner, **field_args) + end + + def resolve_value + resolve_field(field, { value: 'foo' }, object_type: owner) + end + + context 'when the field calls gitaly' do + before do + owner.define_method :value do + Gitlab::SafeRequestStore['gitaly_call_actual'] = 1 + 'fresh-from-the-gitaly-mines!' + end + end + + context 'when the field has a constant complexity' do + let(:field_args) { { complexity: 100 } } + + it 'allows the call' do + expect { resolve_value }.not_to raise_error + end + end + + context 'when the field declares that it calls gitaly' do + let(:field_args) { { calls_gitaly: true } } + + it 'allows the call' do + expect { resolve_value }.not_to raise_error + end + end + + context 'when the field does not have these arguments' do + let(:field_args) { {} } + + it 'notices, and raises, mentioning the field' do + expect { resolve_value }.to raise_error(include('Object.value')) + end + end + end + + context 'when it does not call gitaly' do + let(:field_args) { {} } + + it 'does not raise' do + value = resolve_value + + expect(value).to eq 'foo' + end + end + + context 'when some field calls gitaly while we were waiting' do + let(:extension) { described_class.new(field: field, options: {}) } + + it 'is acceptable if all are accounted for' do + object = :anything + arguments = :any_args + + ::Gitlab::SafeRequestStore['gitaly_call_actual'] = 3 + ::Gitlab::SafeRequestStore['graphql_gitaly_accounted_for'] = 0 + + expect do |b| + extension.resolve(object: object, arguments: arguments, &b) + end.to yield_with_args(object, arguments, [3, 0]) + + ::Gitlab::SafeRequestStore['gitaly_call_actual'] = 13 + ::Gitlab::SafeRequestStore['graphql_gitaly_accounted_for'] = 10 + + expect { extension.after_resolve(value: 'foo', memo: [3, 0]) }.not_to raise_error + end + + it 'is unacceptable if some of the calls are unaccounted for' do + ::Gitlab::SafeRequestStore['gitaly_call_actual'] = 10 + ::Gitlab::SafeRequestStore['graphql_gitaly_accounted_for'] = 9 + + expect { extension.after_resolve(value: 'foo', memo: [0, 0]) }.to raise_error(include('Object.value')) + end + end +end diff --git a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb b/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb deleted file mode 100644 index f16767f7d14c4bc2cc4b12a6835a5cf98f44a318..0000000000000000000000000000000000000000 --- a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Gitlab::Graphql::CallsGitaly::Instrumentation do - subject { described_class.new } - - describe '#calls_gitaly_check' do - let(:gitaly_field) { Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true) } - let(:no_gitaly_field) { Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: false) } - - context 'if there are no Gitaly calls' do - it 'does not raise an error if calls_gitaly is false' do - expect { subject.send(:calls_gitaly_check, no_gitaly_field, 0) }.not_to raise_error - end - end - - context 'if there is at least 1 Gitaly call' do - it 'raises an error if calls_gitaly: is false or not defined' do - expect { subject.send(:calls_gitaly_check, no_gitaly_field, 1) }.to raise_error(/specify a constant complexity or add `calls_gitaly: true`/) - end - end - end -end diff --git a/spec/lib/gitlab/graphql/present/field_extension_spec.rb b/spec/lib/gitlab/graphql/present/field_extension_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..5e66e16d655a07ac18e264f472be79c68769df71 --- /dev/null +++ b/spec/lib/gitlab/graphql/present/field_extension_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Gitlab::Graphql::Present::FieldExtension do + include GraphqlHelpers + + let_it_be(:user) { create(:user) } + + let(:object) { double(value: 'foo') } + let(:owner) { fresh_object_type } + let(:field_name) { 'value' } + let(:field) do + ::Types::BaseField.new(name: field_name, type: GraphQL::STRING_TYPE, null: true, owner: owner) + end + + let(:base_presenter) do + Class.new(SimpleDelegator) do + def initialize(object, **options) + super(object) + @object = object + @options = options + end + end + end + + def resolve_value + resolve_field(field, object, current_user: user, object_type: owner) + end + + context 'when the object does not declare a presenter' do + it 'does not affect normal resolution' do + expect(resolve_value).to eq 'foo' + end + end + + describe 'interactions with inheritance' do + def parent + type = fresh_object_type('Parent') + type.present_using(provide_foo) + type.field :foo, ::GraphQL::INT_TYPE, null: true + type.field :value, ::GraphQL::STRING_TYPE, null: true + type + end + + def child + type = Class.new(parent) + type.graphql_name 'Child' + type.present_using(provide_bar) + type.field :bar, ::GraphQL::INT_TYPE, null: true + type + end + + def provide_foo + Class.new(base_presenter) do + def foo + 100 + end + end + end + + def provide_bar + Class.new(base_presenter) do + def bar + 101 + end + end + end + + it 'can resolve value, foo and bar' do + type = child + value = resolve_field(:value, object, object_type: type) + foo = resolve_field(:foo, object, object_type: type) + bar = resolve_field(:bar, object, object_type: type) + + expect([value, foo, bar]).to eq ['foo', 100, 101] + end + end + + shared_examples 'calling the presenter method' do + it 'calls the presenter method' do + expect(resolve_value).to eq presenter.new(object, current_user: user).send(field_name) + end + end + + context 'when the object declares a presenter' do + before do + owner.present_using(presenter) + end + + context 'when the presenter overrides the original method' do + def twice + Class.new(base_presenter) do + def value + @object.value * 2 + end + end + end + + let(:presenter) { twice } + + it_behaves_like 'calling the presenter method' + end + + # This is exercised here using an explicit `resolve:` proc, but + # @resolver_proc values are used in field instrumentation as well. + context 'when the field uses a resolve proc' do + let(:presenter) { base_presenter } + let(:field) do + ::Types::BaseField.new( + name: field_name, + type: GraphQL::STRING_TYPE, + null: true, + owner: owner, + resolve: ->(obj, args, ctx) { 'Hello from a proc' } + ) + end + + specify { expect(resolve_value).to eq 'Hello from a proc' } + end + + context 'when the presenter provides a new method' do + def presenter + Class.new(base_presenter) do + def current_username + "Hello #{@options[:current_user]&.username} from the presenter!" + end + end + end + + context 'when we select the original field' do + it 'is unaffected' do + expect(resolve_value).to eq 'foo' + end + end + + context 'when we select the new field' do + let(:field_name) { 'current_username' } + + it_behaves_like 'calling the presenter method' + end + end + end +end diff --git a/spec/requests/api/graphql/issue/issue_spec.rb b/spec/requests/api/graphql/issue/issue_spec.rb index 09e89f6588220338f293cfa2a47d191827a2346f..e8b8caf6c2da632f43d371ef386fd47b7bbf7533 100644 --- a/spec/requests/api/graphql/issue/issue_spec.rb +++ b/spec/requests/api/graphql/issue/issue_spec.rb @@ -8,10 +8,9 @@ let_it_be(:project) { create(:project) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:current_user) { create(:user) } + let_it_be(:issue_params) { { 'id' => issue.to_global_id.to_s } } let(:issue_data) { graphql_data['issue'] } - - let_it_be(:issue_params) { { 'id' => issue.to_global_id.to_s } } let(:issue_fields) { all_graphql_fields_for('Issue'.classify) } let(:query) do @@ -62,7 +61,7 @@ def query(fields) ) end - context 'selecting any single field' do + context 'when selecting any single field' do where(:field) do scalar_fields_of('Issue').map { |name| [name] } end @@ -84,13 +83,13 @@ def query(fields) end end - context 'selecting multiple fields' do + context 'when selecting multiple fields' do let(:issue_fields) { ['title', 'description', 'updatedBy { username }'] } it 'returns the Issue with the specified fields' do post_graphql(query, current_user: current_user) - expect(issue_data.keys).to eq( %w(title description updatedBy) ) + expect(issue_data.keys).to eq %w[title description updatedBy] expect(issue_data['title']).to eq(issue.title) expect(issue_data['description']).to eq(issue.description) expect(issue_data['updatedBy']['username']).to eq(issue.author.username) @@ -110,14 +109,14 @@ def query(fields) it 'returns correct attributes' do post_graphql(query, current_user: current_user) - expect(issue_data.keys).to eq( %w(moved movedTo) ) + expect(issue_data.keys).to eq %w[moved movedTo] expect(issue_data['moved']).to eq(true) expect(issue_data['movedTo']['title']).to eq(new_issue.title) end end context 'when passed a non-Issue gid' do - let(:mr) {create(:merge_request)} + let(:mr) { create(:merge_request) } it 'returns an error' do gid = mr.to_global_id.to_s diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 0e05ca320ed382e4558b997c268765611867714a..75d9508f4708381c21ff172c2b5402e43349a9be 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -38,7 +38,10 @@ def resolve( # All resolution goes through fields, so we need to create one here that # uses our resolver. Thankfully, apart from the field name, resolvers # contain all the configuration needed to define one. - field_options = resolver_class.field_options.merge(name: 'field_value') + field_options = resolver_class.field_options.merge( + owner: resolver_parent, + name: 'field_value' + ) field = ::Types::BaseField.new(**field_options) # All mutations accept a single `:input` argument. Wrap arguments here.