From 03bae88d61b8525e6a32586eca194d6e4aea1ac9 Mon Sep 17 00:00:00 2001
From: Luke Duncalfe <lduncalfe@gitlab.com>
Date: Thu, 4 Aug 2022 19:45:21 +0000
Subject: [PATCH] Deprecate GraphQL `feature_flag` argument
This first iteration of
https://gitlab.com/gitlab-org/gitlab/-/issues/369202 documents the
`feature_flag` property as being deprecated and that it should not be
used.
It renames the argument so that it's clear it's not to be used for new
items.
It also updates the developer docs to include advice for using flags
with schema items besides fields, and where the flag is used to toggle
behavior.
---
app/graphql/types/base_field.rb | 4 +-
app/graphql/types/group_type.rb | 2 +-
app/graphql/types/mutation_type.rb | 4 +-
app/graphql/types/namespace_type.rb | 2 +-
app/graphql/types/project_type.rb | 2 +-
doc/development/api_graphql_styleguide.md | 118 ++++++------------
doc/development/fe_guide/graphql.md | 2 +-
.../types/boards/epic_list_metadata_type.rb | 2 +-
ee/app/graphql/types/geo/geo_node_type.rb | 2 +-
rubocop/cop/gitlab/mark_used_feature_flags.rb | 2 +-
spec/graphql/features/feature_flag_spec.rb | 2 +-
spec/graphql/types/base_field_spec.rb | 8 +-
.../gitlab/mark_used_feature_flags_spec.rb | 4 +-
13 files changed, 58 insertions(+), 96 deletions(-)
diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb
index 6aee9a5c05246..1c43432594a20 100644
--- a/app/graphql/types/base_field.rb
+++ b/app/graphql/types/base_field.rb
@@ -17,7 +17,7 @@ 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[:feature_flag]
+ @feature_flag = kwargs[:_deprecated_feature_flag]
kwargs = check_feature_flag(kwargs)
@deprecation = gitlab_deprecation(kwargs)
after_connection_extensions = kwargs.delete(:late_extensions) || []
@@ -136,7 +136,7 @@ def feature_documentation_message(key, description)
end
def check_feature_flag(args)
- ff = args.delete(:feature_flag)
+ ff = args.delete(:_deprecated_feature_flag)
return args unless ff.present?
args[:description] = feature_documentation_message(ff, args[:description])
diff --git a/app/graphql/types/group_type.rb b/app/graphql/types/group_type.rb
index 9cd6dddca09f3..d41b7c34302b1 100644
--- a/app/graphql/types/group_type.rb
+++ b/app/graphql/types/group_type.rb
@@ -22,7 +22,7 @@ class GroupType < NamespaceType
type: Types::CustomEmojiType.connection_type,
null: true,
description: 'Custom emoji within this namespace.',
- feature_flag: :custom_emoji
+ _deprecated_feature_flag: :custom_emoji
field :share_with_group_lock,
type: GraphQL::Types::Boolean,
diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb
index dc9eb369dc890..d677efa349e89 100644
--- a/app/graphql/types/mutation_type.rb
+++ b/app/graphql/types/mutation_type.rb
@@ -37,8 +37,8 @@ class MutationType < BaseObject
mount_mutation Mutations::Clusters::AgentTokens::Create
mount_mutation Mutations::Clusters::AgentTokens::Revoke
mount_mutation Mutations::Commits::Create, calls_gitaly: true
- mount_mutation Mutations::CustomEmoji::Create, feature_flag: :custom_emoji
- mount_mutation Mutations::CustomEmoji::Destroy, feature_flag: :custom_emoji
+ mount_mutation Mutations::CustomEmoji::Create, _deprecated_feature_flag: :custom_emoji
+ mount_mutation Mutations::CustomEmoji::Destroy, _deprecated_feature_flag: :custom_emoji
mount_mutation Mutations::CustomerRelations::Contacts::Create
mount_mutation Mutations::CustomerRelations::Contacts::Update
mount_mutation Mutations::CustomerRelations::Organizations::Create
diff --git a/app/graphql/types/namespace_type.rb b/app/graphql/types/namespace_type.rb
index b90226bd733fa..8a50dae4a3ea3 100644
--- a/app/graphql/types/namespace_type.rb
+++ b/app/graphql/types/namespace_type.rb
@@ -61,7 +61,7 @@ class NamespaceType < BaseObject
Types::TimeTracking::TimelogCategoryType.connection_type,
null: true,
description: "Timelog categories for the namespace.",
- feature_flag: :timelog_categories
+ _deprecated_feature_flag: :timelog_categories
markdown_field :description_html, null: true
diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb
index 5bb8e613844ab..c9223577b07ce 100644
--- a/app/graphql/types/project_type.rb
+++ b/app/graphql/types/project_type.rb
@@ -442,7 +442,7 @@ class ProjectType < BaseObject
Types::TimeTracking::TimelogCategoryType.connection_type,
null: true,
description: "Timelog categories for the project.",
- feature_flag: :timelog_categories
+ _deprecated_feature_flag: :timelog_categories
def timelog_categories
object.project_namespace.timelog_categories
diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md
index 7fe49e0c8f0ce..3baf5c92da819 100644
--- a/doc/development/api_graphql_styleguide.md
+++ b/doc/development/api_graphql_styleguide.md
@@ -98,11 +98,8 @@ See the [deprecating schema items](#deprecating-schema-items) section for how to
### Breaking change exemptions
-Two scenarios exist where schema items are exempt from the deprecation process,
-and can be removed or changed at any time without notice. These are schema items that either:
-
-- Use the [`feature_flag` property](#feature_flag-property) _and_ the flag is disabled by default.
-- Are [marked as alpha](#mark-schema-items-as-alpha).
+Schema items [marked as alpha](#mark-schema-items-as-alpha) are exempt from the deprecation process,
+and can be removed or changed at any time without notice.
## Global IDs
@@ -472,92 +469,37 @@ end
## Feature flags
-Developers can add [feature flags](../development/feature_flags/index.md) to GraphQL
-fields in the following ways:
-
-- Add the [`feature_flag` property](#feature_flag-property) to a field. This allows the field to be _hidden_
- from the GraphQL schema when the flag is disabled.
-- [Toggle the return value](#toggle-the-value-of-a-field) when resolving the field.
-
-You can refer to these guidelines to decide which approach to use:
-
-- If your field is experimental, and its name or type is subject to
- change, use the [`feature_flag` property](#feature_flag-property).
-- If your field is stable and its definition doesn't change, even after the flag is
- removed, [toggle the return value](#toggle-the-value-of-a-field) of the field instead. Note that
- [all fields should be nullable](#nullable-fields) anyway.
-- If your field will be accessed from frontend using the `@include` or `@skip` directive, [do not use the `feature_flag` property](#frontend-and-backend-feature-flag-strategies).
-
-### `feature_flag` property
-
-The `feature_flag` property allows you to toggle the field's
-[visibility](https://graphql-ruby.org/authorization/visibility.html)
-in the GraphQL schema. This removes the field from the schema
-when the flag is disabled.
-
-A description is [appended](https://gitlab.com/gitlab-org/gitlab/-/blob/497b556/app/graphql/types/base_field.rb#L44-53)
-to the field indicating that it is behind a feature flag.
-
-WARNING:
-If a client queries for the field when the feature flag is disabled, the query
-fails. Consider this when toggling the visibility of the feature on or off on
-production.
-
-The `feature_flag` property does not allow the use of
-[feature gates based on actors](../development/feature_flags/index.md).
-This means that the feature flag cannot be toggled only for particular
-projects, groups, or users, but instead can only be toggled globally for
-everyone.
-
-Example:
-
-```ruby
-field :test_field, type: GraphQL::Types::String,
- null: true,
- description: 'Some test field.',
- feature_flag: :my_feature_flag
-```
-
-### Frontend and Backend feature flag strategies
-
-#### Directives
+You can implement [feature flags](../development/feature_flags/index.md) in GraphQL to toggle:
-When feature flags are used in the frontend to control the `@include` and `@skip` directives, do not use the `feature_flag`
-property on the server-side. For the accepted backend workaround, see [Toggle the value of a field](#toggle-the-value-of-a-field). It is recommended that the feature flag used in this approach be the same for frontend and backend.
+- The return value of a field.
+- The behavior of an argument or mutation.
-Even if the frontend directives evaluate to `@include:false` / `@skip:true`, the guarded entity is sent to the backend and matched
-against the GraphQL schema. We would then get an exception due to a schema mismatch. See the [frontend GraphQL guide](../development/fe_guide/graphql.md#the-include-directive) for more guidance.
-
-#### Different versions of a query
-
-See the guide frontend GraphQL guide for [different versions of a query](../development/fe_guide/graphql.md#different-versions-of-a-query), and [why it is not the preferred approach](../development/fe_guide/graphql.md#avoiding-multiple-query-versions)
-
-### Toggle the value of a field
-
-This method of using feature flags for fields is to toggle the
-return value of the field. This can be done in the resolver, in the
+This can be done in a resolver, in the
type, or even in a model method, depending on your preference and
situation.
-Consider also [marking the field as Alpha](#mark-schema-items-as-alpha)
-while the value of the field can be toggled. You can
-[change or remove Alpha fields at any time](#breaking-change-exemptions) without needing to deprecate them.
-This also signals to consumers of the public GraphQL API that the field is not
+NOTE:
+It's recommended that you also [mark the item as Alpha](#mark-schema-items-as-alpha) while it is behind a feature flag.
+This signals to consumers of the public GraphQL API that the field is not
meant to be used yet.
+You can also
+[change or remove Alpha items at any time](#breaking-change-exemptions) without needing to deprecate them. When the flag is removed, "release"
+the schema item by removing its Alpha property to make it public.
-When applying a feature flag to toggle the value of a field, the
-`description` of the field must:
+### Descriptions for feature flagged items
-- State that the value of the field can be toggled by a feature flag.
+When using a feature flag to toggle the value or behavior of a schema item, the
+`description` of the item must:
+
+- State that the value or behavior can be toggled by a feature flag.
- Name the feature flag.
-- State what the field returns when the feature flag is disabled (or
+- State what the field returns, or behavior is, when the feature flag is disabled (or
enabled, if more appropriate).
-Example:
+Example of a feature-flagged field:
```ruby
-field :foo, GraphQL::Types::String,
- null: true,
+field :foo, GraphQL::Types::String, null: true,
alpha: { milestone: '10.0' },
description: 'Some test field. Returns `null`' \
'if `my_feature_flag` feature flag is disabled.'
@@ -567,6 +509,26 @@ def foo
end
```
+Example of a feature-flagged argument:
+
+```ruby
+argument :foo, type: GraphQL::Types::String, required: false,
+ alpha: { milestone: '10.0' },
+ description: 'Some test argument. Is ignored if ' \
+ '`my_feature_flag` feature flag is disabled.'
+
+def resolve(args)
+ args.delete(:foo) unless Feature.enabled?(:my_feature_flag, object)
+ # ...
+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/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md
index db267a6d5b3a4..500e9dfe62a83 100644
--- a/doc/development/fe_guide/graphql.md
+++ b/doc/development/fe_guide/graphql.md
@@ -597,7 +597,7 @@ export default {
Note that, even if the directive evaluates to `false`, the guarded entity is sent to the backend and
matched against the GraphQL schema. So this approach requires that the feature-flagged entity
exists in the schema, even if the feature flag is disabled. When the feature flag is turned off, it
-is recommended that the resolver returns `null` at the very least using the same feature flag as the frontend. See the [API GraphQL guide](../api_graphql_styleguide.md#frontend-and-backend-feature-flag-strategies).
+is recommended that the resolver returns `null` at the very least using the same feature flag as the frontend. See the [API GraphQL guide](../api_graphql_styleguide.md#feature-flags).
##### Different versions of a query
diff --git a/ee/app/graphql/types/boards/epic_list_metadata_type.rb b/ee/app/graphql/types/boards/epic_list_metadata_type.rb
index 797f2474bc487..22e2ab3c2affd 100644
--- a/ee/app/graphql/types/boards/epic_list_metadata_type.rb
+++ b/ee/app/graphql/types/boards/epic_list_metadata_type.rb
@@ -14,7 +14,7 @@ class EpicListMetadataType < BaseObject
field :total_weight, GraphQL::Types::Int, null: true,
description: 'Total weight of all issues in the list.',
- feature_flag: :epic_board_total_weight
+ _deprecated_feature_flag: :epic_board_total_weight
end
# rubocop: enable Graphql/AuthorizeTypes
end
diff --git a/ee/app/graphql/types/geo/geo_node_type.rb b/ee/app/graphql/types/geo/geo_node_type.rb
index a0be29b5bfe50..f6b6c2e8f2bd5 100644
--- a/ee/app/graphql/types/geo/geo_node_type.rb
+++ b/ee/app/graphql/types/geo/geo_node_type.rb
@@ -11,7 +11,7 @@ class GeoNodeType < BaseObject
null: true,
resolver: ::Resolvers::Geo::CiSecureFileRegistriesResolver,
description: 'Find Ci Secure File registries on this Geo node',
- feature_flag: :geo_ci_secure_file_replication
+ _deprecated_feature_flag: :geo_ci_secure_file_replication
field :container_repositories_max_capacity, GraphQL::Types::Int, null: true, description: 'Maximum concurrency of container repository sync for this secondary node.'
field :enabled, GraphQL::Types::Boolean, null: true, description: 'Indicates whether this Geo node is enabled.'
field :files_max_capacity, GraphQL::Types::Int, null: true, description: 'Maximum concurrency of LFS/attachment backfill for this secondary node.'
diff --git a/rubocop/cop/gitlab/mark_used_feature_flags.rb b/rubocop/cop/gitlab/mark_used_feature_flags.rb
index 0bebd7901f3ed..63bccec31c056 100644
--- a/rubocop/cop/gitlab/mark_used_feature_flags.rb
+++ b/rubocop/cop/gitlab/mark_used_feature_flags.rb
@@ -145,7 +145,7 @@ def flag_arg(node)
return unless node.children[opts_index]
node.children[opts_index].each_pair.find do |pair|
- pair.key.value == :feature_flag
+ pair.key.value == :_deprecated_feature_flag
end&.value
else
arg_index = rugged_method?(node) ? 3 : 2
diff --git a/spec/graphql/features/feature_flag_spec.rb b/spec/graphql/features/feature_flag_spec.rb
index e5560fccf894c..b06718eb16ae8 100644
--- a/spec/graphql/features/feature_flag_spec.rb
+++ b/spec/graphql/features/feature_flag_spec.rb
@@ -24,7 +24,7 @@
let(:query_type) do
query_factory do |query|
- query.field :item, type, null: true, feature_flag: feature_flag, resolver: new_resolver(test_object)
+ query.field :item, type, null: true, _deprecated_feature_flag: feature_flag, resolver: new_resolver(test_object)
end
end
diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb
index a3efc60ff5b4f..b85716e4d2114 100644
--- a/spec/graphql/types/base_field_spec.rb
+++ b/spec/graphql/types/base_field_spec.rb
@@ -209,7 +209,7 @@ def self.complexity_multiplier(args)
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, feature_flag: flag, null: false) }
+ let(:field) { described_class.new(name: 'test', type: GraphQL::Types::String, _deprecated_feature_flag: flag, null: false) }
let(:context) { {} }
before do
@@ -253,7 +253,7 @@ def self.complexity_multiplier(args)
describe '#description' do
context 'feature flag given' do
- let(:field) { described_class.new(name: 'test', type: GraphQL::Types::String, feature_flag: flag, null: false, description: 'Test description.') }
+ 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
@@ -313,11 +313,11 @@ def subject(args = {})
described_class.new(**base_args.merge(args))
end
- it 'interacts well with the `feature_flag` property' do
+ it 'interacts well with the `_deprecated_feature_flag` property' do
field = subject(
deprecated: { milestone: '1.10', reason: 'Deprecation reason' },
description: 'Field description.',
- feature_flag: 'foo_flag'
+ _deprecated_feature_flag: 'foo_flag'
)
expect(field.description).to start_with('Field description. Available only when feature flag `foo_flag` is enabled.')
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 2ec3ae7aadae6..9ab5cdc24a447 100644
--- a/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
+++ b/spec/rubocop/cop/gitlab/mark_used_feature_flags_spec.rb
@@ -217,8 +217,8 @@ class Foo < ApplicationRecord
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, feature_flag: :foo', 'foo'
- include_examples 'sets flag as used', 'field :runners, null: true, feature_flag: :foo', 'foo'
+ 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"'
--
GitLab