Commit 675c9b9f authored by charlieablett's avatar charlieablett

Address reviewer comments

- Remove Gitaly call check for fields that have a constant complexity
declared
- Add associated test
parent cf1b0d10
......@@ -8,6 +8,7 @@ module Types
def initialize(*args, **kwargs, &block)
@calls_gitaly = !!kwargs.delete(:calls_gitaly)
@constant_complexity = !!kwargs[:complexity]
kwargs[:complexity] ||= field_complexity(kwargs[:resolver_class])
super(*args, **kwargs, &block)
......@@ -23,6 +24,10 @@ module Types
@calls_gitaly
end
def constant_complexity?
@constant_complexity
end
private
def field_complexity(resolver_class)
......
......@@ -7,7 +7,7 @@ module Types
graphql_name 'Tree'
# Complexity 10 as it triggers a Gitaly call on each render
field :last_commit, Types::CommitType, null: true, complexity: 10, resolve: -> (tree, args, ctx) do
field :last_commit, Types::CommitType, null: true, complexity: 10, calls_gitaly: true, resolve: -> (tree, args, ctx) do
tree.repository.last_commit_for_path(tree.sha, tree.path)
end
......
......@@ -2,8 +2,8 @@
module Gitlab
module Graphql
# Allow fields to declare permissions their objects must have. The field
# will be set to nil unless all required permissions are present.
# 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
......
......@@ -5,9 +5,11 @@ module Gitlab
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 && type_object.respond_to?(:calls_gitaly?)
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
......@@ -25,12 +27,11 @@ module Gitlab
end
def calls_gitaly_check(type_object, calls)
return if type_object.calls_gitaly?
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 add `calls_gitaly: true` to the field declaration")
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::Sentry.track_exception(error)
end
end
......
......@@ -96,10 +96,19 @@ describe Types::BaseField do
expect(field.base_complexity).to eq Types::BaseField::DEFAULT_COMPLEXITY
end
it 'is overridden by declared complexity value' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true, complexity: 12)
context 'with declared constant complexity value' do
it 'has complexity set to that constant' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, complexity: 12)
expect(field.to_graphql.complexity).to eq 12
expect(field.to_graphql.complexity).to eq 12
end
it 'does not raise an error even with Gitaly calls' do
allow(Gitlab::GitalyClient).to receive(:get_request_count).and_return([0, 1])
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, complexity: 12)
expect(field.to_graphql.complexity).to eq 12
end
end
end
end
......
......@@ -4,25 +4,19 @@ require 'spec_helper'
describe Gitlab::Graphql::CallsGitaly::Instrumentation do
subject { described_class.new }
context 'when considering complexity' do
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) }
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
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 'does not raise an error if calls_gitaly is true' do
expect { subject.send(:calls_gitaly_check, gitaly_field, 1) }.not_to raise_error
end
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(/please add `calls_gitaly: true`/)
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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment