From 94a2e7c8840e2810ce87654163a2a975ca1f76eb Mon Sep 17 00:00:00 2001
From: Luke Duncalfe <lduncalfe@eml.cc>
Date: Thu, 18 Aug 2022 12:53:08 +1200
Subject: [PATCH] Remove GraphQL `feature_flag` property
The property had been renamed to `_deprecated_feature_flag` earlier.
This change removes it altogether and all code that was supporting it.
https://gitlab.com/gitlab-org/gitlab/-/issues/369202
---
app/graphql/types/base_field.rb | 31 ------
doc/development/api_graphql_styleguide.md | 6 --
rubocop/code_reuse_helpers.rb | 6 --
rubocop/cop/gitlab/mark_used_feature_flags.rb | 20 +---
spec/graphql/features/feature_flag_spec.rb | 52 ----------
spec/graphql/types/base_field_spec.rb | 99 -------------------
spec/rubocop/code_reuse_helpers_spec.rb | 25 -----
.../gitlab/mark_used_feature_flags_spec.rb | 13 ---
8 files changed, 2 insertions(+), 250 deletions(-)
delete mode 100644 spec/graphql/features/feature_flag_spec.rb
diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb
index 1c43432594a20..6f64e5b5053e8 100644
--- a/app/graphql/types/base_field.rb
+++ b/app/graphql/types/base_field.rb
@@ -17,8 +17,6 @@ def initialize(**kwargs, &block)
@requires_argument = !!kwargs.delete(:requires_argument)
@authorize = Array.wrap(kwargs.delete(:authorize))
kwargs[:complexity] = field_complexity(kwargs[:resolver_class], kwargs[:complexity])
- @feature_flag = kwargs[:_deprecated_feature_flag]
- kwargs = check_feature_flag(kwargs)
@deprecation = gitlab_deprecation(kwargs)
after_connection_extensions = kwargs.delete(:late_extensions) || []
@@ -91,16 +89,8 @@ def constant_complexity?
@constant_complexity
end
- def visible?(context)
- return false if feature_flag.present? && !Feature.enabled?(feature_flag)
-
- super
- end
-
private
- attr_reader :feature_flag
-
def field_authorized?(object, ctx)
object = object.node if object.is_a?(GraphQL::Pagination::Connection::Edge)
@@ -123,27 +113,6 @@ def authorization
@authorization ||= ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(@authorize)
end
- def feature_documentation_message(key, description)
- message_parts = ["#{description} Available only when feature flag `#{key}` is enabled."]
-
- message_parts << if Feature::Definition.has_definition?(key) && Feature::Definition.default_enabled?(key)
- "This flag is enabled by default."
- else
- "This flag is disabled by default, because the feature is experimental and is subject to change without notice."
- end
-
- message_parts.join(' ')
- end
-
- def check_feature_flag(args)
- ff = args.delete(:_deprecated_feature_flag)
- return args unless ff.present?
-
- args[:description] = feature_documentation_message(ff, args[:description])
-
- args
- end
-
def field_complexity(resolver_class, current)
return current if current.present? && current > 0
diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md
index 190e853a37663..78ff8fbba36c9 100644
--- a/doc/development/api_graphql_styleguide.md
+++ b/doc/development/api_graphql_styleguide.md
@@ -546,12 +546,6 @@ def resolve(id: )
end
```
-### `feature_flag` property (deprecated)
-
-NOTE:
-This property is deprecated and should no longer be used. The property
-has been temporarily renamed to `_deprecated_feature_flag` and support for it will be removed in [#369202](https://gitlab.com/gitlab-org/gitlab/-/issues/369202).
-
## Deprecating schema items
The GitLab GraphQL API is versionless, which means we maintain backwards
diff --git a/rubocop/code_reuse_helpers.rb b/rubocop/code_reuse_helpers.rb
index 2769da2389c82..87b907f87786b 100644
--- a/rubocop/code_reuse_helpers.rb
+++ b/rubocop/code_reuse_helpers.rb
@@ -81,12 +81,6 @@ def in_graphql?(node)
in_app_directory?(node, 'graphql')
end
- # Returns true if the given node resides in app/graphql/types,
- # ee/app/graphql/types, or ee/app/graphql/ee/types.
- def in_graphql_types?(node)
- in_graphql_directory?(node, 'types')
- end
-
# Returns true if the given node resides in lib/api or ee/lib/api.
def in_api?(node)
in_lib_directory?(node, 'api')
diff --git a/rubocop/cop/gitlab/mark_used_feature_flags.rb b/rubocop/cop/gitlab/mark_used_feature_flags.rb
index 63bccec31c056..4844dab0a30b3 100644
--- a/rubocop/cop/gitlab/mark_used_feature_flags.rb
+++ b/rubocop/cop/gitlab/mark_used_feature_flags.rb
@@ -26,9 +26,6 @@ class MarkUsedFeatureFlags < RuboCop::Cop::Cop
data_consistency
deduplicate
].freeze
- GRAPHQL_METHODS = %i[
- field
- ].freeze
SELF_METHODS = %i[
push_frontend_feature_flag
push_force_frontend_feature_flag
@@ -36,7 +33,7 @@ class MarkUsedFeatureFlags < RuboCop::Cop::Cop
limit_feature_flag_for_override=
].freeze + EXPERIMENT_METHODS + RUGGED_METHODS + WORKER_METHODS
- RESTRICT_ON_SEND = FEATURE_METHODS + EXPERIMENTATION_METHODS + GRAPHQL_METHODS + SELF_METHODS
+ RESTRICT_ON_SEND = FEATURE_METHODS + EXPERIMENTATION_METHODS + SELF_METHODS
USAGE_DATA_COUNTERS_EVENTS_YAML_GLOBS = [
File.expand_path("../../../config/metrics/aggregates/*.yml", __dir__),
@@ -138,15 +135,6 @@ def flag_arg(node)
node.children[3].each_pair.find do |pair|
pair.key.value == :feature_flag
end&.value
- elsif graphql_method?(node)
- return unless node.children.size > 3
-
- opts_index = node.children[3].hash_type? ? 3 : 4
- return unless node.children[opts_index]
-
- node.children[opts_index].each_pair.find do |pair|
- pair.key.value == :_deprecated_feature_flag
- end&.value
else
arg_index = rugged_method?(node) ? 3 : 2
@@ -209,16 +197,12 @@ def worker_method?(node)
WORKER_METHODS.include?(method_name(node))
end
- def graphql_method?(node)
- GRAPHQL_METHODS.include?(method_name(node)) && in_graphql_types?(node)
- end
-
def self_method?(node)
SELF_METHODS.include?(method_name(node)) && class_caller(node).empty?
end
def trackable_flag?(node)
- feature_method?(node) || experimentation_method?(node) || graphql_method?(node) || self_method?(node)
+ feature_method?(node) || experimentation_method?(node) || self_method?(node)
end
# Marking all event's feature flags as used as Gitlab::UsageDataCounters::HLLRedisCounter.track_event{,context}
diff --git a/spec/graphql/features/feature_flag_spec.rb b/spec/graphql/features/feature_flag_spec.rb
deleted file mode 100644
index b06718eb16ae8..0000000000000
--- a/spec/graphql/features/feature_flag_spec.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'Graphql Field feature flags' do
- include GraphqlHelpers
- include Graphql::ResolverFactories
-
- let_it_be(:user) { create(:user) }
-
- let(:feature_flag) { 'test_feature' }
- let(:test_object) { double(name: 'My name') }
- let(:query_string) { '{ item { name } }' }
- let(:result) { execute_query(query_type)['data'] }
-
- before do
- skip_feature_flags_yaml_validation
- end
-
- subject { result }
-
- describe 'Feature flagged field' do
- let(:type) { type_factory }
-
- let(:query_type) do
- query_factory do |query|
- query.field :item, type, null: true, _deprecated_feature_flag: feature_flag, resolver: new_resolver(test_object)
- end
- end
-
- it 'checks YAML definition for default_enabled' do
- # Exception is indicative of a check for YAML definition
- expect { subject }.to raise_error(Feature::InvalidFeatureFlagError, /The feature flag YAML definition for '#{feature_flag}' does not exist/)
- end
-
- context 'skipping YAML check' do
- before do
- skip_default_enabled_yaml_check
- end
-
- it 'returns the value when feature is enabled' do
- expect(subject['item']).to eq('name' => test_object.name)
- end
-
- it 'returns nil when the feature is disabled' do
- stub_feature_flags(feature_flag => false)
-
- expect(subject).to be_nil
- end
- end
- end
-end
diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb
index b85716e4d2114..9f8a8717efbd4 100644
--- a/spec/graphql/types/base_field_spec.rb
+++ b/spec/graphql/types/base_field_spec.rb
@@ -205,39 +205,6 @@ def self.complexity_multiplier(args)
end
end
end
-
- describe '#visible?' do
- context 'and has a feature_flag' do
- let(:flag) { :test_feature }
- let(:field) { described_class.new(name: 'test', type: GraphQL::Types::String, _deprecated_feature_flag: flag, null: false) }
- let(:context) { {} }
-
- before do
- skip_feature_flags_yaml_validation
- end
-
- it 'checks YAML definition for default_enabled' do
- # Exception is indicative of a check for YAML definition
- expect { field.visible?(context) }.to raise_error(Feature::InvalidFeatureFlagError, /The feature flag YAML definition for '#{flag}' does not exist/)
- end
-
- context 'skipping YAML check' do
- before do
- skip_default_enabled_yaml_check
- end
-
- it 'returns false if the feature is not enabled' do
- stub_feature_flags(flag => false)
-
- expect(field.visible?(context)).to eq(false)
- end
-
- it 'returns true if the feature is enabled' do
- expect(field.visible?(context)).to eq(true)
- end
- end
- end
- end
end
describe '#resolve' do
@@ -251,77 +218,11 @@ def self.complexity_multiplier(args)
end
end
- describe '#description' do
- context 'feature flag given' do
- let(:field) { described_class.new(name: 'test', type: GraphQL::Types::String, _deprecated_feature_flag: flag, null: false, description: 'Test description.') }
- let(:flag) { :test_flag }
-
- it 'prepends the description' do
- expect(field.description).to start_with 'Test description. Available only when feature flag `test_flag` is enabled.'
- end
-
- context 'falsey feature_flag values' do
- using RSpec::Parameterized::TableSyntax
-
- where(:flag, :feature_value, :default_enabled) do
- '' | false | false
- '' | true | false
- nil | false | true
- nil | true | false
- end
-
- with_them do
- it 'returns the correct description' do
- expect(field.description).to eq('Test description.')
- end
- end
- end
-
- context 'with different default_enabled values' do
- using RSpec::Parameterized::TableSyntax
-
- where(:feature_value, :default_enabled, :expected_description) do
- disabled_ff_description = "Test description. Available only when feature flag `test_flag` is enabled. This flag is disabled by default, because the feature is experimental and is subject to change without notice."
- enabled_ff_description = "Test description. Available only when feature flag `test_flag` is enabled. This flag is enabled by default."
-
- false | false | disabled_ff_description
- true | false | disabled_ff_description
- false | true | enabled_ff_description
- true | true | enabled_ff_description
- end
-
- with_them do
- before do
- stub_feature_flags("#{flag}": feature_value)
-
- allow(Feature::Definition).to receive(:has_definition?).with(flag).and_return(true)
- allow(Feature::Definition).to receive(:default_enabled?).and_return(default_enabled)
- end
-
- it 'returns the correct availability in the description' do
- expect(field.description).to eq expected_description
- end
- end
- end
- end
- end
-
include_examples 'Gitlab-style deprecations' do
def subject(args = {})
base_args = { name: 'test', type: GraphQL::Types::String, null: true }
described_class.new(**base_args.merge(args))
end
-
- it 'interacts well with the `_deprecated_feature_flag` property' do
- field = subject(
- deprecated: { milestone: '1.10', reason: 'Deprecation reason' },
- description: 'Field description.',
- _deprecated_feature_flag: 'foo_flag'
- )
-
- expect(field.description).to start_with('Field description. Available only when feature flag `foo_flag` is enabled.')
- expect(field.description).to end_with('Deprecated in 1.10: Deprecation reason.')
- end
end
end
diff --git a/spec/rubocop/code_reuse_helpers_spec.rb b/spec/rubocop/code_reuse_helpers_spec.rb
index 5cc396a4e3ef0..a8f935426e693 100644
--- a/spec/rubocop/code_reuse_helpers_spec.rb
+++ b/spec/rubocop/code_reuse_helpers_spec.rb
@@ -172,31 +172,6 @@ def build_and_parse_source(source, path = 'foo.rb')
end
end
- describe '#in_graphql_types?' do
- %w[
- app/graphql/types
- ee/app/graphql/ee/types
- ee/app/graphql/types
- ].each do |path|
- it "returns true for a node in #{path}" do
- node = build_and_parse_source('10', rails_root_join(path, 'foo.rb'))
-
- expect(cop.in_graphql_types?(node)).to eq(true)
- end
- end
-
- %w[
- app/graphql/resolvers
- app/foo
- ].each do |path|
- it "returns false for a node in #{path}" do
- node = build_and_parse_source('10', rails_root_join(path, 'foo.rb'))
-
- expect(cop.in_graphql_types?(node)).to eq(false)
- end
- end
- end
-
describe '#in_api?' do
it 'returns true for a node in the API directory' do
node = build_and_parse_source('10', rails_root_join('lib', 'api', 'foo.rb'))
diff --git a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
index 5f72996a06a00..c61376d77607a 100644
--- a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
+++ b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
@@ -212,19 +212,6 @@ class Foo < ApplicationRecord
include_examples 'does not set any flags as used', 'deduplicate :delayed'
end
- describe 'GraphQL `field` method' do
- before do
- allow(cop).to receive(:in_graphql_types?).and_return(true)
- end
-
- include_examples 'sets flag as used', 'field :runners, Types::Ci::RunnerType.connection_type, null: true, _deprecated_feature_flag: :foo', 'foo'
- include_examples 'sets flag as used', 'field :runners, null: true, _deprecated_feature_flag: :foo', 'foo'
- include_examples 'does not set any flags as used', 'field :solution'
- include_examples 'does not set any flags as used', 'field :runners, Types::Ci::RunnerType.connection_type'
- include_examples 'does not set any flags as used', 'field :runners, Types::Ci::RunnerType.connection_type, null: true, description: "hello world"'
- include_examples 'does not set any flags as used', 'field :solution, type: GraphQL::Types::String, null: true, description: "URL to the vulnerabilitys details page."'
- end
-
describe "tracking of usage data metrics known events happens at the beginning of inspection" do
let(:usage_data_counters_known_event_feature_flags) { ['an_event_feature_flag'] }
--
GitLab