From 515ff5218430f20c71a39411e8636233d7a510ed Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 18 Mar 2020 12:24:38 +1300 Subject: [PATCH] Add `deprecated` property for GraphQL This property sets the `deprecation_reason` property with the added semantics of the milestone that the deprecation happened in. Added the Deprecating fields section to the GraphQL Styleguide. https://gitlab.com/gitlab-org/gitlab/-/issues/199952 --- app/graphql/types/base_field.rb | 24 ++++++- app/graphql/types/commit_type.rb | 4 +- app/graphql/types/grafana_integration_type.rb | 4 +- app/graphql/types/merge_request_type.rb | 5 +- .../graphql/reference/gitlab_schema.graphql | 26 ++++---- doc/api/graphql/reference/gitlab_schema.json | 26 ++++---- doc/api/graphql/reference/index.md | 14 ++--- doc/development/api_graphql_styleguide.md | 45 ++++++++++++++ ee/app/graphql/ee/types/issue_type.rb | 4 +- spec/graphql/types/base_field_spec.rb | 62 ++++++++++++++++++- spec/lib/gitlab/graphql/docs/renderer_spec.rb | 12 ++-- 11 files changed, 177 insertions(+), 49 deletions(-) diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 1b296f8d52bb85..148675e9d9e980 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -12,6 +12,7 @@ def initialize(*args, **kwargs, &block) kwargs[:complexity] = field_complexity(kwargs[:resolver_class], kwargs[:complexity]) @feature_flag = kwargs[:feature_flag] kwargs = check_feature_flag(kwargs) + kwargs = handle_deprecated(kwargs) super(*args, **kwargs, &block) end @@ -41,7 +42,7 @@ def visible?(context) attr_reader :feature_flag def feature_documentation_message(key, description) - "#{description}. Available only when feature flag `#{key}` is enabled." + "#{description}. Available only when feature flag `#{key}` is enabled" end def check_feature_flag(args) @@ -51,6 +52,27 @@ def check_feature_flag(args) args end + def handle_deprecated(kwargs) + if kwargs[:deprecation_reason].present? + raise ArgumentError, 'Use `deprecated` property instead of `deprecation_reason`. ' \ + 'See https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#deprecating-fields' + end + + deprecation = kwargs.delete(:deprecated) + return kwargs unless deprecation + + milestone, reason = deprecation.values_at(:milestone, :reason).map(&:presence) + + raise ArgumentError, 'Please provide a `milestone` within `deprecated`' unless milestone + raise ArgumentError, 'Please provide a `reason` within `deprecated`' unless reason + + deprecated_in = "Deprecated in #{milestone}" + kwargs[:deprecation_reason] = "#{reason}. #{deprecated_in}" + kwargs[:description] += ". #{deprecated_in}: #{reason}" if kwargs[:description] + + kwargs + end + def field_complexity(resolver_class, current) return current if current.present? && current > 0 diff --git a/app/graphql/types/commit_type.rb b/app/graphql/types/commit_type.rb index eb25e3651a8f49..437da3bb585d92 100644 --- a/app/graphql/types/commit_type.rb +++ b/app/graphql/types/commit_type.rb @@ -44,8 +44,8 @@ class CommitType < BaseObject field :latest_pipeline, type: Types::Ci::PipelineType, null: true, - description: "Latest pipeline of the commit", - deprecation_reason: 'Use pipelines', + deprecated: { reason: 'Use `pipelines`', milestone: 12.5 }, + description: 'Latest pipeline of the commit', resolver: Resolvers::CommitPipelinesResolver.last end end diff --git a/app/graphql/types/grafana_integration_type.rb b/app/graphql/types/grafana_integration_type.rb index f234008ee0dd7c..71018f6ce0a054 100644 --- a/app/graphql/types/grafana_integration_type.rb +++ b/app/graphql/types/grafana_integration_type.rb @@ -18,8 +18,8 @@ class GrafanaIntegrationType < ::Types::BaseObject description: 'Timestamp of the issue\'s last activity' field :token, GraphQL::STRING_TYPE, null: false, - deprecation_reason: 'Plain text token has been masked for security reasons', - description: 'API token for the Grafana integration. Field is permanently masked.' + deprecated: { reason: 'Plain text token has been masked for security reasons', milestone: 12.7 }, + description: 'API token for the Grafana integration' def token object.masked_token diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 0da95b367d8b05..8cb439cb465e5a 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -74,8 +74,9 @@ class MergeRequestType < BaseObject description: 'Rebase commit SHA of the merge request' field :rebase_in_progress, GraphQL::BOOLEAN_TYPE, method: :rebase_in_progress?, null: false, calls_gitaly: true, description: 'Indicates if there is a rebase currently in progress for the merge request' - field :merge_commit_message, GraphQL::STRING_TYPE, method: :default_merge_commit_message, null: true, deprecation_reason: "Renamed to defaultMergeCommitMessage", - description: 'Deprecated - renamed to defaultMergeCommitMessage' + field :merge_commit_message, GraphQL::STRING_TYPE, method: :default_merge_commit_message, null: true, + deprecated: { reason: 'Use `defaultMergeCommitMessage`', milestone: 11.8 }, + description: 'Default merge commit message of the merge request' field :default_merge_commit_message, GraphQL::STRING_TYPE, null: true, description: 'Default merge commit message of the merge request' field :merge_ongoing, GraphQL::BOOLEAN_TYPE, method: :merge_ongoing?, null: false, diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 65934e53260850..6d02b2905e2e1e 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -306,7 +306,7 @@ type Commit { id: ID! """ - Latest pipeline of the commit + Latest pipeline of the commit. Deprecated in 12.5: Use `pipelines` """ latestPipeline( """ @@ -323,7 +323,7 @@ type Commit { Filter pipelines by their status """ status: PipelineStatusEnum - ): Pipeline @deprecated(reason: "Use pipelines") + ): Pipeline @deprecated(reason: "Use `pipelines`. Deprecated in 12.5") """ Raw commit message @@ -2493,9 +2493,9 @@ type EpicIssue implements Noteable { designCollection: DesignCollection """ - Deprecated. Use `designCollection` + The designs associated with this issue. Deprecated in 12.2: Use `designCollection` """ - designs: DesignCollection @deprecated(reason: "Use designCollection") + designs: DesignCollection @deprecated(reason: "Use `designCollection`. Deprecated in 12.2") """ Indicates discussion is locked on the issue @@ -2548,7 +2548,7 @@ type EpicIssue implements Noteable { epicIssueId: ID! """ - Current health status. Available only when feature flag `save_issuable_health_status` is enabled. + Current health status. Available only when feature flag `save_issuable_health_status` is enabled """ healthStatus: HealthStatus @@ -2984,9 +2984,9 @@ type GrafanaIntegration { id: ID! """ - API token for the Grafana integration. Field is permanently masked. + API token for the Grafana integration. Deprecated in 12.7: Plain text token has been masked for security reasons """ - token: String! @deprecated(reason: "Plain text token has been masked for security reasons") + token: String! @deprecated(reason: "Plain text token has been masked for security reasons. Deprecated in 12.7") """ Timestamp of the issue's last activity @@ -3489,9 +3489,9 @@ type Issue implements Noteable { designCollection: DesignCollection """ - Deprecated. Use `designCollection` + The designs associated with this issue. Deprecated in 12.2: Use `designCollection` """ - designs: DesignCollection @deprecated(reason: "Use designCollection") + designs: DesignCollection @deprecated(reason: "Use `designCollection`. Deprecated in 12.2") """ Indicates discussion is locked on the issue @@ -3539,7 +3539,7 @@ type Issue implements Noteable { epic: Epic """ - Current health status. Available only when feature flag `save_issuable_health_status` is enabled. + Current health status. Available only when feature flag `save_issuable_health_status` is enabled """ healthStatus: HealthStatus @@ -4242,9 +4242,9 @@ type MergeRequest implements Noteable { ): LabelConnection """ - Deprecated - renamed to defaultMergeCommitMessage + Default merge commit message of the merge request. Deprecated in 11.8: Use `defaultMergeCommitMessage` """ - mergeCommitMessage: String @deprecated(reason: "Renamed to defaultMergeCommitMessage") + mergeCommitMessage: String @deprecated(reason: "Use `defaultMergeCommitMessage`. Deprecated in 11.8") """ SHA of the merge request commit (set once merged) @@ -6191,7 +6191,7 @@ type Project { visibility: String """ - Vulnerabilities reported on the project. Available only when feature flag `first_class_vulnerabilities` is enabled. + Vulnerabilities reported on the project. Available only when feature flag `first_class_vulnerabilities` is enabled """ vulnerabilities( """ diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 6da44c36cae501..40053199d724b6 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -968,7 +968,7 @@ }, { "name": "latestPipeline", - "description": "Latest pipeline of the commit", + "description": "Latest pipeline of the commit. Deprecated in 12.5: Use `pipelines`", "args": [ { "name": "status", @@ -1007,7 +1007,7 @@ "ofType": null }, "isDeprecated": true, - "deprecationReason": "Use pipelines" + "deprecationReason": "Use `pipelines`. Deprecated in 12.5" }, { "name": "message", @@ -7244,7 +7244,7 @@ }, { "name": "designs", - "description": "Deprecated. Use `designCollection`", + "description": "The designs associated with this issue. Deprecated in 12.2: Use `designCollection`", "args": [ ], @@ -7254,7 +7254,7 @@ "ofType": null }, "isDeprecated": true, - "deprecationReason": "Use designCollection" + "deprecationReason": "Use `designCollection`. Deprecated in 12.2" }, { "name": "discussionLocked", @@ -7397,7 +7397,7 @@ }, { "name": "healthStatus", - "description": "Current health status. Available only when feature flag `save_issuable_health_status` is enabled.", + "description": "Current health status. Available only when feature flag `save_issuable_health_status` is enabled", "args": [ ], @@ -8659,7 +8659,7 @@ }, { "name": "token", - "description": "API token for the Grafana integration. Field is permanently masked.", + "description": "API token for the Grafana integration. Deprecated in 12.7: Plain text token has been masked for security reasons", "args": [ ], @@ -8673,7 +8673,7 @@ } }, "isDeprecated": true, - "deprecationReason": "Plain text token has been masked for security reasons" + "deprecationReason": "Plain text token has been masked for security reasons. Deprecated in 12.7" }, { "name": "updatedAt", @@ -9990,7 +9990,7 @@ }, { "name": "designs", - "description": "Deprecated. Use `designCollection`", + "description": "The designs associated with this issue. Deprecated in 12.2: Use `designCollection`", "args": [ ], @@ -10000,7 +10000,7 @@ "ofType": null }, "isDeprecated": true, - "deprecationReason": "Use designCollection" + "deprecationReason": "Use `designCollection`. Deprecated in 12.2" }, { "name": "discussionLocked", @@ -10125,7 +10125,7 @@ }, { "name": "healthStatus", - "description": "Current health status. Available only when feature flag `save_issuable_health_status` is enabled.", + "description": "Current health status. Available only when feature flag `save_issuable_health_status` is enabled", "args": [ ], @@ -12105,7 +12105,7 @@ }, { "name": "mergeCommitMessage", - "description": "Deprecated - renamed to defaultMergeCommitMessage", + "description": "Default merge commit message of the merge request. Deprecated in 11.8: Use `defaultMergeCommitMessage`", "args": [ ], @@ -12115,7 +12115,7 @@ "ofType": null }, "isDeprecated": true, - "deprecationReason": "Renamed to defaultMergeCommitMessage" + "deprecationReason": "Use `defaultMergeCommitMessage`. Deprecated in 11.8" }, { "name": "mergeCommitSha", @@ -18522,7 +18522,7 @@ }, { "name": "vulnerabilities", - "description": "Vulnerabilities reported on the project. Available only when feature flag `first_class_vulnerabilities` is enabled.", + "description": "Vulnerabilities reported on the project. Available only when feature flag `first_class_vulnerabilities` is enabled", "args": [ { "name": "after", diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2acb4822e959c2..cce8603a54ff28 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -82,7 +82,7 @@ Represents a project or group board | `authoredDate` | Time | Timestamp of when the commit was authored | | `description` | String | Description of the commit message | | `id` | ID! | ID (global ID) of the commit | -| `latestPipeline` **{warning-solid}** | Pipeline | **Deprecated:** Use pipelines | +| `latestPipeline` **{warning-solid}** | Pipeline | **Deprecated:** Use `pipelines`. Deprecated in 12.5 | | `message` | String | Raw commit message | | `sha` | String! | SHA1 ID of the commit | | `signatureHtml` | String | Rendered HTML of the commit signature | @@ -413,13 +413,13 @@ Relationship between an epic and an issue | `description` | String | Description of the issue | | `descriptionHtml` | String | The GitLab Flavored Markdown rendering of `description` | | `designCollection` | DesignCollection | Collection of design images associated with this issue | -| `designs` **{warning-solid}** | DesignCollection | **Deprecated:** Use designCollection | +| `designs` **{warning-solid}** | DesignCollection | **Deprecated:** Use `designCollection`. Deprecated in 12.2 | | `discussionLocked` | Boolean! | Indicates discussion is locked on the issue | | `downvotes` | Int! | Number of downvotes the issue has received | | `dueDate` | Time | Due date of the issue | | `epic` | Epic | Epic to which this issue belongs | | `epicIssueId` | ID! | ID of the epic-issue relation | -| `healthStatus` | HealthStatus | Current health status. Available only when feature flag `save_issuable_health_status` is enabled. | +| `healthStatus` | HealthStatus | Current health status. Available only when feature flag `save_issuable_health_status` is enabled | | `id` | ID | Global ID of the epic-issue relation | | `iid` | ID! | Internal ID of the issue | | `milestone` | Milestone | Milestone of the issue | @@ -483,7 +483,7 @@ Autogenerated return type of EpicTreeReorder | `enabled` | Boolean! | Indicates whether Grafana integration is enabled | | `grafanaUrl` | String! | Url for the Grafana host for the Grafana integration | | `id` | ID! | Internal ID of the Grafana integration | -| `token` **{warning-solid}** | String! | **Deprecated:** Plain text token has been masked for security reasons | +| `token` **{warning-solid}** | String! | **Deprecated:** Plain text token has been masked for security reasons. Deprecated in 12.7 | | `updatedAt` | Time! | Timestamp of the issue's last activity | ## Group @@ -535,12 +535,12 @@ Autogenerated return type of EpicTreeReorder | `description` | String | Description of the issue | | `descriptionHtml` | String | The GitLab Flavored Markdown rendering of `description` | | `designCollection` | DesignCollection | Collection of design images associated with this issue | -| `designs` **{warning-solid}** | DesignCollection | **Deprecated:** Use designCollection | +| `designs` **{warning-solid}** | DesignCollection | **Deprecated:** Use `designCollection`. Deprecated in 12.2 | | `discussionLocked` | Boolean! | Indicates discussion is locked on the issue | | `downvotes` | Int! | Number of downvotes the issue has received | | `dueDate` | Time | Due date of the issue | | `epic` | Epic | Epic to which this issue belongs | -| `healthStatus` | HealthStatus | Current health status. Available only when feature flag `save_issuable_health_status` is enabled. | +| `healthStatus` | HealthStatus | Current health status. Available only when feature flag `save_issuable_health_status` is enabled | | `iid` | ID! | Internal ID of the issue | | `milestone` | Milestone | Milestone of the issue | | `reference` | String! | Internal reference of the issue. Returned in shortened format by default | @@ -644,7 +644,7 @@ Autogenerated return type of MarkAsSpamSnippet | `id` | ID! | ID of the merge request | | `iid` | String! | Internal ID of the merge request | | `inProgressMergeCommitSha` | String | Commit SHA of the merge request if merge is in progress | -| `mergeCommitMessage` **{warning-solid}** | String | **Deprecated:** Renamed to defaultMergeCommitMessage | +| `mergeCommitMessage` **{warning-solid}** | String | **Deprecated:** Use `defaultMergeCommitMessage`. Deprecated in 11.8 | | `mergeCommitSha` | String | SHA of the merge request commit (set once merged) | | `mergeError` | String | Error message due to a merge error | | `mergeOngoing` | Boolean! | Indicates if a merge is currently occurring | diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index 3002289f6a6e49..1c93527da0c2c5 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -283,6 +283,51 @@ the `some_feature_flag` feature flag is enabled. If the feature flag is not enabled, an error will be returned saying the field does not exist. +## Deprecating fields + +GitLab's GraphQL API is versionless, which means we maintain backwards +compatibility with older versions of the API with every change. Rather +than removing a field, we need to _deprecate_ the field instead. In +future, GitLab +[may remove deprecated fields](https://gitlab.com/gitlab-org/gitlab/issues/32292). + +Fields are deprecated using the `deprecated` property. The value +of the property is a `Hash` of: + +- `reason` - Reason for the deprecation. +- `milestone` - Milestone that the field was deprecated. + +Example: + +```ruby +field :token, GraphQL::STRING_TYPE, null: true, + deprecated: { reason: 'Login via token has been removed', milestone: 10.0 }, + description: 'Token for login' +``` + +The original `description:` of the field should be maintained, and should +_not_ be updated to mention the deprecation. + +### Deprecation reason styleguide + +Where the reason for deprecation is due to the field being replaced +with another field, the `reason` must be: + +```plaintext +Use `otherFieldName` +``` + +Example: + +```ruby +field :designs, ::Types::DesignManagement::DesignCollectionType, null: true, + deprecated: { reason: 'Use `designCollection`', milestone: 10.0 }, + description: 'The designs associated with this issue', +``` + +If the field is not being replaced by another field, a descriptive +deprecation `reason` should be given. + ## Enums GitLab GraphQL enums are defined in `app/graphql/types`. When defining new enums, the diff --git a/ee/app/graphql/ee/types/issue_type.rb b/ee/app/graphql/ee/types/issue_type.rb index 6cda757dc2670a..28a22199d5c466 100644 --- a/ee/app/graphql/ee/types/issue_type.rb +++ b/ee/app/graphql/ee/types/issue_type.rb @@ -14,9 +14,9 @@ module IssueType resolve: -> (obj, _args, _ctx) { obj.supports_weight? ? obj.weight : nil } field :designs, ::Types::DesignManagement::DesignCollectionType, null: true, - description: "Deprecated. Use `designCollection`", method: :design_collection, - deprecation_reason: 'Use designCollection' + deprecated: { reason: 'Use `designCollection`', milestone: 12.2 }, + description: 'The designs associated with this issue' field :design_collection, ::Types::DesignManagement::DesignCollectionType, null: true, description: 'Collection of design images associated with this issue' diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb index 9251eaee3df446..915700f64379d7 100644 --- a/spec/graphql/types/base_field_spec.rb +++ b/spec/graphql/types/base_field_spec.rb @@ -175,7 +175,7 @@ def self.complexity_multiplier(args) let(:flag) { :test_flag } it 'prepends the description' do - expect(field.description). to eq 'Test description. Available only when feature flag `test_flag` is enabled.' + expect(field.description). to eq 'Test description. Available only when feature flag `test_flag` is enabled' end context 'falsey feature_flag values' do @@ -196,4 +196,64 @@ def self.complexity_multiplier(args) end end end + + describe '`deprecated` property' do + def test_field(args = {}) + base_args = { name: 'test', type: GraphQL::STRING_TYPE, null: true } + + described_class.new(**base_args.merge(args)) + end + + describe 'validations' do + it 'raises an informative error if `deprecation_reason` is used' do + expect { test_field(deprecation_reason: 'foo') }.to raise_error( + ArgumentError, + 'Use `deprecated` property instead of `deprecation_reason`. ' \ + 'See https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#deprecating-fields' + ) + end + + it 'raises an error if a required property is missing', :aggregate_failures do + expect { test_field(deprecated: { milestone: 1.0 }) }.to raise_error( + ArgumentError, + 'Please provide a `reason` within `deprecated`' + ) + expect { test_field(deprecated: { reason: 'Deprecation reason' }) }.to raise_error( + ArgumentError, + 'Please provide a `milestone` within `deprecated`' + ) + end + end + + it 'adds a formatted `deprecated_reason` to the field' do + field = test_field(deprecated: { milestone: 1.0, reason: 'Deprecation reason' }) + + expect(field.deprecation_reason).to eq('Deprecation reason. Deprecated in 1.0') + end + + it 'appends to the description if given' do + field = test_field( + deprecated: { milestone: 1.0, reason: 'Deprecation reason' }, + description: 'Field description' + ) + + expect(field.description).to eq('Field description. Deprecated in 1.0: Deprecation reason') + end + + it 'does not append to the description if it is absent' do + field = test_field(deprecated: { milestone: 1.0, reason: 'Deprecation reason' }) + + expect(field.description).to be_nil + end + + it 'interacts well with the `feature_flag` property' do + field = test_field( + deprecated: { milestone: 1.0, reason: 'Deprecation reason' }, + description: 'Field description', + feature_flag: 'foo_flag' + ) + + expect(field.description).to eq('Field description. Available only when feature flag `foo_flag` is enabled. Deprecated in 1.0: Deprecation reason') + end + end end diff --git a/spec/lib/gitlab/graphql/docs/renderer_spec.rb b/spec/lib/gitlab/graphql/docs/renderer_spec.rb index 5ba70bb8f0aa48..3982c79e23c548 100644 --- a/spec/lib/gitlab/graphql/docs/renderer_spec.rb +++ b/spec/lib/gitlab/graphql/docs/renderer_spec.rb @@ -6,7 +6,7 @@ describe '#contents' do # Returns a Schema that uses the given `type` def mock_schema(type) - query_type = Class.new(GraphQL::Schema::Object) do + query_type = Class.new(Types::BaseObject) do graphql_name 'QueryType' field :foo, type, null: true @@ -27,7 +27,7 @@ def mock_schema(type) context 'A type with a field with a [Array] return type' do let(:type) do - Class.new(GraphQL::Schema::Object) do + Class.new(Types::BaseObject) do graphql_name 'ArrayTest' field :foo, [GraphQL::STRING_TYPE], null: false, description: 'A description' @@ -49,7 +49,7 @@ def mock_schema(type) context 'A type with fields defined in reverse alphabetical order' do let(:type) do - Class.new(GraphQL::Schema::Object) do + Class.new(Types::BaseObject) do graphql_name 'OrderingTest' field :foo, GraphQL::STRING_TYPE, null: false, description: 'A description of foo field' @@ -73,10 +73,10 @@ def mock_schema(type) context 'A type with a deprecated field' do let(:type) do - Class.new(GraphQL::Schema::Object) do + Class.new(Types::BaseObject) do graphql_name 'DeprecatedTest' - field :foo, GraphQL::STRING_TYPE, null: false, deprecation_reason: 'This is deprecated', description: 'A description' + field :foo, GraphQL::STRING_TYPE, null: false, deprecated: { reason: 'This is deprecated', milestone: 1.0 }, description: 'A description' end end @@ -86,7 +86,7 @@ def mock_schema(type) | Name | Type | Description | | --- | ---- | ---------- | - | `foo` **{warning-solid}** | String! | **Deprecated:** This is deprecated | + | `foo` **{warning-solid}** | String! | **Deprecated:** This is deprecated. Deprecated in 1.0 | DOC is_expected.to include(expectation) -- GitLab