From 3b96d44119035ee3226c8eeb433a30a3ccdaf673 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Thu, 4 Mar 2021 12:00:13 +0000 Subject: [PATCH 1/8] Only pass kwargs to Schema::Field.new --- app/graphql/types/base_field.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index f85675f72f265d..4eee7e4ff47113 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -9,7 +9,7 @@ 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] @requires_argument = !!kwargs.delete(:requires_argument) @@ -18,7 +18,7 @@ def initialize(*args, **kwargs, &block) kwargs = check_feature_flag(kwargs) kwargs = gitlab_deprecation(kwargs) - super(*args, **kwargs, &block) + super(**kwargs, &block) end def requires_argument? -- GitLab From a379ab207f41c441b633caa3a44b2d866de8da42 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Sat, 27 Feb 2021 22:33:36 +0000 Subject: [PATCH 2/8] Fix outstanding rubocop violations in BaseField These were discovered using REVEAL_RUBOCOP_TODO --- app/graphql/types/base_field.rb | 12 ++++++++---- spec/graphql/gitlab_schema_spec.rb | 17 ++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 4eee7e4ff47113..a3593cb2c351e8 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -54,8 +54,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 +80,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 +97,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/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index 4db1264306923f..f2dfe4886507d6 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -47,7 +47,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 +78,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 +96,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 +133,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 +165,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 +185,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 -- GitLab From 0d510961c133435a5f985005ae94730ffc762cd9 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 26 Feb 2021 13:49:28 +0000 Subject: [PATCH 3/8] Convert calls-gitaly graphql tooling to field extension The calls-gitaly tooling is not available when calling `GraphqlHelpers#resolve` and `#resolve_field`, because it was implemented as schema instrumentation which must be called from `Schema.execute`. This changes to use field extensions that are: - simpler (less code, easier to read) - can be applied selectively (here we can avoid the runtime cost in prod) - not deprecated; to use the intepreter, we will need to make this move Changes: - Adds `#may_call_gitaly` method to BaseField --- app/graphql/gitlab_schema.rb | 1 - app/graphql/types/base_field.rb | 9 ++ lib/gitlab/graphql/calls_gitaly.rb | 15 ---- .../graphql/calls_gitaly/field_extension.rb | 87 +++++++++++++++++++ .../graphql/calls_gitaly/instrumentation.rb | 40 --------- spec/graphql/gitlab_schema_spec.rb | 4 - .../calls_gitaly/field_extension_spec.rb | 87 +++++++++++++++++++ .../calls_gitaly/instrumentation_spec.rb | 23 ----- 8 files changed, 183 insertions(+), 83 deletions(-) delete mode 100644 lib/gitlab/graphql/calls_gitaly.rb create mode 100644 lib/gitlab/graphql/calls_gitaly/field_extension.rb delete mode 100644 lib/gitlab/graphql/calls_gitaly/instrumentation.rb create mode 100644 spec/lib/gitlab/graphql/calls_gitaly/field_extension_spec.rb delete mode 100644 spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index d66a2333d119a9..ab7a9068cb8f5a 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -14,7 +14,6 @@ class GitlabSchema < GraphQL::Schema 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/types/base_field.rb b/app/graphql/types/base_field.rb index a3593cb2c351e8..8c751846dfce97 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -19,6 +19,15 @@ def initialize(**kwargs, &block) kwargs = gitlab_deprecation(kwargs) super(**kwargs, &block) + + # We want to avoid the overhead of this in prod + unless Rails.env.production? + extension ::Gitlab::Graphql::CallsGitaly::FieldExtension + end + end + + def may_call_gitaly? + @constant_complexity || @calls_gitaly end def requires_argument? diff --git a/lib/gitlab/graphql/calls_gitaly.rb b/lib/gitlab/graphql/calls_gitaly.rb deleted file mode 100644 index 40cd74a34f2ef2..00000000000000 --- 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 00000000000000..8f7ae26308a0a3 --- /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_type.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 11d3c50e093337..00000000000000 --- 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/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index f2dfe4886507d6..4d26c4332f3e71 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -22,10 +22,6 @@ 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 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 00000000000000..1d8849f7e38d59 --- /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 f16767f7d14c4b..00000000000000 --- 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 -- GitLab From e300fad5df8352369998e06436eeb8b87a0afc27 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 26 Feb 2021 23:30:51 +0000 Subject: [PATCH 4/8] Fix complexity calculation to allow gitaly call detection This was erroneously setting constant_complexity to true for all fields that have a resolver, since our BaseResolver sets complexity to 0 (and `!!zero == true`). This masks gitaly calls. --- app/graphql/types/base_field.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 8c751846dfce97..9f2622afa9a49e 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -11,7 +11,7 @@ class BaseField < GraphQL::Schema::Field 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] -- GitLab From 8d8ff5332dc91f95e95bfdbc3b3b87175135f6df Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Sat, 27 Feb 2021 00:06:12 +0000 Subject: [PATCH 5/8] Annotate resolvers that call gitaly The following resolvers call gitaly: - metrics/dashboard_resolver - last_commit_resolver - snippets/blobs_resolver - tree_resolver Remove an unnecessary gitaly annotation (I was unable to verify that this gitaly annotation was needed. So it is removed.) --- app/graphql/resolvers/base_resolver.rb | 11 +++++++++- app/graphql/resolvers/last_commit_resolver.rb | 2 ++ .../resolvers/metrics/dashboard_resolver.rb | 21 ++++++++++++++----- .../resolvers/snippets/blobs_resolver.rb | 1 + app/graphql/resolvers/tree_resolver.rb | 2 ++ app/graphql/types/snippets/blob_type.rb | 1 - app/graphql/types/tree/blob_type.rb | 1 + .../graphql/calls_gitaly/field_extension.rb | 2 +- spec/support/helpers/graphql_helpers.rb | 5 ++++- 9 files changed, 37 insertions(+), 9 deletions(-) diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb index 5db618254cbbfd..67bba079512cb9 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 dd89c322617043..00c43bdfee647e 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 f569cb0b2c3f78..a82a4a952545dd 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 672214df7d5b0d..569b82149d3d47 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 7a70c35897d0e8..c07d9187d4d810 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/snippets/blob_type.rb b/app/graphql/types/snippets/blob_type.rb index fb0c1d9409b70c..fb9ee3807054ff 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 3823bd94083229..d192c8d3c57224 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/field_extension.rb b/lib/gitlab/graphql/calls_gitaly/field_extension.rb index 8f7ae26308a0a3..32530b47ce3737 100644 --- a/lib/gitlab/graphql/calls_gitaly/field_extension.rb +++ b/lib/gitlab/graphql/calls_gitaly/field_extension.rb @@ -79,7 +79,7 @@ def accounted_for(count = nil) end def field_name - "#{field.owner_type.graphql_name}.#{field.graphql_name}" + "#{field.owner.graphql_name}.#{field.graphql_name}" end end end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 0e05ca320ed382..75d9508f470838 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. -- GitLab From 8feb5180c1683b8f602be83eccd102699775b083 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Thu, 4 Mar 2021 13:00:06 +0000 Subject: [PATCH 6/8] Prefer Gitlab.dev_or_test_env? to Rails.env predicates --- app/graphql/types/base_field.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 9f2622afa9a49e..b540dc8c34cc58 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -21,9 +21,7 @@ def initialize(**kwargs, &block) super(**kwargs, &block) # We want to avoid the overhead of this in prod - unless Rails.env.production? - extension ::Gitlab::Graphql::CallsGitaly::FieldExtension - end + extension ::Gitlab::Graphql::CallsGitaly::FieldExtension if Gitlab.dev_or_test_env? end def may_call_gitaly? -- GitLab From df6252d0cf825f1e7e994db61aa37a6718fd76bd Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 26 Feb 2021 13:51:05 +0000 Subject: [PATCH 7/8] Convert GQL presenter tool to field extension This changes the presenter instrumentation to a field extension, which is simpler, easier to maintain, and will allow us to make use of the interpreter execution engine in the future. This also makes is easier to test resolver and types that make use of these facilities. Other changes: In the current user TODOs type, ensure we take the type of the unwrapped object If we don't do this, the presented class name will be used instead (for types that use `present_using`). This results in incorrect todo_type values. --- app/graphql/gitlab_schema.rb | 1 - app/graphql/types/base_field.rb | 2 + app/graphql/types/current_user_todos.rb | 5 +- lib/gitlab/graphql/present.rb | 23 ++- lib/gitlab/graphql/present/field_extension.rb | 35 +++++ lib/gitlab/graphql/present/instrumentation.rb | 49 ------ spec/graphql/gitlab_schema_spec.rb | 4 - .../graphql/present/field_extension_spec.rb | 143 ++++++++++++++++++ spec/requests/api/graphql/issue/issue_spec.rb | 13 +- 9 files changed, 210 insertions(+), 65 deletions(-) create mode 100644 lib/gitlab/graphql/present/field_extension.rb delete mode 100644 lib/gitlab/graphql/present/instrumentation.rb create mode 100644 spec/lib/gitlab/graphql/present/field_extension_spec.rb diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index ab7a9068cb8f5a..7ab5dc36e4a446 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -13,7 +13,6 @@ class GitlabSchema < GraphQL::Schema use GraphQL::Pagination::Connections use BatchLoader::GraphQL use Gitlab::Graphql::Authorize - use Gitlab::Graphql::Present 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/types/base_field.rb b/app/graphql/types/base_field.rb index b540dc8c34cc58..78ab689092336a 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -22,6 +22,8 @@ def initialize(**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? diff --git a/app/graphql/types/current_user_todos.rb b/app/graphql/types/current_user_todos.rb index 79a430af1d7e63..2551db875b0fb6 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/lib/gitlab/graphql/present.rb b/lib/gitlab/graphql/present.rb index 6d86d632ab4455..fdaf075eb256d7 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 00000000000000..52201eeb243e92 --- /dev/null +++ b/lib/gitlab/graphql/present/field_extension.rb @@ -0,0 +1,35 @@ +# 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. + 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 b8535575da5d8e..00000000000000 --- 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 4d26c4332f3e71..cb2bb25b098c22 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -18,10 +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 'has the base mutation' do expect(described_class.mutation).to eq(::Types::MutationType) 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 00000000000000..5e66e16d655a07 --- /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 09e89f65882203..e8b8caf6c2da63 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 -- GitLab From f5c82c288674919c939fee934572888a1cd50575 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Thu, 11 Mar 2021 09:10:16 +0000 Subject: [PATCH 8/8] Add issue reference --- lib/gitlab/graphql/present/field_extension.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/graphql/present/field_extension.rb b/lib/gitlab/graphql/present/field_extension.rb index 52201eeb243e92..2e211b70d35f47 100644 --- a/lib/gitlab/graphql/present/field_extension.rb +++ b/lib/gitlab/graphql/present/field_extension.rb @@ -19,6 +19,7 @@ def resolve(object:, arguments:, context:) # 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 -- GitLab