diff --git a/.rubocop.yml b/.rubocop.yml index 9322efd8591ef18a222dcd8c3eda7016b59a2b18..3ab01b807c57cc34df937ab9fab7284d9825964a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -840,15 +840,21 @@ API/ClassLevelAllowAccessWithScope: API/DescriptionDetail: Enabled: true Include: - - 'lib/**/api/**/*.rb' - - 'ee/lib/**/api/**/*.rb' + - 'lib/**/api/**/*.rb' + - 'ee/lib/**/api/**/*.rb' + +API/DescriptionSuccessResponse: + Enabled: true + Include: + - 'lib/**/api/**/*.rb' + - 'ee/lib/**/api/**/*.rb' API/DescriptionTags: Enabled: true SafeAutoCorrect: false Include: - - 'lib/**/api/**/*.rb' - - 'ee/lib/**/api/**/*.rb' + - 'lib/**/api/**/*.rb' + - 'ee/lib/**/api/**/*.rb' API/ParameterDescription: Enabled: true diff --git a/.rubocop_todo/api/description_success_response.yml b/.rubocop_todo/api/description_success_response.yml new file mode 100644 index 0000000000000000000000000000000000000000..acd0757dab0d116b4eaa4afb27c347411f6b0c06 --- /dev/null +++ b/.rubocop_todo/api/description_success_response.yml @@ -0,0 +1,70 @@ +API/DescriptionSuccessResponse: + Details: grace period + Exclude: + - 'ee/lib/api/admin/search/migrations.rb' + - 'ee/lib/api/admin/search/zoekt.rb' + - 'ee/lib/api/clusters/agent_url_configurations.rb' + - 'ee/lib/api/elasticsearch_indexed_namespaces.rb' + - 'ee/lib/api/group_security_settings.rb' + - 'ee/lib/api/group_service_accounts.rb' + - 'ee/lib/api/internal/ai/x_ray/scan.rb' + - 'ee/lib/api/internal/search/zoekt.rb' + - 'ee/lib/api/ldap_group_links.rb' + - 'ee/lib/api/license.rb' + - 'ee/lib/api/manage/groups.rb' + - 'ee/lib/api/project_google_cloud_integration.rb' + - 'ee/lib/api/project_security_settings.rb' + - 'ee/lib/api/protected_environments.rb' + - 'ee/lib/api/remote_development/internal/agents/agentw/agent_info.rb' + - 'ee/lib/api/remote_development/internal/agents/agentw/authorize_user_access.rb' + - 'ee/lib/api/remote_development/internal/agents/agentw/server_config.rb' + - 'ee/lib/api/sbom/occurrences.rb' + - 'ee/lib/api/scim/group_scim.rb' + - 'ee/lib/api/virtual_registries/cleanup/policies.rb' + - 'ee/lib/ee/api/group_milestones.rb' + - 'ee/lib/ee/api/internal/kubernetes.rb' + - 'ee/lib/ee/api/project_milestones.rb' + - 'lib/api/access_requests.rb' + - 'lib/api/admin/token.rb' + - 'lib/api/badges.rb' + - 'lib/api/ci/job_artifacts.rb' + - 'lib/api/ci/pipelines.rb' + - 'lib/api/ci/runner.rb' + - 'lib/api/ci/secure_files.rb' + - 'lib/api/clusters/agent_tokens.rb' + - 'lib/api/clusters/agents.rb' + - 'lib/api/deploy_keys.rb' + - 'lib/api/deploy_tokens.rb' + - 'lib/api/deployments.rb' + - 'lib/api/environments.rb' + - 'lib/api/feature_flags_user_lists.rb' + - 'lib/api/features.rb' + - 'lib/api/go_proxy.rb' + - 'lib/api/group_import.rb' + - 'lib/api/group_placeholder_reassignments.rb' + - 'lib/api/groups.rb' + - 'lib/api/integrations.rb' + - 'lib/api/internal/auto_flow.rb' + - 'lib/api/internal/kubernetes.rb' + - 'lib/api/internal/lfs.rb' + - 'lib/api/internal/pages.rb' + - 'lib/api/members.rb' + - 'lib/api/merge_request_approvals.rb' + - 'lib/api/merge_requests.rb' + - 'lib/api/ml/mlflow/entrypoint.rb' + - 'lib/api/ml/mlflow/experiments.rb' + - 'lib/api/ml/mlflow/registered_models.rb' + - 'lib/api/ml/mlflow/runs.rb' + - 'lib/api/ml/mlflow_artifacts/artifacts.rb' + - 'lib/api/ml/mlflow_artifacts/entrypoint.rb' + - 'lib/api/project_import.rb' + - 'lib/api/project_milestones.rb' + - 'lib/api/releases.rb' + - 'lib/api/rpm_project_packages.rb' + - 'lib/api/rubygem_packages.rb' + - 'lib/api/search.rb' + - 'lib/api/snippets.rb' + - 'lib/api/supply_chain/attestations.rb' + - 'lib/api/topics.rb' + - 'lib/api/unleash.rb' + - 'lib/api/users.rb' diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index ed976d64da346a6469e495794cbd4a003dbb0d18..189d10fcd814d9c9b1091592e783e37eec732f8b 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -48,6 +48,7 @@ for a good example): - `desc` for the method summary. - `detail` for each `desc` block. This must be a string. +- `success` for each `desc` block. This defines the success response. - `tags` for each `desc` block. This should be a string, or array of strings. - `params` for the method parameters. This acts as description, [validation, and coercion of the parameters](https://github.com/ruby-grape/grape#parameter-validation-and-coercion) @@ -80,6 +81,19 @@ The `detail` should describe any additional details not covered by the `desc` su - If it is behind a feature flag, mention that instead: `This feature is gated by the :feature\_flag\_symbol feature flag.` - If the endpoint is deprecated, and if so, its planned removal date +### Defining endpoint success + +Every endpoint must have a `success` value for each `desc` block. The value should accurately +describe a success response for the endpoint. + +Do not use the `http_codes` option to document the success response. Instead, format the response +based on the endpoint response: + +- If the endpoint responds with an object, include the `Grape::Entity` class. + For example, `success Entities::System::BroadcastMessage` +- If the endpoint does not respond with an object, include a status code and message. +For example, `success code: 204, message: 'Record was deleted'` + ### Choosing a tag Every endpoint must have at least one value defined in `tags` per `desc` block. diff --git a/gems/gitlab-grape-openapi/spec/fixtures/test_audit_events.rb b/gems/gitlab-grape-openapi/spec/fixtures/test_audit_events.rb index 7eacb30d41c56577ba2a25aa5f831903fe73c06e..9c7fc88aaca9e4828babf9ec28983e2ce8039d6a 100644 --- a/gems/gitlab-grape-openapi/spec/fixtures/test_audit_events.rb +++ b/gems/gitlab-grape-openapi/spec/fixtures/test_audit_events.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +# rubocop:disable API/DescriptionSuccessResponse -- Needed to test edge cases + require 'grape' module API @@ -29,3 +31,4 @@ class TestAuditEvents < Grape::API end end end +# rubocop:enable API/DescriptionSuccessResponse diff --git a/rubocop/cop/api/description_success_response.rb b/rubocop/cop/api/description_success_response.rb new file mode 100644 index 0000000000000000000000000000000000000000..0f617882a802c1227179e3aa530022689ba56144 --- /dev/null +++ b/rubocop/cop/api/description_success_response.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require_relative '../../code_reuse_helpers' + +module RuboCop + module Cop + module API + # Checks that API desc blocks define a valid success response + # + # @example + # + # # bad + # desc 'Get a specific thing' do + # detail 'This feature was introduced in GitLab 18.2.' + # tags ['things'] + # ... + # end + # + # # bad + # desc 'Get a specific thing' do + # detail 'This feature was introduced in GitLab 18.2.' + # http_codes [[204, 'Thing was deleted'], [403, 'Forbidden']] + # tags ['things'] + # ... + # end + + # # good + # desc 'Get a specific thing' do + # detail 'This feature was introduced in GitLab 18.2.' + # success Entities::Thing + # tags ['things'] + # ... + # end + # + # # good + # desc 'Get a specific thing' do + # detail 'This feature was introduced in GitLab 18.2.' + # success code: 204, message: 'Thing was deleted' + # failure [{ code: 403, message: 'Forbidden' }] + # tags ['things'] + # ... + # end + class DescriptionSuccessResponse < RuboCop::Cop::Base + include CodeReuseHelpers + + MSG = 'API desc blocks must define a success response. https://docs.gitlab.com/development/api_styleguide/#defining-endpoint-success.' + + # @!method has_success_response?(node) + def_node_matcher :has_success_response?, '`(send nil? :success ...)' + + def on_block(node) + return unless node.method?(:desc) + return if has_success_response?(node) + + add_offense(node) + end + + alias_method :on_numblock, :on_block + end + end + end +end diff --git a/spec/rubocop/cop/api/description_success_response_spec.rb b/spec/rubocop/cop/api/description_success_response_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ef52f2da2310fa23855615e58d910b0f241c28bd --- /dev/null +++ b/spec/rubocop/cop/api/description_success_response_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' +require_relative '../../../../rubocop/cop/api/description_success_response' + +RSpec.describe RuboCop::Cop::API::DescriptionSuccessResponse, :config, feature_category: :api do + context 'when desc block includes success' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + desc 'Get a list of things' do + detail 'This endpoint was introduced in 18.2' + success Entities::Thing + tags %w[things] + end + RUBY + end + + context 'when desc block only includes success' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + desc 'Get a list of things' do + success Entities::Thing + end + RUBY + end + end + end + + context 'when desc block does not have a success' do + it 'registers an offense' do + expect_offense(<<~RUBY) + desc 'Get a list of things' do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ API desc blocks must define a success response. https://docs.gitlab.com/development/api_styleguide/#defining-endpoint-success. + tags %w[things] + end + RUBY + end + end +end