From 3a0c6dd9f8de0cc0e89e31301652cd47a35089b1 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 29 May 2020 16:12:45 -0700 Subject: [PATCH] Upgrade to Grape v1.3.3 This brings back many of the changes in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27276. This was reverted due to some failures in the QA tests with nil parameters. Grape v1.3.3 brings in Ruby 2.7 support and a number of fixes: https://github.com/ruby-grape/grape/blob/master/CHANGELOG.md 1. Move all inherited `Grape::API` -> `Grape::API::Instance` 2. Remove use of Virtus since this has been removed from Grape. 3. Extract `Rack::Response` from API error 4. Grape v1.2.3 pulled in a fix used in `SafeFile`: https://github.com/ruby-grape/grape/pull/1844, so we no longer need to maintain our custom type. 5. Adapt `WorkhorseFile` with the latest changes to make custom types work with Grape and dry-types. 6. Ensure `Array[String]` is coerced properly. The change from Virtus to dry-types now requires all strings to be coerced to arrays. Before this was done within Virtus. 7. Coerce `Array[Integer]` types to arrays of integers 8. Use a new helper, `coerce_nil_params_to_array!`, that coerces nil Array inputs to empty arrays to preserve previous behavior. If you have a parameter of type `Array[String]`, for example, Grape used to coerce a provided `nil` value to `[]`. Grape no longer does this for us, so we need a helper method that will automatically do this if the parameter is present. This merge request also introduces two Rubocop rules for Grape v1.3: 1. `Grape::API::Instance` instead of `Grape::API` is required, unless we solve https://gitlab.com/gitlab-org/gitlab/-/issues/215230. 2. Grape parameters defined with `Array` types (e.g. `Array[String]`, `Array[Integer]`) must have a `coerce_with` block or they will fail to parse properly. See https://github.com/ruby-grape/grape/blob/master/UPGRADING.md for more details. --- .rubocop.yml | 12 +++ Gemfile | 4 +- Gemfile.lock | 55 +++++++----- changelogs/unreleased/sh-update-grape-gem.yml | 5 ++ doc/development/api_styleguide.md | 40 +++++++++ doc/development/ee_features.md | 12 +-- ee/lib/api/analytics/code_review_analytics.rb | 4 +- .../api/analytics/group_activity_analytics.rb | 2 +- ee/lib/api/audit_events.rb | 2 +- ee/lib/api/composer_packages.rb | 2 +- ee/lib/api/conan_packages.rb | 2 +- ee/lib/api/dependencies.rb | 10 +-- ee/lib/api/dependency_proxy.rb | 2 +- .../api/elasticsearch_indexed_namespaces.rb | 2 +- ee/lib/api/epic_issues.rb | 2 +- ee/lib/api/epic_links.rb | 2 +- ee/lib/api/epics.rb | 8 +- ee/lib/api/feature_flag_scopes.rb | 2 +- ee/lib/api/feature_flags.rb | 2 +- ee/lib/api/feature_flags_user_lists.rb | 2 +- ee/lib/api/geo.rb | 2 +- ee/lib/api/geo_nodes.rb | 14 ++-- ee/lib/api/geo_replication.rb | 2 +- ee/lib/api/go_proxy.rb | 2 +- ee/lib/api/group_hooks.rb | 2 +- ee/lib/api/group_packages.rb | 2 +- .../helpers/project_approval_rules_helpers.rb | 14 ++-- ee/lib/api/issue_links.rb | 2 +- ee/lib/api/ldap.rb | 2 +- ee/lib/api/ldap_group_links.rb | 2 +- ee/lib/api/license.rb | 2 +- ee/lib/api/managed_licenses.rb | 2 +- ee/lib/api/maven_packages.rb | 2 +- ee/lib/api/merge_request_approval_rules.rb | 10 +-- ee/lib/api/merge_request_approvals.rb | 10 +-- ee/lib/api/merge_trains.rb | 2 +- ee/lib/api/npm_packages.rb | 2 +- ee/lib/api/nuget_packages.rb | 2 +- ee/lib/api/package_files.rb | 2 +- ee/lib/api/project_aliases.rb | 2 +- ee/lib/api/project_approval_rules.rb | 2 +- ee/lib/api/project_approval_settings.rb | 2 +- ee/lib/api/project_approvals.rb | 8 +- ee/lib/api/project_mirror.rb | 2 +- ee/lib/api/project_packages.rb | 2 +- ee/lib/api/project_push_rule.rb | 2 +- ee/lib/api/protected_environments.rb | 2 +- ee/lib/api/pypi_packages.rb | 2 +- ee/lib/api/resource_weight_events.rb | 2 +- ee/lib/api/scim.rb | 2 +- ee/lib/api/unleash.rb | 2 +- ee/lib/api/v3/github.rb | 2 +- ee/lib/api/visual_review_discussions.rb | 2 +- ee/lib/api/vulnerabilities.rb | 2 +- ee/lib/api/vulnerability_exports.rb | 2 +- ee/lib/api/vulnerability_findings.rb | 8 +- ee/lib/api/vulnerability_issue_links.rb | 2 +- ee/lib/ee/api/boards.rb | 2 +- ee/lib/ee/api/group_boards.rb | 2 +- ee/lib/ee/api/helpers/settings_helpers.rb | 6 +- ee/spec/lib/ee/api/helpers_spec.rb | 2 +- ee/spec/requests/api/geo_nodes_spec.rb | 2 +- .../api/merge_request_approval_rules_spec.rb | 34 +++++--- .../api/merge_request_approvals_spec.rb | 10 +-- .../api/project_approval_rules_spec.rb | 1 + .../api/project_approval_settings_spec.rb | 1 + ...ject_approval_rules_api_shared_examples.rb | 7 +- lib/api/access_requests.rb | 2 +- lib/api/admin/ci/variables.rb | 2 +- lib/api/admin/sidekiq.rb | 2 +- lib/api/api.rb | 4 +- lib/api/api_guard.rb | 11 ++- lib/api/appearance.rb | 2 +- lib/api/applications.rb | 2 +- lib/api/avatar.rb | 2 +- lib/api/award_emoji.rb | 2 +- lib/api/badges.rb | 2 +- lib/api/boards.rb | 2 +- lib/api/branches.rb | 2 +- lib/api/broadcast_messages.rb | 2 +- lib/api/ci/runner.rb | 4 +- lib/api/ci/runners.rb | 12 +-- lib/api/commit_statuses.rb | 2 +- lib/api/commits.rb | 2 +- lib/api/container_registry_event.rb | 2 +- lib/api/deploy_keys.rb | 2 +- lib/api/deploy_tokens.rb | 6 +- lib/api/deployments.rb | 2 +- lib/api/discussions.rb | 2 +- lib/api/environments.rb | 2 +- lib/api/error_tracking.rb | 2 +- lib/api/events.rb | 2 +- lib/api/features.rb | 2 +- lib/api/files.rb | 2 +- lib/api/freeze_periods.rb | 2 +- lib/api/group_boards.rb | 2 +- lib/api/group_clusters.rb | 2 +- lib/api/group_container_repositories.rb | 2 +- lib/api/group_export.rb | 2 +- lib/api/group_import.rb | 2 +- lib/api/group_labels.rb | 2 +- lib/api/group_milestones.rb | 6 +- lib/api/group_variables.rb | 2 +- lib/api/groups.rb | 4 +- lib/api/helpers/common_helpers.rb | 20 +++++ lib/api/helpers/merge_requests_helpers.rb | 2 +- lib/api/helpers/projects_helpers.rb | 2 +- lib/api/import_github.rb | 2 +- lib/api/internal/base.rb | 2 +- lib/api/internal/pages.rb | 2 +- lib/api/issues.rb | 14 ++-- lib/api/job_artifacts.rb | 2 +- lib/api/jobs.rb | 2 +- lib/api/keys.rb | 2 +- lib/api/labels.rb | 2 +- lib/api/lint.rb | 2 +- lib/api/markdown.rb | 2 +- lib/api/members.rb | 6 +- lib/api/merge_request_diffs.rb | 2 +- lib/api/merge_requests.rb | 16 ++-- lib/api/metrics/dashboard/annotations.rb | 2 +- lib/api/metrics/user_starred_dashboards.rb | 2 +- lib/api/milestone_responses.rb | 2 +- lib/api/namespaces.rb | 2 +- lib/api/notes.rb | 2 +- lib/api/notification_settings.rb | 2 +- lib/api/pages.rb | 2 +- lib/api/pages_domains.rb | 2 +- lib/api/pagination_params.rb | 2 +- lib/api/pipeline_schedules.rb | 2 +- lib/api/pipelines.rb | 2 +- lib/api/project_clusters.rb | 2 +- lib/api/project_container_repositories.rb | 2 +- lib/api/project_events.rb | 2 +- lib/api/project_export.rb | 2 +- lib/api/project_hooks.rb | 2 +- lib/api/project_import.rb | 2 +- lib/api/project_milestones.rb | 6 +- lib/api/project_repository_storage_moves.rb | 2 +- lib/api/project_snapshots.rb | 2 +- lib/api/project_snippets.rb | 2 +- lib/api/project_statistics.rb | 2 +- lib/api/project_templates.rb | 2 +- lib/api/projects.rb | 4 +- lib/api/protected_branches.rb | 2 +- lib/api/protected_tags.rb | 2 +- lib/api/release/links.rb | 2 +- lib/api/releases.rb | 4 +- lib/api/remote_mirrors.rb | 2 +- lib/api/repositories.rb | 4 +- lib/api/resource_label_events.rb | 2 +- lib/api/resource_milestone_events.rb | 2 +- lib/api/search.rb | 2 +- lib/api/services.rb | 2 +- lib/api/settings.rb | 11 +-- lib/api/sidekiq_metrics.rb | 2 +- lib/api/snippets.rb | 2 +- lib/api/statistics.rb | 2 +- lib/api/submodules.rb | 2 +- lib/api/subscriptions.rb | 2 +- lib/api/suggestions.rb | 4 +- lib/api/system_hooks.rb | 2 +- lib/api/tags.rb | 2 +- lib/api/templates.rb | 2 +- lib/api/terraform/state.rb | 2 +- lib/api/todos.rb | 2 +- lib/api/triggers.rb | 2 +- lib/api/user_counts.rb | 2 +- lib/api/users.rb | 2 +- .../types/comma_separated_to_array.rb | 2 +- .../types/comma_separated_to_integer_array.rb | 15 ++++ lib/api/validations/types/labels_list.rb | 24 ------ lib/api/validations/types/safe_file.rb | 15 ---- lib/api/validations/types/workhorse_file.rb | 13 ++- lib/api/variables.rb | 2 +- lib/api/version.rb | 2 +- lib/api/wikis.rb | 4 +- rubocop/cop/api/grape_api_instance.rb | 42 ++++++++++ rubocop/cop/api/grape_array_missing_coerce.rb | 83 +++++++++++++++++++ spec/lib/api/helpers/common_helpers_spec.rb | 51 ++++++++++++ spec/requests/api/applications_spec.rb | 9 +- spec/requests/api/settings_spec.rb | 10 ++- .../cop/api/grape_api_instance_spec.rb | 29 +++++++ .../api/grape_array_missing_coerce_spec.rb | 62 ++++++++++++++ spec/rubocop/cop/code_reuse/worker_spec.rb | 2 +- 185 files changed, 695 insertions(+), 341 deletions(-) create mode 100644 changelogs/unreleased/sh-update-grape-gem.yml create mode 100644 lib/api/validations/types/comma_separated_to_integer_array.rb delete mode 100644 lib/api/validations/types/labels_list.rb delete mode 100644 lib/api/validations/types/safe_file.rb create mode 100644 rubocop/cop/api/grape_api_instance.rb create mode 100644 rubocop/cop/api/grape_array_missing_coerce.rb create mode 100644 spec/lib/api/helpers/common_helpers_spec.rb create mode 100644 spec/rubocop/cop/api/grape_api_instance_spec.rb create mode 100644 spec/rubocop/cop/api/grape_array_missing_coerce_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 1967cbfb982e..89e6214fe47d 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -308,6 +308,18 @@ Gitlab/Union: - 'spec/**/*' - 'ee/spec/**/*' +API/GrapeAPIInstance: + Enabled: true + Include: + - 'lib/**/api/**/*.rb' + - 'ee/**/api/**/*.rb' + +API/GrapeArrayMissingCoerce: + Enabled: true + Include: + - 'lib/**/api/**/*.rb' + - 'ee/**/api/**/*.rb' + Cop/SidekiqOptionsQueue: Enabled: true Exclude: diff --git a/Gemfile b/Gemfile index c33975966cbe..099cf5477dc9 100644 --- a/Gemfile +++ b/Gemfile @@ -19,7 +19,7 @@ gem 'default_value_for', '~> 3.3.0' gem 'pg', '~> 1.1' gem 'rugged', '~> 0.28' -gem 'grape-path-helpers', '~> 1.2' +gem 'grape-path-helpers', '~> 1.3' gem 'faraday', '~> 0.12' gem 'marginalia', '~> 1.8.0' @@ -81,7 +81,7 @@ gem 'gitlab_omniauth-ldap', '~> 2.1.1', require: 'omniauth-ldap' gem 'net-ldap' # API -gem 'grape', '~> 1.1.0' +gem 'grape', '~> 1.3.3' gem 'grape-entity', '~> 0.7.1' gem 'rack-cors', '~> 1.0.6', require: 'rack/cors' diff --git a/Gemfile.lock b/Gemfile.lock index be2d98cb0632..ae35fa30e6e7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -103,10 +103,6 @@ GEM aws-sdk-core (= 2.11.374) aws-sigv4 (1.1.0) aws-eventstream (~> 1.0, >= 1.0.2) - axiom-types (0.1.1) - descendants_tracker (~> 0.0.4) - ice_nine (~> 0.11.0) - thread_safe (~> 0.3, >= 0.3.1) babosa (1.0.2) base32 (0.3.2) batch-loader (1.4.0) @@ -164,8 +160,6 @@ GEM nap open4 (~> 1.3) coderay (1.1.2) - coercible (1.0.0) - descendants_tracker (~> 0.0.1) colored2 (3.1.2) commonmarker (0.20.1) ruby-enum (~> 0.5) @@ -221,8 +215,6 @@ GEM ruby-statistics (>= 2.1) thor (>= 0.19, < 2) unicode_plot (>= 0.0.4, < 1.0.0) - descendants_tracker (0.0.4) - thread_safe (~> 0.3, >= 0.3.1) device_detector (1.0.0) devise (4.7.1) bcrypt (~> 3.0) @@ -249,6 +241,28 @@ GEM doorkeeper-openid_connect (1.6.3) doorkeeper (>= 5.0, < 5.2) json-jwt (~> 1.6) + dry-configurable (0.11.5) + concurrent-ruby (~> 1.0) + dry-core (~> 0.4, >= 0.4.7) + dry-equalizer (~> 0.2) + dry-container (0.7.2) + concurrent-ruby (~> 1.0) + dry-configurable (~> 0.1, >= 0.1.3) + dry-core (0.4.9) + concurrent-ruby (~> 1.0) + dry-equalizer (0.3.0) + dry-inflector (0.2.0) + dry-logic (1.0.6) + concurrent-ruby (~> 1.0) + dry-core (~> 0.2) + dry-equalizer (~> 0.2) + dry-types (1.4.0) + concurrent-ruby (~> 1.0) + dry-container (~> 0.3) + dry-core (~> 0.4, >= 0.4.4) + dry-equalizer (~> 0.3) + dry-inflector (~> 0.1, >= 0.1.2) + dry-logic (~> 1.0, >= 1.0.2) ed25519 (1.2.4) elasticsearch (6.8.0) elasticsearch-api (= 6.8.0) @@ -439,19 +453,19 @@ GEM signet (~> 0.14) gpgme (2.0.20) mini_portile2 (~> 2.3) - grape (1.1.0) + grape (1.3.3) activesupport builder + dry-types (>= 1.1) mustermann-grape (~> 1.0.0) rack (>= 1.3.0) rack-accept - virtus (>= 1.0.0) grape-entity (0.7.1) activesupport (>= 4.0) multi_json (>= 1.3.2) - grape-path-helpers (1.2.0) + grape-path-helpers (1.3.0) activesupport - grape (~> 1.0) + grape (~> 1.3) rake (~> 12) grape_logging (1.8.3) grape @@ -642,9 +656,10 @@ GEM multi_xml (0.6.0) multipart-post (2.1.1) murmurhash3 (0.1.6) - mustermann (1.0.3) - mustermann-grape (1.0.0) - mustermann (~> 1.0.0) + mustermann (1.1.1) + ruby2_keywords (~> 0.0.1) + mustermann-grape (1.0.1) + mustermann (>= 1.0.0) nakayoshi_fork (0.0.4) nap (1.1.0) nenv (0.3.0) @@ -959,6 +974,7 @@ GEM ruby-saml (1.7.2) nokogiri (>= 1.5.10) ruby-statistics (2.1.2) + ruby2_keywords (0.0.2) ruby_dep (1.5.0) ruby_parser (3.13.1) sexp_processor (~> 4.9) @@ -1122,11 +1138,6 @@ GEM activerecord (>= 3.0) activesupport (>= 3.0) version_sorter (2.2.4) - virtus (1.0.5) - axiom-types (~> 0.1) - coercible (~> 1.0) - descendants_tracker (~> 0.0, >= 0.0.3) - equalizer (~> 0.0, >= 0.0.9) vmstat (2.3.0) warden (1.2.8) rack (>= 2.0.6) @@ -1257,9 +1268,9 @@ DEPENDENCIES google-api-client (~> 0.33) google-protobuf (~> 3.8.0) gpgme (~> 2.0.19) - grape (~> 1.1.0) + grape (~> 1.3.3) grape-entity (~> 0.7.1) - grape-path-helpers (~> 1.2) + grape-path-helpers (~> 1.3) grape_logging (~> 1.7) graphiql-rails (~> 1.4.10) graphql (~> 1.10.5) diff --git a/changelogs/unreleased/sh-update-grape-gem.yml b/changelogs/unreleased/sh-update-grape-gem.yml new file mode 100644 index 000000000000..fbed6ad665b6 --- /dev/null +++ b/changelogs/unreleased/sh-update-grape-gem.yml @@ -0,0 +1,5 @@ +--- +title: Upgrade Grape v1.1.0 to v1.3.3 +merge_request: 33450 +author: +type: other diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index 6a0440049267..327f919d7f4a 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -98,6 +98,46 @@ For instance: Model.create(foo: params[:foo]) ``` +## Array types + +With Grape v1.3+, Array types must be defined with a `coerce_with` +block, or parameters will fail to validate when passed a string from an +API request. See the [Grape upgrading +documentation](https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#ensure-that-array-types-have-explicit-coercions) +for more details. + +### Automatic coercion of nil inputs + +Prior to Grape v1.3.3, Array parameters with `nil` values would +automatically be coerced to an empty Array. However, due to [this pull +request in v1.3.3](https://github.com/ruby-grape/grape/pull/2040), this +is no longer the case. For example, suppose you define a PUT `/test` +request that has an optional parameter: + +```ruby +optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule' +``` + +Normally, a request to PUT `/test?user_ids` would cause Grape to pass +`params` of `{ user_ids: nil }`. + +This may introduce errors with endpoints that expect a blank array and +do not handle `nil` inputs properly. To preserve the previous behavior, +there is a helper method `coerce_nil_params_to_array!` that is used +in the `before` block of all API calls: + +```ruby +before do + coerce_nil_params_to_array! +end +``` + +With this change, a request to PUT `/test?user_ids` will cause Grape to +pass `params` to be `{ user_ids: [] }`. + +There is [an open issue in the Grape tracker](https://github.com/ruby-grape/grape/issues/2068) +to make this easier. + ## Using HTTP status helpers For non-200 HTTP responses, use the provided helpers in `lib/api/helpers.rb` to ensure correct behavior (`not_found!`, `no_content!` etc.). These will `throw` inside Grape and abort the execution of your endpoint. diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index e22e96b6f063..e2cbcd6cc228 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -512,12 +512,12 @@ do that, so we'll follow regular object-oriented practices that we define the interface first here. For example, suppose we have a few more optional parameters for EE. We can move the -parameters out of the `Grape::API` class to a helper module, so we can inject it +parameters out of the `Grape::API::Instance` class to a helper module, so we can inject it before it would be used in the class. ```ruby module API - class Projects < Grape::API + class Projects < Grape::API::Instance helpers Helpers::ProjectsHelpers end end @@ -578,7 +578,7 @@ class definition to make it easy and clear: ```ruby module API - class JobArtifacts < Grape::API + class JobArtifacts < Grape::API::Instance # EE::API::JobArtifacts would override the following helpers helpers do def authorize_download_artifacts! @@ -622,7 +622,7 @@ route. Something like this: ```ruby module API - class MergeRequests < Grape::API + class MergeRequests < Grape::API::Instance helpers do # EE::API::MergeRequests would override the following helpers def update_merge_request_ee(merge_request) @@ -691,7 +691,7 @@ least argument. We would approach this as follows: ```ruby # api/merge_requests/parameters.rb module API - class MergeRequests < Grape::API + class MergeRequests < Grape::API::Instance module Parameters def self.update_params_at_least_one_of %i[ @@ -707,7 +707,7 @@ API::MergeRequests::Parameters.prepend_if_ee('EE::API::MergeRequests::Parameters # api/merge_requests.rb module API - class MergeRequests < Grape::API + class MergeRequests < Grape::API::Instance params do at_least_one_of(*Parameters.update_params_at_least_one_of) end diff --git a/ee/lib/api/analytics/code_review_analytics.rb b/ee/lib/api/analytics/code_review_analytics.rb index 5c2b629e3839..5633ae315107 100644 --- a/ee/lib/api/analytics/code_review_analytics.rb +++ b/ee/lib/api/analytics/code_review_analytics.rb @@ -2,7 +2,7 @@ module API module Analytics - class CodeReviewAnalytics < Grape::API + class CodeReviewAnalytics < Grape::API::Instance include PaginationParams helpers do @@ -24,7 +24,7 @@ def finder end params :negatable_params do - optional :label_name, type: Array, desc: 'Array of label names to filter by' + optional :label_name, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Array of label names to filter by' optional :milestone_title, type: String, desc: 'Milestone title to filter by' end end diff --git a/ee/lib/api/analytics/group_activity_analytics.rb b/ee/lib/api/analytics/group_activity_analytics.rb index 8d1306b35a1c..037f89fadbe3 100644 --- a/ee/lib/api/analytics/group_activity_analytics.rb +++ b/ee/lib/api/analytics/group_activity_analytics.rb @@ -2,7 +2,7 @@ module API module Analytics - class GroupActivityAnalytics < Grape::API + class GroupActivityAnalytics < Grape::API::Instance DESCRIPTION_DETAIL = 'This feature is gated by the `:group_activity_analytics`'\ ' feature flag, introduced in GitLab 12.9.' diff --git a/ee/lib/api/audit_events.rb b/ee/lib/api/audit_events.rb index eee4f2eb5f8b..85779cc2bcd3 100644 --- a/ee/lib/api/audit_events.rb +++ b/ee/lib/api/audit_events.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class AuditEvents < ::Grape::API + class AuditEvents < ::Grape::API::Instance include ::API::PaginationParams before do diff --git a/ee/lib/api/composer_packages.rb b/ee/lib/api/composer_packages.rb index 0936e7d0e97d..264bb78c59ea 100644 --- a/ee/lib/api/composer_packages.rb +++ b/ee/lib/api/composer_packages.rb @@ -2,7 +2,7 @@ # PHP composer support (https://getcomposer.org/) module API - class ComposerPackages < Grape::API + class ComposerPackages < Grape::API::Instance helpers ::API::Helpers::PackagesManagerClientsHelpers helpers ::API::Helpers::RelatedResourcesHelpers helpers ::API::Helpers::Packages::BasicAuthHelpers diff --git a/ee/lib/api/conan_packages.rb b/ee/lib/api/conan_packages.rb index 2e899d3e5404..37ef43e14e3a 100644 --- a/ee/lib/api/conan_packages.rb +++ b/ee/lib/api/conan_packages.rb @@ -9,7 +9,7 @@ # # Technical debt: https://gitlab.com/gitlab-org/gitlab/issues/35798 module API - class ConanPackages < Grape::API + class ConanPackages < Grape::API::Instance helpers ::API::Helpers::PackagesManagerClientsHelpers PACKAGE_REQUIREMENTS = { diff --git a/ee/lib/api/dependencies.rb b/ee/lib/api/dependencies.rb index f3a11bb1366e..0a34d9919fd1 100644 --- a/ee/lib/api/dependencies.rb +++ b/ee/lib/api/dependencies.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Dependencies < Grape::API + class Dependencies < Grape::API::Instance helpers do def dependencies_by(params) pipeline = ::Security::ReportFetchService.new(user_project, ::Ci::JobArtifact.dependency_list_reports).pipeline @@ -12,9 +12,7 @@ def dependencies_by(params) end end - before do - authenticate! - end + before { authenticate! } params do requires :id, type: String, desc: 'The ID of a project' @@ -28,6 +26,7 @@ def dependencies_by(params) params do optional :package_manager, type: Array[String], + coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: "Returns dependencies belonging to specified package managers: #{::Security::DependencyListService::FILTER_PACKAGE_MANAGERS_VALUES.join(', ')}.", values: ::Security::DependencyListService::FILTER_PACKAGE_MANAGERS_VALUES end @@ -37,7 +36,8 @@ def dependencies_by(params) track_event('view_dependencies') - dependencies = dependencies_by(declared_params.merge(project: user_project)) + dependency_params = declared_params(include_missing: false).merge(project: user_project) + dependencies = dependencies_by(dependency_params) present dependencies, with: ::EE::API::Entities::Dependency, user: current_user, project: user_project end diff --git a/ee/lib/api/dependency_proxy.rb b/ee/lib/api/dependency_proxy.rb index 9ca425d6c0e0..d1bc4d2c379b 100644 --- a/ee/lib/api/dependency_proxy.rb +++ b/ee/lib/api/dependency_proxy.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class DependencyProxy < Grape::API + class DependencyProxy < Grape::API::Instance helpers ::API::Helpers::PackagesHelpers helpers do diff --git a/ee/lib/api/elasticsearch_indexed_namespaces.rb b/ee/lib/api/elasticsearch_indexed_namespaces.rb index a37954919e05..98934edbff65 100644 --- a/ee/lib/api/elasticsearch_indexed_namespaces.rb +++ b/ee/lib/api/elasticsearch_indexed_namespaces.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ElasticsearchIndexedNamespaces < Grape::API + class ElasticsearchIndexedNamespaces < Grape::API::Instance before { authenticated_as_admin! } resource :elasticsearch_indexed_namespaces do diff --git a/ee/lib/api/epic_issues.rb b/ee/lib/api/epic_issues.rb index 5e0de5aec9c9..8f91d4818c27 100644 --- a/ee/lib/api/epic_issues.rb +++ b/ee/lib/api/epic_issues.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class EpicIssues < Grape::API + class EpicIssues < Grape::API::Instance before do authenticate! authorize_epics_feature! diff --git a/ee/lib/api/epic_links.rb b/ee/lib/api/epic_links.rb index 50c725c2e4a6..08d3f824c77f 100644 --- a/ee/lib/api/epic_links.rb +++ b/ee/lib/api/epic_links.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class EpicLinks < Grape::API + class EpicLinks < Grape::API::Instance include ::Gitlab::Utils::StrongMemoize before do diff --git a/ee/lib/api/epics.rb b/ee/lib/api/epics.rb index 52f74a873e25..dd118e7aaeac 100644 --- a/ee/lib/api/epics.rb +++ b/ee/lib/api/epics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Epics < Grape::API + class Epics < Grape::API::Instance include PaginationParams before do @@ -28,7 +28,7 @@ class Epics < Grape::API optional :state, type: String, values: %w[opened closed all], default: 'all', desc: 'Return opened, closed, or all epics' optional :author_id, type: Integer, desc: 'Return epics which are authored by the user with the given ID' - optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' + optional :labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :with_labels_details, type: Boolean, desc: 'Return titles of labels and other details', default: false optional :created_after, type: DateTime, desc: 'Return epics created after the specified time' optional :created_before, type: DateTime, desc: 'Return epics created before the specified time' @@ -70,7 +70,7 @@ class Epics < Grape::API optional :start_date_is_fixed, type: Boolean, desc: 'Indicates start date should be sourced from start_date_fixed field not the issue milestones' optional :end_date, as: :due_date_fixed, type: String, desc: 'The due date of an epic' optional :due_date_is_fixed, type: Boolean, desc: 'Indicates due date should be sourced from due_date_fixed field not the issue milestones' - optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' + optional :labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :parent_id, type: Integer, desc: 'The id of a parent epic' end post ':id/(-/)epics' do @@ -96,7 +96,7 @@ class Epics < Grape::API optional :start_date_is_fixed, type: Boolean, desc: 'Indicates start date should be sourced from start_date_fixed field not the issue milestones' optional :end_date, as: :due_date_fixed, type: String, desc: 'The due date of an epic' optional :due_date_is_fixed, type: Boolean, desc: 'Indicates due date should be sourced from due_date_fixed field not the issue milestones' - optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' + optional :labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :state_event, type: String, values: %w[reopen close], desc: 'State event for an epic' at_least_one_of :title, :description, :start_date_fixed, :start_date_is_fixed, :due_date_fixed, :due_date_is_fixed, :labels, :state_event, :confidential end diff --git a/ee/lib/api/feature_flag_scopes.rb b/ee/lib/api/feature_flag_scopes.rb index d3b59c6ede8c..1dd457d84853 100644 --- a/ee/lib/api/feature_flag_scopes.rb +++ b/ee/lib/api/feature_flag_scopes.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class FeatureFlagScopes < Grape::API + class FeatureFlagScopes < Grape::API::Instance include PaginationParams ENVIRONMENT_SCOPE_ENDPOINT_REQUIREMENTS = FeatureFlags::FEATURE_FLAG_ENDPOINT_REQUIREMENTS diff --git a/ee/lib/api/feature_flags.rb b/ee/lib/api/feature_flags.rb index 53be399e59b9..c89b569c0514 100644 --- a/ee/lib/api/feature_flags.rb +++ b/ee/lib/api/feature_flags.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class FeatureFlags < Grape::API + class FeatureFlags < Grape::API::Instance include PaginationParams FEATURE_FLAG_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS diff --git a/ee/lib/api/feature_flags_user_lists.rb b/ee/lib/api/feature_flags_user_lists.rb index 3e7dc6075c21..a343f3dc37fc 100644 --- a/ee/lib/api/feature_flags_user_lists.rb +++ b/ee/lib/api/feature_flags_user_lists.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class FeatureFlagsUserLists < Grape::API + class FeatureFlagsUserLists < Grape::API::Instance include PaginationParams error_formatter :json, -> (message, _backtrace, _options, _env, _original_exception) { diff --git a/ee/lib/api/geo.rb b/ee/lib/api/geo.rb index f2612fda57e4..ce421342e2b7 100644 --- a/ee/lib/api/geo.rb +++ b/ee/lib/api/geo.rb @@ -3,7 +3,7 @@ require 'base64' module API - class Geo < Grape::API + class Geo < Grape::API::Instance resource :geo do helpers do def sanitized_node_status_params diff --git a/ee/lib/api/geo_nodes.rb b/ee/lib/api/geo_nodes.rb index b7974211883c..1ffb7237b794 100644 --- a/ee/lib/api/geo_nodes.rb +++ b/ee/lib/api/geo_nodes.rb @@ -1,12 +1,14 @@ # frozen_string_literal: true module API - class GeoNodes < Grape::API + class GeoNodes < Grape::API::Instance include PaginationParams include APIGuard include ::Gitlab::Utils::StrongMemoize - before { authenticate_admin_or_geo_node! } + before do + authenticate_admin_or_geo_node! + end helpers do def authenticate_admin_or_geo_node! @@ -45,8 +47,8 @@ def update_geo_nodes_endpoint? optional :container_repositories_max_capacity, type: Integer, desc: 'Control the maximum concurrency of container repository sync for this node. Defaults to 10.' optional :sync_object_storage, type: Boolean, desc: 'Flag indicating if the secondary Geo node will replicate blobs in Object Storage. Defaults to false.' optional :selective_sync_type, type: String, desc: 'Limit syncing to only specific groups, or shards. Valid values: `"namespaces"`, `"shards"`, or `null`' - optional :selective_sync_shards, type: Array, desc: 'The repository storages whose projects should be synced, if `selective_sync_type` == `shards`' - optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' + optional :selective_sync_shards, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The repository storages whose projects should be synced, if `selective_sync_type` == `shards`' + optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' optional :minimum_reverification_interval, type: Integer, desc: 'The interval (in days) in which the repository verification is valid. Once expired, it will be reverified. This has no effect when set on a secondary node.' end post do @@ -201,8 +203,8 @@ def geo_node_status optional :container_repositories_max_capacity, type: Integer, desc: 'Control the maximum concurrency of container repository sync for this node' optional :sync_object_storage, type: Boolean, desc: 'Flag indicating if the secondary Geo node will replicate blobs in Object Storage' optional :selective_sync_type, type: String, desc: 'Limit syncing to only specific groups, or shards. Valid values: `"namespaces"`, `"shards"`, or `null`' - optional :selective_sync_shards, type: Array, desc: 'The repository storages whose projects should be synced, if `selective_sync_type` == `shards`' - optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' + optional :selective_sync_shards, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The repository storages whose projects should be synced, if `selective_sync_type` == `shards`' + optional :selective_sync_namespace_ids, as: :namespace_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IDs of groups that should be synced, if `selective_sync_type` == `namespaces`' optional :minimum_reverification_interval, type: Integer, desc: 'The interval (in days) in which the repository verification is valid. Once expired, it will be reverified. This has no effect when set on a secondary node.' end put do diff --git a/ee/lib/api/geo_replication.rb b/ee/lib/api/geo_replication.rb index 212a0219a4c2..a726b6d1b3b0 100644 --- a/ee/lib/api/geo_replication.rb +++ b/ee/lib/api/geo_replication.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GeoReplication < Grape::API + class GeoReplication < Grape::API::Instance include PaginationParams include APIGuard include ::Gitlab::Utils::StrongMemoize diff --git a/ee/lib/api/go_proxy.rb b/ee/lib/api/go_proxy.rb index da541b721ac4..64ce0e3e83f2 100755 --- a/ee/lib/api/go_proxy.rb +++ b/ee/lib/api/go_proxy.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module API - class GoProxy < Grape::API + class GoProxy < Grape::API::Instance helpers Gitlab::Golang helpers ::API::Helpers::PackagesHelpers diff --git a/ee/lib/api/group_hooks.rb b/ee/lib/api/group_hooks.rb index 13fbb77d3aea..1da1b99242b9 100644 --- a/ee/lib/api/group_hooks.rb +++ b/ee/lib/api/group_hooks.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupHooks < Grape::API + class GroupHooks < Grape::API::Instance include ::API::PaginationParams before { authenticate! } diff --git a/ee/lib/api/group_packages.rb b/ee/lib/api/group_packages.rb index 321d823c2891..809178eb8701 100644 --- a/ee/lib/api/group_packages.rb +++ b/ee/lib/api/group_packages.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupPackages < Grape::API + class GroupPackages < Grape::API::Instance include PaginationParams before do diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb index 3e43e7c27fd7..3bffd10e49f4 100644 --- a/ee/lib/api/helpers/project_approval_rules_helpers.rb +++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb @@ -5,24 +5,22 @@ module Helpers module ProjectApprovalRulesHelpers extend Grape::API::Helpers - ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) } - params :create_project_approval_rule do requires :name, type: String, desc: 'The name of the approval rule' requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' optional :rule_type, type: String, desc: 'The type of approval rule' - optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' - optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' - optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule' + optional :users, as: :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule' + optional :groups, as: :group_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule' + optional :protected_branch_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The protected branch ids for this rule' end params :update_project_approval_rule do requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule' optional :name, type: String, desc: 'The name of the approval rule' optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' - optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' - optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' - optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule' + optional :users, as: :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule' + optional :groups, as: :group_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule' + optional :protected_branch_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The protected branch ids for this rule' optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' end diff --git a/ee/lib/api/issue_links.rb b/ee/lib/api/issue_links.rb index 4769f8ef8657..79a37f0e5e3c 100644 --- a/ee/lib/api/issue_links.rb +++ b/ee/lib/api/issue_links.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class IssueLinks < Grape::API + class IssueLinks < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/ee/lib/api/ldap.rb b/ee/lib/api/ldap.rb index 2695399b5af5..980fda44e5e8 100644 --- a/ee/lib/api/ldap.rb +++ b/ee/lib/api/ldap.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Ldap < Grape::API + class Ldap < Grape::API::Instance # Admin users by default should be able to access these API endpoints. # However, non-admin users can access these endpoints if the "Allow group # owners to manage LDAP-related group settings" is enabled, and they own a diff --git a/ee/lib/api/ldap_group_links.rb b/ee/lib/api/ldap_group_links.rb index 9a82a2c1ff1c..19763a41dc83 100644 --- a/ee/lib/api/ldap_group_links.rb +++ b/ee/lib/api/ldap_group_links.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class LdapGroupLinks < Grape::API + class LdapGroupLinks < Grape::API::Instance before { authenticate! } params do diff --git a/ee/lib/api/license.rb b/ee/lib/api/license.rb index 0682effccfd1..1bc234576bc5 100644 --- a/ee/lib/api/license.rb +++ b/ee/lib/api/license.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class License < Grape::API + class License < Grape::API::Instance before { authenticated_as_admin! } resource :license do diff --git a/ee/lib/api/managed_licenses.rb b/ee/lib/api/managed_licenses.rb index 807471382b7f..899b14bc9ac2 100644 --- a/ee/lib/api/managed_licenses.rb +++ b/ee/lib/api/managed_licenses.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ManagedLicenses < Grape::API + class ManagedLicenses < Grape::API::Instance include PaginationParams before { authenticate! unless route.settings[:skip_authentication] } diff --git a/ee/lib/api/maven_packages.rb b/ee/lib/api/maven_packages.rb index 23b3a3367dbe..85dbbece164e 100644 --- a/ee/lib/api/maven_packages.rb +++ b/ee/lib/api/maven_packages.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module API - class MavenPackages < Grape::API + class MavenPackages < Grape::API::Instance MAVEN_ENDPOINT_REQUIREMENTS = { file_name: API::NO_SLASH_URL_PART_REGEX }.freeze diff --git a/ee/lib/api/merge_request_approval_rules.rb b/ee/lib/api/merge_request_approval_rules.rb index a959f9a60135..738fd33d39d6 100644 --- a/ee/lib/api/merge_request_approval_rules.rb +++ b/ee/lib/api/merge_request_approval_rules.rb @@ -4,8 +4,6 @@ module API class MergeRequestApprovalRules < ::Grape::API before { authenticate_non_get! } - ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) } - helpers do def find_merge_request_approval_rule(merge_request, id) merge_request.approval_rules.find_by_id!(id) @@ -34,8 +32,8 @@ def find_merge_request_approval_rule(merge_request, id) requires :name, type: String, desc: 'The name of the approval rule' requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' optional :approval_project_rule_id, type: Integer, desc: 'The ID of a project-level approval rule' - optional :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' - optional :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' + optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule' + optional :group_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule' end post do merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers) @@ -56,8 +54,8 @@ def find_merge_request_approval_rule(merge_request, id) requires :approval_rule_id, type: Integer, desc: 'The ID of an approval rule' optional :name, type: String, desc: 'The name of the approval rule' optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' - optional :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' - optional :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' + optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The user ids for this rule' + optional :group_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The group ids for this rule' optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' end put do diff --git a/ee/lib/api/merge_request_approvals.rb b/ee/lib/api/merge_request_approvals.rb index 0d5fe7964014..a759a25244a4 100644 --- a/ee/lib/api/merge_request_approvals.rb +++ b/ee/lib/api/merge_request_approvals.rb @@ -1,11 +1,9 @@ # frozen_string_literal: true module API - class MergeRequestApprovals < ::Grape::API + class MergeRequestApprovals < ::Grape::API::Instance before { authenticate_non_get! } - ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) } - helpers do def present_approval(merge_request) present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user @@ -109,8 +107,10 @@ def present_merge_request_approval_state(presenter:, target_branch: nil) success ::EE::API::Entities::ApprovalState end params do - requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.' - requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.' + requires :approver_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, + desc: 'Array of User IDs to set as approvers.' + requires :approver_group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, + desc: 'Array of Group IDs to set as approvers.' end put 'approvers' do Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/8883') diff --git a/ee/lib/api/merge_trains.rb b/ee/lib/api/merge_trains.rb index 203ed17d1458..e8d60d10e6f5 100644 --- a/ee/lib/api/merge_trains.rb +++ b/ee/lib/api/merge_trains.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class MergeTrains < ::Grape::API + class MergeTrains < ::Grape::API::Instance include PaginationParams before do diff --git a/ee/lib/api/npm_packages.rb b/ee/lib/api/npm_packages.rb index 48f1470c0392..a73cdc0611db 100644 --- a/ee/lib/api/npm_packages.rb +++ b/ee/lib/api/npm_packages.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module API - class NpmPackages < Grape::API + class NpmPackages < Grape::API::Instance helpers ::API::Helpers::PackagesHelpers helpers ::API::Helpers::Packages::DependencyProxyHelpers diff --git a/ee/lib/api/nuget_packages.rb b/ee/lib/api/nuget_packages.rb index 17066f56b267..f6919cbf804f 100644 --- a/ee/lib/api/nuget_packages.rb +++ b/ee/lib/api/nuget_packages.rb @@ -6,7 +6,7 @@ # called by the NuGet package manager client when users run commands # like `nuget install` or `nuget push`. module API - class NugetPackages < Grape::API + class NugetPackages < Grape::API::Instance helpers ::API::Helpers::PackagesManagerClientsHelpers helpers ::API::Helpers::Packages::BasicAuthHelpers diff --git a/ee/lib/api/package_files.rb b/ee/lib/api/package_files.rb index e82a4fa76314..69ba78c55882 100644 --- a/ee/lib/api/package_files.rb +++ b/ee/lib/api/package_files.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class PackageFiles < Grape::API + class PackageFiles < Grape::API::Instance include PaginationParams before do diff --git a/ee/lib/api/project_aliases.rb b/ee/lib/api/project_aliases.rb index 8e346b4c6583..fba4fbbca9c3 100644 --- a/ee/lib/api/project_aliases.rb +++ b/ee/lib/api/project_aliases.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectAliases < Grape::API + class ProjectAliases < Grape::API::Instance include PaginationParams before { check_feature_availability } diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index 2e7a26870d6f..1f8ebf442f05 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectApprovalRules < ::Grape::API + class ProjectApprovalRules < ::Grape::API::Instance before { authenticate! } helpers ::API::Helpers::ProjectApprovalRulesHelpers diff --git a/ee/lib/api/project_approval_settings.rb b/ee/lib/api/project_approval_settings.rb index dbd0ccb753b0..30d9d704a3de 100644 --- a/ee/lib/api/project_approval_settings.rb +++ b/ee/lib/api/project_approval_settings.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectApprovalSettings < ::Grape::API + class ProjectApprovalSettings < ::Grape::API::Instance before { authenticate! } helpers ::API::Helpers::ProjectApprovalRulesHelpers diff --git a/ee/lib/api/project_approvals.rb b/ee/lib/api/project_approvals.rb index 306f9cc147d0..3aa5f48cc22f 100644 --- a/ee/lib/api/project_approvals.rb +++ b/ee/lib/api/project_approvals.rb @@ -1,12 +1,10 @@ # frozen_string_literal: true module API - class ProjectApprovals < ::Grape::API + class ProjectApprovals < ::Grape::API::Instance before { authenticate! } before { authorize! :update_approvers, user_project } - ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) } - helpers do def filter_forbidden_param!(permission, param) unless can?(current_user, permission, user_project) @@ -67,8 +65,8 @@ def filter_params(params) success EE::API::Entities::ApprovalSettings end params do - requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.' - requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.' + requires :approver_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of User IDs to set as approvers.' + requires :approver_group_ids, type: Array[Integer], coerce_with: Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of Group IDs to set as approvers.' end put ':id/approvers' do result = ::Projects::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute diff --git a/ee/lib/api/project_mirror.rb b/ee/lib/api/project_mirror.rb index 482f6fbf20ff..e0173b74e144 100644 --- a/ee/lib/api/project_mirror.rb +++ b/ee/lib/api/project_mirror.rb @@ -3,7 +3,7 @@ require_dependency 'declarative_policy' module API - class ProjectMirror < Grape::API + class ProjectMirror < Grape::API::Instance helpers do def github_webhook_signature @github_webhook_signature ||= headers['X-Hub-Signature'] diff --git a/ee/lib/api/project_packages.rb b/ee/lib/api/project_packages.rb index a9d94b457a98..165ca0c1be8f 100644 --- a/ee/lib/api/project_packages.rb +++ b/ee/lib/api/project_packages.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectPackages < Grape::API + class ProjectPackages < Grape::API::Instance include PaginationParams before do diff --git a/ee/lib/api/project_push_rule.rb b/ee/lib/api/project_push_rule.rb index ed66fce5977c..7a3cc1dba7d8 100644 --- a/ee/lib/api/project_push_rule.rb +++ b/ee/lib/api/project_push_rule.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectPushRule < Grape::API + class ProjectPushRule < Grape::API::Instance before { authenticate! } before { authorize_admin_project } before { check_project_feature_available!(:push_rules) } diff --git a/ee/lib/api/protected_environments.rb b/ee/lib/api/protected_environments.rb index 23c92ed7391c..6de0ce19d918 100644 --- a/ee/lib/api/protected_environments.rb +++ b/ee/lib/api/protected_environments.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProtectedEnvironments < Grape::API + class ProtectedEnvironments < Grape::API::Instance include PaginationParams ENVIRONMENT_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(name: API::NO_SLASH_URL_PART_REGEX) diff --git a/ee/lib/api/pypi_packages.rb b/ee/lib/api/pypi_packages.rb index 5047d8783075..d82e0d673dd2 100644 --- a/ee/lib/api/pypi_packages.rb +++ b/ee/lib/api/pypi_packages.rb @@ -6,7 +6,7 @@ # called by the PyPI package manager client when users run commands # like `pip install` or `twine upload`. module API - class PypiPackages < Grape::API + class PypiPackages < Grape::API::Instance helpers ::API::Helpers::PackagesManagerClientsHelpers helpers ::API::Helpers::RelatedResourcesHelpers helpers ::API::Helpers::Packages::BasicAuthHelpers diff --git a/ee/lib/api/resource_weight_events.rb b/ee/lib/api/resource_weight_events.rb index 4fca231c4afd..b423a49a4306 100644 --- a/ee/lib/api/resource_weight_events.rb +++ b/ee/lib/api/resource_weight_events.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ResourceWeightEvents < Grape::API + class ResourceWeightEvents < Grape::API::Instance include PaginationParams helpers ::API::Helpers::NotesHelpers diff --git a/ee/lib/api/scim.rb b/ee/lib/api/scim.rb index aa4e0dbb2c1c..63731f3e9609 100644 --- a/ee/lib/api/scim.rb +++ b/ee/lib/api/scim.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Scim < Grape::API + class Scim < Grape::API::Instance include ::Gitlab::Utils::StrongMemoize prefix 'api/scim' diff --git a/ee/lib/api/unleash.rb b/ee/lib/api/unleash.rb index 878ee0aa0ff0..4adb693b5681 100644 --- a/ee/lib/api/unleash.rb +++ b/ee/lib/api/unleash.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Unleash < Grape::API + class Unleash < Grape::API::Instance include PaginationParams namespace :feature_flags do diff --git a/ee/lib/api/v3/github.rb b/ee/lib/api/v3/github.rb index 2fe5ef45f7ff..c477cbb663e7 100644 --- a/ee/lib/api/v3/github.rb +++ b/ee/lib/api/v3/github.rb @@ -7,7 +7,7 @@ # module API module V3 - class Github < Grape::API + class Github < Grape::API::Instance JIRA_DEV_PANEL_FEATURE = :jira_dev_panel_integration.freeze NO_SLASH_URL_PART_REGEX = %r{[^/]+}.freeze ENDPOINT_REQUIREMENTS = { diff --git a/ee/lib/api/visual_review_discussions.rb b/ee/lib/api/visual_review_discussions.rb index 76ae44fa4052..86fdf3342271 100644 --- a/ee/lib/api/visual_review_discussions.rb +++ b/ee/lib/api/visual_review_discussions.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class VisualReviewDiscussions < Grape::API + class VisualReviewDiscussions < Grape::API::Instance include PaginationParams helpers ::API::Helpers::NotesHelpers helpers ::RendersNotes diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index e0504884212f..156faeb66283 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/ee/lib/api/vulnerabilities.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Vulnerabilities < Grape::API + class Vulnerabilities < Grape::API::Instance include ::API::Helpers::VulnerabilitiesHooks include PaginationParams diff --git a/ee/lib/api/vulnerability_exports.rb b/ee/lib/api/vulnerability_exports.rb index 87fb2796a8f8..28cc6a228f29 100644 --- a/ee/lib/api/vulnerability_exports.rb +++ b/ee/lib/api/vulnerability_exports.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class VulnerabilityExports < Grape::API + class VulnerabilityExports < Grape::API::Instance include ::API::Helpers::VulnerabilitiesHooks include ::Gitlab::Utils::StrongMemoize diff --git a/ee/lib/api/vulnerability_findings.rb b/ee/lib/api/vulnerability_findings.rb index 2245752fd7b2..9a2a8b8f1dd9 100644 --- a/ee/lib/api/vulnerability_findings.rb +++ b/ee/lib/api/vulnerability_findings.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class VulnerabilityFindings < Grape::API + class VulnerabilityFindings < Grape::API::Instance include PaginationParams include ::Gitlab::Utils::StrongMemoize @@ -33,19 +33,23 @@ def vulnerability_occurrences_by(params) end resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do params do - optional :report_type, type: Array[String], desc: 'The type of report vulnerability belongs to', + optional :report_type, type: Array[String], + coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, + desc: 'The type of report vulnerability belongs to', values: ::Vulnerabilities::Occurrence.report_types.keys, default: ::Vulnerabilities::Occurrence.report_types.keys optional :scope, type: String, desc: 'Return vulnerabilities for the given scope: `dismissed` or `all`', default: 'dismissed', values: %w[all dismissed] optional :severity, type: Array[String], + coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Returns vulnerabilities belonging to specified severity level: '\ '`info`, `unknown`, `low`, `medium`, `high`, or `critical`. Defaults to all', values: ::Vulnerabilities::Occurrence.severities.keys, default: ::Vulnerabilities::Occurrence.severities.keys optional :confidence, type: Array[String], + coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Returns vulnerabilities belonging to specified confidence level: '\ '`ignore`, `unknown`, `experimental`, `low`, `medium`, `high`, or `confirmed`. '\ 'Defaults to all', diff --git a/ee/lib/api/vulnerability_issue_links.rb b/ee/lib/api/vulnerability_issue_links.rb index d64c621ea0e7..8780bc53f34d 100644 --- a/ee/lib/api/vulnerability_issue_links.rb +++ b/ee/lib/api/vulnerability_issue_links.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class VulnerabilityIssueLinks < Grape::API + class VulnerabilityIssueLinks < Grape::API::Instance include ::API::Helpers::VulnerabilitiesHooks helpers ::API::Helpers::VulnerabilitiesHelpers diff --git a/ee/lib/ee/api/boards.rb b/ee/lib/ee/api/boards.rb index 6b62a086960a..60495ea87018 100644 --- a/ee/lib/ee/api/boards.rb +++ b/ee/lib/ee/api/boards.rb @@ -2,7 +2,7 @@ module EE module API - class Boards < ::Grape::API + class Boards < ::Grape::API::Instance include ::API::PaginationParams include ::API::BoardsResponses diff --git a/ee/lib/ee/api/group_boards.rb b/ee/lib/ee/api/group_boards.rb index 1206fda0383f..44c5ba107ff0 100644 --- a/ee/lib/ee/api/group_boards.rb +++ b/ee/lib/ee/api/group_boards.rb @@ -2,7 +2,7 @@ module EE module API - class GroupBoards < ::Grape::API + class GroupBoards < ::Grape::API::Instance include ::API::PaginationParams include ::API::BoardsResponses diff --git a/ee/lib/ee/api/helpers/settings_helpers.rb b/ee/lib/ee/api/helpers/settings_helpers.rb index b6fc63f2d02a..f4e44a970bec 100644 --- a/ee/lib/ee/api/helpers/settings_helpers.rb +++ b/ee/lib/ee/api/helpers/settings_helpers.rb @@ -26,8 +26,8 @@ module SettingsHelpers end given elasticsearch_limit_indexing: ->(val) { val } do - optional :elasticsearch_namespace_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::LabelsList.coerce, desc: 'The namespace ids to index with Elasticsearch.' - optional :elasticsearch_project_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::LabelsList.coerce, desc: 'The project ids to index with Elasticsearch.' + optional :elasticsearch_namespace_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The namespace ids to index with Elasticsearch.' + optional :elasticsearch_project_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The project ids to index with Elasticsearch.' end optional :email_additional_text, type: String, desc: 'Additional text added to the bottom of every email for legal/auditing/compliance reasons' @@ -36,7 +36,7 @@ module SettingsHelpers optional :help_text, type: String, desc: 'GitLab server administrator information' optional :repository_size_limit, type: Integer, desc: 'Size limit per repository (MB)' optional :file_template_project_id, type: Integer, desc: 'ID of project where instance-level file templates are stored.' - optional :repository_storages, type: Array[String], desc: 'A list of names of enabled storage paths, taken from `gitlab.yml`. New projects will be created in one of these stores, chosen at random.' + optional :repository_storages, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'A list of names of enabled storage paths, taken from `gitlab.yml`. New projects will be created in one of these stores, chosen at random.' optional :usage_ping_enabled, type: Grape::API::Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.' optional :updating_name_disabled_for_users, type: Grape::API::Boolean, desc: 'Flag indicating if users are permitted to update their profile name' optional :disable_overriding_approvers_per_merge_request, type: Grape::API::Boolean, desc: 'Disable Users ability to overwrite approvers in merge requests.' diff --git a/ee/spec/lib/ee/api/helpers_spec.rb b/ee/spec/lib/ee/api/helpers_spec.rb index 6770b678a4c6..4c3423de22d0 100644 --- a/ee/spec/lib/ee/api/helpers_spec.rb +++ b/ee/spec/lib/ee/api/helpers_spec.rb @@ -6,7 +6,7 @@ include Rack::Test::Methods let(:helper) do - Class.new(Grape::API) do + Class.new(Grape::API::Instance) do helpers EE::API::Helpers helpers API::APIGuard::HelperMethods helpers API::Helpers diff --git a/ee/spec/requests/api/geo_nodes_spec.rb b/ee/spec/requests/api/geo_nodes_spec.rb index d8353664f51c..b758f69cfe48 100644 --- a/ee/spec/requests/api/geo_nodes_spec.rb +++ b/ee/spec/requests/api/geo_nodes_spec.rb @@ -33,7 +33,7 @@ url: 'http://example.com', selective_sync_type: "shards", selective_sync_shards: %w[shard1 shard2], - selective_sync_namespace_ids: [group_to_sync.id], + selective_sync_namespace_ids: group_to_sync.id, minimum_reverification_interval: 10 } expect_any_instance_of(Geo::NodeCreateService).to receive(:execute).once.and_call_original diff --git a/ee/spec/requests/api/merge_request_approval_rules_spec.rb b/ee/spec/requests/api/merge_request_approval_rules_spec.rb index d1c41699c1e0..14d1cba68391 100644 --- a/ee/spec/requests/api/merge_request_approval_rules_spec.rb +++ b/ee/spec/requests/api/merge_request_approval_rules_spec.rb @@ -144,7 +144,9 @@ let(:current_user) { user } let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules" } let(:approver) { create(:user) } + let(:other_approver) { create(:user) } let(:group) { create(:group) } + let(:other_group) { create(:group) } let(:approval_project_rule_id) { nil } let(:approver_params) do { @@ -171,7 +173,9 @@ before do project.update!(disable_overriding_approvers_per_merge_request: false) project.add_developer(approver) + project.add_developer(other_approver) group.add_developer(approver) + other_group.add_developer(other_approver) action end @@ -184,7 +188,7 @@ expect(rule['name']).to eq(params[:name]) expect(rule['approvals_required']).to eq(params[:approvals_required]) - expect(rule['rule_type']).to eq('regular') + expect(rule['rule_type']).to eq('any_approver') expect(rule['contains_hidden_groups']).to eq(false) expect(rule['source_rule']).to be_nil expect(rule['eligible_approvers']).to be_empty @@ -193,24 +197,24 @@ end context 'users are passed' do - let(:user_ids) { [approver.id] } + let(:user_ids) { "#{approver.id},#{other_approver.id}" } it 'includes users' do rule = json_response - expect(rule['eligible_approvers']).to match([hash_including('id' => approver.id)]) - expect(rule['users']).to match([hash_including('id' => approver.id)]) + expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(approver.id, other_approver.id) + expect(rule['users'].map { |user| user['id'] }).to contain_exactly(approver.id, other_approver.id) end end context 'groups are passed' do - let(:group_ids) { [group.id] } + let(:group_ids) { "#{group.id},#{other_group.id}" } it 'includes groups' do rule = json_response - expect(rule['eligible_approvers']).to match([hash_including('id' => approver.id)]) - expect(rule['groups']).to match([hash_including('id' => group.id)]) + expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(approver.id, other_approver.id) + expect(rule['groups'].map { |group| group['id'] }).to contain_exactly(group.id, other_group.id) end end @@ -279,6 +283,8 @@ let(:user_ids) { [] } let(:group_ids) { [] } let(:remove_hidden_groups) { nil } + let(:other_approver) { create(:user) } + let(:other_group) { create(:group) } let(:params) do { @@ -299,8 +305,10 @@ project.update!(disable_overriding_approvers_per_merge_request: false) project.add_developer(existing_approver) project.add_developer(new_approver) + project.add_developer(other_approver) existing_group.add_developer(existing_approver) new_group.add_developer(new_approver) + other_group.add_developer(other_approver) action end @@ -324,24 +332,24 @@ end context 'users are passed' do - let(:user_ids) { [new_approver.id] } + let(:user_ids) { "#{new_approver.id},#{existing_approver.id}" } it 'changes users' do rule = json_response - expect(rule['eligible_approvers']).to match([hash_including('id' => new_approver.id)]) - expect(rule['users']).to match([hash_including('id' => new_approver.id)]) + expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(new_approver.id, existing_approver.id) + expect(rule['users'].map { |user| user['id'] }).to contain_exactly(new_approver.id, existing_approver.id) end end context 'groups are passed' do - let(:group_ids) { [new_group.id] } + let(:group_ids) { "#{new_group.id},#{other_group.id}" } it 'changes groups' do rule = json_response - expect(rule['eligible_approvers']).to match([hash_including('id' => new_approver.id)]) - expect(rule['groups']).to match([hash_including('id' => new_group.id)]) + expect(rule['eligible_approvers'].map { |approver| approver['id'] }).to contain_exactly(new_approver.id, other_approver.id) + expect(rule['groups'].map { |group| group['id'] }).to contain_exactly(new_group.id, other_group.id) end end diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index 6b901e87cb80..847ffbcb164c 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -342,7 +342,7 @@ it 'does not allow overriding approvers' do expect do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user), - params: { approver_ids: [approver.id], approver_group_ids: [group.id] } + params: { approver_ids: approver.id.to_s, approver_group_ids: group.id.to_s } end.to not_change { merge_request.approvers.count }.and not_change { merge_request.approver_groups.count } end end @@ -355,12 +355,12 @@ it 'allows overriding approvers' do expect do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user), - params: { approver_ids: [approver.id], approver_group_ids: [group.id] } - end.to change { merge_request.approvers.count }.from(0).to(1) + params: { approver_ids: "#{approver.id},#{user2.id}", approver_group_ids: "#{group.id}" } + end.to change { merge_request.approvers.count }.from(0).to(2) .and change { merge_request.approver_groups.count }.from(0).to(1) expect(response).to have_gitlab_http_status(:ok) - expect(json_response['approvers'][0]['user']['username']).to eq(approver.username) + expect(json_response['approvers'].map { |approver| approver['user'] }.map { |user| user['username'] }).to contain_exactly(approver.username, user2.username) expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name) end @@ -370,7 +370,7 @@ expect do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user), - params: { approver_ids: [], approver_group_ids: [] }.to_json, headers: { CONTENT_TYPE: 'application/json' } + params: { approver_ids: '', approver_group_ids: '' }.to_json, headers: { CONTENT_TYPE: 'application/json' } end.to change { merge_request.approvers.count }.from(1).to(0) .and change { merge_request.approver_groups.count }.from(1).to(0) diff --git a/ee/spec/requests/api/project_approval_rules_spec.rb b/ee/spec/requests/api/project_approval_rules_spec.rb index fcfc5782a5c1..8d4cf47696cd 100644 --- a/ee/spec/requests/api/project_approval_rules_spec.rb +++ b/ee/spec/requests/api/project_approval_rules_spec.rb @@ -9,6 +9,7 @@ let_it_be(:admin) { create(:user, :admin) } let_it_be(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) } let_it_be(:approver) { create(:user) } + let_it_be(:other_approver) { create(:user) } describe 'GET /projects/:id/approval_rules' do let(:url) { "/projects/#{project.id}/approval_rules" } diff --git a/ee/spec/requests/api/project_approval_settings_spec.rb b/ee/spec/requests/api/project_approval_settings_spec.rb index 4ad3dc7fe090..0e2a3bbc2740 100644 --- a/ee/spec/requests/api/project_approval_settings_spec.rb +++ b/ee/spec/requests/api/project_approval_settings_spec.rb @@ -9,6 +9,7 @@ let_it_be(:admin) { create(:user, :admin) } let_it_be(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) } let_it_be(:approver) { create(:user) } + let_it_be(:other_approver) { create(:user) } describe 'GET /projects/:id/approval_settings' do let(:url) { "/projects/#{project.id}/approval_settings" } diff --git a/ee/spec/support/shared_examples/requests/api/project_approval_rules_api_shared_examples.rb b/ee/spec/support/shared_examples/requests/api/project_approval_rules_api_shared_examples.rb index 44bc93a57dd1..c791ffa541f5 100644 --- a/ee/spec/support/shared_examples/requests/api/project_approval_rules_api_shared_examples.rb +++ b/ee/spec/support/shared_examples/requests/api/project_approval_rules_api_shared_examples.rb @@ -79,6 +79,7 @@ shared_examples 'a user with access' do before do project.add_developer(approver) + project.add_developer(other_approver) end context 'when protected_branch_ids param is present' do @@ -117,10 +118,10 @@ it 'sets approvers' do expect do - put api(url, current_user), params: { users: [approver.id] } - end.to change { approval_rule.users.count }.from(0).to(1) + put api(url, current_user), params: { users: "#{approver.id},#{other_approver.id}" } + end.to change { approval_rule.users.count }.from(0).to(2) - expect(approval_rule.users).to contain_exactly(approver) + expect(approval_rule.users).to contain_exactly(approver, other_approver) expect(approval_rule.groups).to be_empty expect(response).to have_gitlab_http_status(:ok) diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index ee8dc822098c..5305b25538f8 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class AccessRequests < Grape::API + class AccessRequests < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/admin/ci/variables.rb b/lib/api/admin/ci/variables.rb index df731148bac3..6b0ff5e9395c 100644 --- a/lib/api/admin/ci/variables.rb +++ b/lib/api/admin/ci/variables.rb @@ -3,7 +3,7 @@ module API module Admin module Ci - class Variables < Grape::API + class Variables < Grape::API::Instance include PaginationParams before { authenticated_as_admin! } diff --git a/lib/api/admin/sidekiq.rb b/lib/api/admin/sidekiq.rb index a700bea0fd79..f4c84f2eee80 100644 --- a/lib/api/admin/sidekiq.rb +++ b/lib/api/admin/sidekiq.rb @@ -2,7 +2,7 @@ module API module Admin - class Sidekiq < Grape::API + class Sidekiq < Grape::API::Instance before { authenticated_as_admin! } namespace 'admin' do diff --git a/lib/api/api.rb b/lib/api/api.rb index 1cdb500e6ac6..e5bc49de32e1 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class API < Grape::API + class API < Grape::API::Instance include APIGuard LOG_FILENAME = Rails.root.join("log", "api_json.log") @@ -46,6 +46,8 @@ class API < Grape::API end before do + coerce_nil_params_to_array! + Gitlab::ApplicationContext.push( user: -> { @current_user }, project: -> { @project }, diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index c6557fce5419..1e56b1f3bfcf 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -153,7 +153,16 @@ def oauth2_bearer_token_error_handler { scope: e.scopes }) end - response.finish + finished_response = nil + response.finish do |rack_response| + # Grape expects a Rack::Response + # (https://github.com/ruby-grape/grape/commit/c117bff7d22971675f4b34367d3a98bc31c8fc02), + # and we need to retrieve it here: + # https://github.com/nov/rack-oauth2/blob/40c9a99fd80486ccb8de0e4869ae384547c0d703/lib/rack/oauth2/server/abstract/error.rb#L28 + finished_response = rack_response + end + + finished_response end end end diff --git a/lib/api/appearance.rb b/lib/api/appearance.rb index 71a35bb4493e..f98004af4805 100644 --- a/lib/api/appearance.rb +++ b/lib/api/appearance.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Appearance < Grape::API + class Appearance < Grape::API::Instance before { authenticated_as_admin! } helpers do diff --git a/lib/api/applications.rb b/lib/api/applications.rb index 70e6b8395d76..4e8d68c8d094 100644 --- a/lib/api/applications.rb +++ b/lib/api/applications.rb @@ -2,7 +2,7 @@ module API # External applications API - class Applications < Grape::API + class Applications < Grape::API::Instance before { authenticated_as_admin! } resource :applications do diff --git a/lib/api/avatar.rb b/lib/api/avatar.rb index 0f14d0030659..9501e777fffe 100644 --- a/lib/api/avatar.rb +++ b/lib/api/avatar.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Avatar < Grape::API + class Avatar < Grape::API::Instance resource :avatar do desc 'Return avatar url for a user' do success Entities::Avatar diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb index 8e3b3ff8ce5e..0a3df3ed96ed 100644 --- a/lib/api/award_emoji.rb +++ b/lib/api/award_emoji.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class AwardEmoji < Grape::API + class AwardEmoji < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/badges.rb b/lib/api/badges.rb index d2152fad07ba..f6cd3f83ff38 100644 --- a/lib/api/badges.rb +++ b/lib/api/badges.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Badges < Grape::API + class Badges < Grape::API::Instance include PaginationParams before { authenticate_non_get! } diff --git a/lib/api/boards.rb b/lib/api/boards.rb index 87818903705f..1f5086127a81 100644 --- a/lib/api/boards.rb +++ b/lib/api/boards.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Boards < Grape::API + class Boards < Grape::API::Instance include BoardsResponses include PaginationParams diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 081e8ffe4f00..bbed50e96ea3 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -3,7 +3,7 @@ require 'mime/types' module API - class Branches < Grape::API + class Branches < Grape::API::Instance include PaginationParams BRANCH_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(branch: API::NO_SLASH_URL_PART_REGEX) diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index 42e7dc751f08..dcf950d7a035 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class BroadcastMessages < Grape::API + class BroadcastMessages < Grape::API::Instance include PaginationParams resource :broadcast_messages do diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb index 3116af24ff5d..b216406695b4 100644 --- a/lib/api/ci/runner.rb +++ b/lib/api/ci/runner.rb @@ -2,7 +2,7 @@ module API module Ci - class Runner < Grape::API + class Runner < Grape::API::Instance helpers ::API::Helpers::Runner resource :runners do @@ -19,7 +19,7 @@ class Runner < Grape::API optional :access_level, type: String, values: ::Ci::Runner.access_levels.keys, desc: 'The access_level of the runner' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' - optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: %q(List of Runner's tags) optional :maximum_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do diff --git a/lib/api/ci/runners.rb b/lib/api/ci/runners.rb index a4ef54534bac..2c156a711606 100644 --- a/lib/api/ci/runners.rb +++ b/lib/api/ci/runners.rb @@ -2,7 +2,7 @@ module API module Ci - class Runners < Grape::API + class Runners < Grape::API::Instance include PaginationParams before { authenticate! } @@ -18,7 +18,7 @@ class Runners < Grape::API desc: 'The type of the runners to show' optional :status, type: String, values: ::Ci::Runner::AVAILABLE_STATUSES, desc: 'The status of the runners to show' - optional :tag_list, type: Array[String], desc: 'The tags of the runners to show' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show' use :pagination end get do @@ -41,7 +41,7 @@ class Runners < Grape::API desc: 'The type of the runners to show' optional :status, type: String, values: ::Ci::Runner::AVAILABLE_STATUSES, desc: 'The status of the runners to show' - optional :tag_list, type: Array[String], desc: 'The tags of the runners to show' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show' use :pagination end get 'all' do @@ -76,7 +76,7 @@ class Runners < Grape::API requires :id, type: Integer, desc: 'The ID of the runner' optional :description, type: String, desc: 'The description of the runner' optional :active, type: Boolean, desc: 'The state of a runner' - optional :tag_list, type: Array[String], desc: 'The list of tags for a runner' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The list of tags for a runner' optional :run_untagged, type: Boolean, desc: 'Flag indicating the runner can execute untagged jobs' optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' optional :access_level, type: String, values: ::Ci::Runner.access_levels.keys, @@ -146,7 +146,7 @@ class Runners < Grape::API desc: 'The type of the runners to show' optional :status, type: String, values: ::Ci::Runner::AVAILABLE_STATUSES, desc: 'The status of the runners to show' - optional :tag_list, type: Array[String], desc: 'The tags of the runners to show' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show' use :pagination end get ':id/runners' do @@ -209,7 +209,7 @@ class Runners < Grape::API desc: 'The type of the runners to show' optional :status, type: String, values: ::Ci::Runner::AVAILABLE_STATUSES, desc: 'The status of the runners to show' - optional :tag_list, type: Array[String], desc: 'The tags of the runners to show' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The tags of the runners to show' use :pagination end get ':id/runners' do diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index fcd317b2daac..140351c9e5ca 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -3,7 +3,7 @@ require 'mime/types' module API - class CommitStatuses < Grape::API + class CommitStatuses < Grape::API::Instance params do requires :id, type: String, desc: 'The ID of a project' end diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 086a1b7c4024..1a0fe393753d 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -3,7 +3,7 @@ require 'mime/types' module API - class Commits < Grape::API + class Commits < Grape::API::Instance include PaginationParams before do diff --git a/lib/api/container_registry_event.rb b/lib/api/container_registry_event.rb index 6d93cc653361..0b7c35cadbd4 100644 --- a/lib/api/container_registry_event.rb +++ b/lib/api/container_registry_event.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ContainerRegistryEvent < Grape::API + class ContainerRegistryEvent < Grape::API::Instance DOCKER_DISTRIBUTION_EVENTS_V1_JSON = 'application/vnd.docker.distribution.events.v1+json' before { authenticate_registry_notification! } diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 3259b6153696..ad37b7578ad8 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class DeployKeys < Grape::API + class DeployKeys < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/deploy_tokens.rb b/lib/api/deploy_tokens.rb index 0fbbd96cf020..96aa2445f565 100644 --- a/lib/api/deploy_tokens.rb +++ b/lib/api/deploy_tokens.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class DeployTokens < Grape::API + class DeployTokens < Grape::API::Instance include PaginationParams helpers do @@ -56,7 +56,7 @@ def scope_params params do requires :name, type: String, desc: "New deploy token's name" - requires :scopes, type: Array[String], values: ::DeployToken::AVAILABLE_SCOPES.map(&:to_s), + requires :scopes, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, values: ::DeployToken::AVAILABLE_SCOPES.map(&:to_s), desc: 'Indicates the deploy token scopes. Must be at least one of "read_repository", "read_registry", "write_registry", "read_package_registry", or "write_package_registry".' optional :expires_at, type: DateTime, desc: 'Expiration date for the deploy token. Does not expire if no value is provided.' optional :username, type: String, desc: 'Username for deploy token. Default is `gitlab+deploy-token-{n}`' @@ -119,7 +119,7 @@ def scope_params params do requires :name, type: String, desc: 'The name of the deploy token' - requires :scopes, type: Array[String], values: ::DeployToken::AVAILABLE_SCOPES.map(&:to_s), + requires :scopes, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, values: ::DeployToken::AVAILABLE_SCOPES.map(&:to_s), desc: 'Indicates the deploy token scopes. Must be at least one of "read_repository", "read_registry", "write_registry", "read_package_registry", or "write_package_registry".' optional :expires_at, type: DateTime, desc: 'Expiration date for the deploy token. Does not expire if no value is provided.' optional :username, type: String, desc: 'Username for deploy token. Default is `gitlab+deploy-token-{n}`' diff --git a/lib/api/deployments.rb b/lib/api/deployments.rb index cb1dca11e872..87144fd31cca 100644 --- a/lib/api/deployments.rb +++ b/lib/api/deployments.rb @@ -2,7 +2,7 @@ module API # Deployments RESTful API endpoints - class Deployments < Grape::API + class Deployments < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 7b453ada41c5..29a13e4420dc 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Discussions < Grape::API + class Discussions < Grape::API::Instance include PaginationParams helpers ::API::Helpers::NotesHelpers helpers ::RendersNotes diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 28019ce77961..b825904e2c5f 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -2,7 +2,7 @@ module API # Environments RESTfull API endpoints - class Environments < Grape::API + class Environments < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/error_tracking.rb b/lib/api/error_tracking.rb index 14888037f539..64ec6f0a57a6 100644 --- a/lib/api/error_tracking.rb +++ b/lib/api/error_tracking.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ErrorTracking < Grape::API + class ErrorTracking < Grape::API::Instance before { authenticate! } params do diff --git a/lib/api/events.rb b/lib/api/events.rb index e4c017fab42b..0b79431a76dd 100644 --- a/lib/api/events.rb +++ b/lib/api/events.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Events < Grape::API + class Events < Grape::API::Instance include PaginationParams include APIGuard helpers ::API::Helpers::EventsHelpers diff --git a/lib/api/features.rb b/lib/api/features.rb index 3fb3fc92e42c..9d011d658f63 100644 --- a/lib/api/features.rb +++ b/lib/api/features.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Features < Grape::API + class Features < Grape::API::Instance before { authenticated_as_admin! } helpers do diff --git a/lib/api/files.rb b/lib/api/files.rb index c525897b8fee..748bdfa894d5 100644 --- a/lib/api/files.rb +++ b/lib/api/files.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Files < Grape::API + class Files < Grape::API::Instance include APIGuard FILE_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(file_path: API::NO_SLASH_URL_PART_REGEX) diff --git a/lib/api/freeze_periods.rb b/lib/api/freeze_periods.rb index 9c7e5a5832df..b8254ee9ab4d 100644 --- a/lib/api/freeze_periods.rb +++ b/lib/api/freeze_periods.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class FreezePeriods < Grape::API + class FreezePeriods < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/group_boards.rb b/lib/api/group_boards.rb index 88d04e70e119..7efc12121d24 100644 --- a/lib/api/group_boards.rb +++ b/lib/api/group_boards.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupBoards < Grape::API + class GroupBoards < Grape::API::Instance include BoardsResponses include PaginationParams diff --git a/lib/api/group_clusters.rb b/lib/api/group_clusters.rb index 2c12c6387fb6..c6d10f22bb45 100644 --- a/lib/api/group_clusters.rb +++ b/lib/api/group_clusters.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupClusters < Grape::API + class GroupClusters < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/group_container_repositories.rb b/lib/api/group_container_repositories.rb index d34317b5271b..25b3059f63bb 100644 --- a/lib/api/group_container_repositories.rb +++ b/lib/api/group_container_repositories.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupContainerRepositories < Grape::API + class GroupContainerRepositories < Grape::API::Instance include PaginationParams before { authorize_read_group_container_images! } diff --git a/lib/api/group_export.rb b/lib/api/group_export.rb index d3010b6d1471..dc14813eefc3 100644 --- a/lib/api/group_export.rb +++ b/lib/api/group_export.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupExport < Grape::API + class GroupExport < Grape::API::Instance helpers Helpers::RateLimiter before do diff --git a/lib/api/group_import.rb b/lib/api/group_import.rb index afcbc24d3ce2..b82d9fc519a2 100644 --- a/lib/api/group_import.rb +++ b/lib/api/group_import.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupImport < Grape::API + class GroupImport < Grape::API::Instance helpers Helpers::FileUploadHelpers helpers do diff --git a/lib/api/group_labels.rb b/lib/api/group_labels.rb index 7585293031f0..56f2b769464f 100644 --- a/lib/api/group_labels.rb +++ b/lib/api/group_labels.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupLabels < Grape::API + class GroupLabels < Grape::API::Instance include PaginationParams helpers ::API::Helpers::LabelHelpers diff --git a/lib/api/group_milestones.rb b/lib/api/group_milestones.rb index 9e9f51012855..82f5df793564 100644 --- a/lib/api/group_milestones.rb +++ b/lib/api/group_milestones.rb @@ -1,13 +1,11 @@ # frozen_string_literal: true module API - class GroupMilestones < Grape::API + class GroupMilestones < Grape::API::Instance include MilestoneResponses include PaginationParams - before do - authenticate! - end + before { authenticate! } params do requires :id, type: String, desc: 'The ID of a group' diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb index 95aa587d0c79..d3ca1c79e73d 100644 --- a/lib/api/group_variables.rb +++ b/lib/api/group_variables.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class GroupVariables < Grape::API + class GroupVariables < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 6e07bb467217..4671e82ab66b 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Groups < Grape::API + class Groups < Grape::API::Instance include PaginationParams include Helpers::CustomAttributes @@ -16,7 +16,7 @@ class Groups < Grape::API params :group_list_params do use :statistics_params - optional :skip_groups, type: Array[Integer], desc: 'Array of group ids to exclude from list' + optional :skip_groups, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of group ids to exclude from list' optional :all_available, type: Boolean, desc: 'Show all group that you have access to' optional :search, type: String, desc: 'Search for a specific group' optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user' diff --git a/lib/api/helpers/common_helpers.rb b/lib/api/helpers/common_helpers.rb index 32a15381f27e..a44fd4b0a5b1 100644 --- a/lib/api/helpers/common_helpers.rb +++ b/lib/api/helpers/common_helpers.rb @@ -12,6 +12,26 @@ def convert_parameters_from_legacy_format(params) end end end + + # Grape v1.3.3 no longer automatically coerces an Array + # type to an empty array if the value is nil. + def coerce_nil_params_to_array! + keys_to_coerce = params_with_array_types + + params.each do |key, val| + params[key] = Array(val) if val.nil? && keys_to_coerce.include?(key) + end + end + + def params_with_array_types + options[:route_options][:params].map do |key, val| + param_type = val[:type] + # Search for parameters with Array types (e.g. "[String]", "[Integer]", etc.) + if param_type =~ %r(\[\w*\]) + key + end + end.compact.to_set + end end end end diff --git a/lib/api/helpers/merge_requests_helpers.rb b/lib/api/helpers/merge_requests_helpers.rb index 9dab2a88f0b0..939f29a04b3e 100644 --- a/lib/api/helpers/merge_requests_helpers.rb +++ b/lib/api/helpers/merge_requests_helpers.rb @@ -24,7 +24,7 @@ module MergeRequestsHelpers optional :milestone, type: String, desc: 'Return merge requests for a specific milestone' optional :labels, type: Array[String], - coerce_with: Validations::Types::LabelsList.coerce, + coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :with_labels_details, type: Boolean, desc: 'Return titles of labels and other details', default: false optional :with_merge_status_recheck, type: Boolean, desc: 'Request that stale merge statuses be rechecked asynchronously', default: false diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb index 92030415d02e..d541d3d42db1 100644 --- a/lib/api/helpers/projects_helpers.rb +++ b/lib/api/helpers/projects_helpers.rb @@ -48,7 +48,7 @@ module ProjectsHelpers optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed' optional :allow_merge_on_skipped_pipeline, type: Boolean, desc: 'Allow to merge if pipeline is skipped' optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved' - optional :tag_list, type: Array[String], desc: 'The list of tags for a project' + optional :tag_list, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The list of tags for a project' # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab/issues/14960 optional :avatar, type: File, desc: 'Avatar image for project' # rubocop:disable Scalability/FileUploads optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line' diff --git a/lib/api/import_github.rb b/lib/api/import_github.rb index 21d4928193ee..986827e80be2 100644 --- a/lib/api/import_github.rb +++ b/lib/api/import_github.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ImportGithub < Grape::API + class ImportGithub < Grape::API::Instance rescue_from Octokit::Unauthorized, with: :provider_unauthorized helpers do diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 83615c7639b8..6d4a4fc9c8ba 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -3,7 +3,7 @@ module API # Internal access API module Internal - class Base < Grape::API + class Base < Grape::API::Instance before { authenticate_by_gitlab_shell_token! } before do diff --git a/lib/api/internal/pages.rb b/lib/api/internal/pages.rb index 6c8da414e4db..5f8d23f15fa4 100644 --- a/lib/api/internal/pages.rb +++ b/lib/api/internal/pages.rb @@ -3,7 +3,7 @@ module API # Pages Internal API module Internal - class Pages < Grape::API + class Pages < Grape::API::Instance before do authenticate_gitlab_pages_request! end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 2374ac11f4a9..93b0fbc5223e 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Issues < Grape::API + class Issues < Grape::API::Instance include PaginationParams helpers Helpers::IssuesHelpers helpers Helpers::RateLimiter @@ -10,9 +10,9 @@ class Issues < Grape::API helpers do params :negatable_issue_filter_params do - optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' + optional :labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :milestone, type: String, desc: 'Milestone title' - optional :iids, type: Array[Integer], desc: 'The IID array of issues' + optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IID array of issues' optional :search, type: String, desc: 'Search issues for text present in the title, description, or any combination of these' optional :in, type: String, desc: '`title`, `description`, or a string joining them with comma' @@ -62,12 +62,12 @@ class Issues < Grape::API params :issue_params do optional :description, type: String, desc: 'The description of an issue' - optional :assignee_ids, type: Array[Integer], desc: 'The array of user IDs to assign issue' + optional :assignee_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The array of user IDs to assign issue' optional :assignee_id, type: Integer, desc: '[Deprecated] The ID of a user to assign issue' optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue' - optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' - optional :add_labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' - optional :remove_labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' + optional :labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' + optional :add_labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' + optional :remove_labels, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :due_date, type: String, desc: 'Date string in the format YEAR-MONTH-DAY' optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential' optional :discussion_locked, type: Boolean, desc: " Boolean parameter indicating if the issue's discussion is locked" diff --git a/lib/api/job_artifacts.rb b/lib/api/job_artifacts.rb index 6a82256cc96a..61c279a76e9f 100644 --- a/lib/api/job_artifacts.rb +++ b/lib/api/job_artifacts.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class JobArtifacts < Grape::API + class JobArtifacts < Grape::API::Instance before { authenticate_non_get! } # EE::API::JobArtifacts would override the following helpers diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 9432b992d745..bcc00429dd6c 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Jobs < Grape::API + class Jobs < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/keys.rb b/lib/api/keys.rb index b730e0270638..c014641ca043 100644 --- a/lib/api/keys.rb +++ b/lib/api/keys.rb @@ -2,7 +2,7 @@ module API # Keys API - class Keys < Grape::API + class Keys < Grape::API::Instance before { authenticate! } resource :keys do diff --git a/lib/api/labels.rb b/lib/api/labels.rb index 2b283d82e4ae..edf4a8ca14ea 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Labels < Grape::API + class Labels < Grape::API::Instance include PaginationParams helpers ::API::Helpers::LabelHelpers diff --git a/lib/api/lint.rb b/lib/api/lint.rb index a7672021db01..f7796b1e9694 100644 --- a/lib/api/lint.rb +++ b/lib/api/lint.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Lint < Grape::API + class Lint < Grape::API::Instance namespace :ci do desc 'Validation of .gitlab-ci.yml content' params do diff --git a/lib/api/markdown.rb b/lib/api/markdown.rb index de77bef43ce9..a0822271cca5 100644 --- a/lib/api/markdown.rb +++ b/lib/api/markdown.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Markdown < Grape::API + class Markdown < Grape::API::Instance params do requires :text, type: String, desc: "The markdown text to render" optional :gfm, type: Boolean, desc: "Render text using GitLab Flavored Markdown" diff --git a/lib/api/members.rb b/lib/api/members.rb index 341d48374683..3de7ae13d5ea 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Members < Grape::API + class Members < Grape::API::Instance include PaginationParams before { authenticate! } @@ -18,7 +18,7 @@ class Members < Grape::API end params do optional :query, type: String, desc: 'A query string to search for members' - optional :user_ids, type: Array[Integer], desc: 'Array of user ids to look up for membership' + optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of user ids to look up for membership' optional :show_seat_info, type: Boolean, desc: 'Show seat information for members' use :optional_filter_params_ee use :pagination @@ -37,7 +37,7 @@ class Members < Grape::API end params do optional :query, type: String, desc: 'A query string to search for members' - optional :user_ids, type: Array[Integer], desc: 'Array of user ids to look up for membership' + optional :user_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Array of user ids to look up for membership' optional :show_seat_info, type: Boolean, desc: 'Show seat information for members' use :pagination end diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index 6ad30aa56e0f..3e43fe8b257c 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -2,7 +2,7 @@ module API # MergeRequestDiff API - class MergeRequestDiffs < Grape::API + class MergeRequestDiffs < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 773a451d3a8e..82f103ff0aef 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class MergeRequests < Grape::API + class MergeRequests < Grape::API::Instance include PaginationParams CONTEXT_COMMITS_POST_LIMIT = 20 @@ -179,11 +179,11 @@ def handle_merge_request_errors!(errors) params :optional_params do optional :description, type: String, desc: 'The description of the merge request' optional :assignee_id, type: Integer, desc: 'The ID of a user to assign the merge request' - optional :assignee_ids, type: Array[Integer], desc: 'The array of user IDs to assign issue' + optional :assignee_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The array of user IDs to assign issue' optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request' - optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' - optional :add_labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' - optional :remove_labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names' + optional :labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' + optional :add_labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' + optional :remove_labels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Comma-separated list of label names' optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging' optional :allow_collaboration, type: Boolean, desc: 'Allow commits from members who can merge to the target branch' optional :allow_maintainer_to_push, type: Boolean, as: :allow_collaboration, desc: '[deprecated] See allow_collaboration' @@ -198,7 +198,7 @@ def handle_merge_request_errors!(errors) end params do use :merge_requests_params - optional :iids, type: Array[Integer], desc: 'The IID array of merge requests' + optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IID array of merge requests' end get ":id/merge_requests" do authorize! :read_merge_request, user_project @@ -315,7 +315,7 @@ def handle_merge_request_errors!(errors) end params do - requires :commits, type: Array, allow_blank: false, desc: 'List of context commits sha' + requires :commits, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, allow_blank: false, desc: 'List of context commits sha' end desc 'create context commits of merge request' do success Entities::Commit @@ -345,7 +345,7 @@ def handle_merge_request_errors!(errors) end params do - requires :commits, type: Array, allow_blank: false, desc: 'List of context commits sha' + requires :commits, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, allow_blank: false, desc: 'List of context commits sha' end desc 'remove context commits of merge request' delete ':id/merge_requests/:merge_request_iid/context_commits' do diff --git a/lib/api/metrics/dashboard/annotations.rb b/lib/api/metrics/dashboard/annotations.rb index c8ec4d296570..e07762ac6d35 100644 --- a/lib/api/metrics/dashboard/annotations.rb +++ b/lib/api/metrics/dashboard/annotations.rb @@ -3,7 +3,7 @@ module API module Metrics module Dashboard - class Annotations < Grape::API + class Annotations < Grape::API::Instance desc 'Create a new monitoring dashboard annotation' do success Entities::Metrics::Dashboard::Annotation end diff --git a/lib/api/metrics/user_starred_dashboards.rb b/lib/api/metrics/user_starred_dashboards.rb index 85fc0f33ed82..263d23942766 100644 --- a/lib/api/metrics/user_starred_dashboards.rb +++ b/lib/api/metrics/user_starred_dashboards.rb @@ -2,7 +2,7 @@ module API module Metrics - class UserStarredDashboards < Grape::API + class UserStarredDashboards < Grape::API::Instance resource :projects do desc 'Marks selected metrics dashboard as starred' do success Entities::Metrics::UserStarredDashboard diff --git a/lib/api/milestone_responses.rb b/lib/api/milestone_responses.rb index 62e159ab0034..8ff885983bcc 100644 --- a/lib/api/milestone_responses.rb +++ b/lib/api/milestone_responses.rb @@ -15,7 +15,7 @@ module MilestoneResponses params :list_params do optional :state, type: String, values: %w[active closed all], default: 'all', desc: 'Return "active", "closed", or "all" milestones' - optional :iids, type: Array[Integer], desc: 'The IIDs of the milestones' + optional :iids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'The IIDs of the milestones' optional :title, type: String, desc: 'The title of the milestones' optional :search, type: String, desc: 'The search criteria for the title or description of the milestone' use :pagination diff --git a/lib/api/namespaces.rb b/lib/api/namespaces.rb index e40a5dde7ce7..e1f279df045a 100644 --- a/lib/api/namespaces.rb +++ b/lib/api/namespaces.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Namespaces < Grape::API + class Namespaces < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 3eafc1ead77b..4fb7bffb3d55 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Notes < Grape::API + class Notes < Grape::API::Instance include PaginationParams helpers ::API::Helpers::NotesHelpers diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index 8cb46bd3ad6a..f8b621c1c384 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -2,7 +2,7 @@ module API # notification_settings API - class NotificationSettings < Grape::API + class NotificationSettings < Grape::API::Instance before { authenticate! } helpers ::API::Helpers::MembersHelpers diff --git a/lib/api/pages.rb b/lib/api/pages.rb index ee7fe669519a..79a6b527581e 100644 --- a/lib/api/pages.rb +++ b/lib/api/pages.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Pages < Grape::API + class Pages < Grape::API::Instance before do require_pages_config_enabled! authenticated_with_can_read_all_resources! diff --git a/lib/api/pages_domains.rb b/lib/api/pages_domains.rb index 4c3d2d131acf..7d27b575efa2 100644 --- a/lib/api/pages_domains.rb +++ b/lib/api/pages_domains.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class PagesDomains < Grape::API + class PagesDomains < Grape::API::Instance include PaginationParams PAGES_DOMAINS_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(domain: API::NO_SLASH_URL_PART_REGEX) diff --git a/lib/api/pagination_params.rb b/lib/api/pagination_params.rb index ae03595eb25e..a232b58d3f7e 100644 --- a/lib/api/pagination_params.rb +++ b/lib/api/pagination_params.rb @@ -4,7 +4,7 @@ module API # Concern for declare pagination params. # # @example - # class CustomApiResource < Grape::API + # class CustomApiResource < Grape::API::Instance # include PaginationParams # # params do diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb index 7486518cd4a0..9af16f619670 100644 --- a/lib/api/pipeline_schedules.rb +++ b/lib/api/pipeline_schedules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class PipelineSchedules < Grape::API + class PipelineSchedules < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index b27ff2d24f8e..1aac7b7deb42 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Pipelines < Grape::API + class Pipelines < Grape::API::Instance include PaginationParams before { authenticate_non_get! } diff --git a/lib/api/project_clusters.rb b/lib/api/project_clusters.rb index 299301aabc46..e1dfb647fa08 100644 --- a/lib/api/project_clusters.rb +++ b/lib/api/project_clusters.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectClusters < Grape::API + class ProjectClusters < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/project_container_repositories.rb b/lib/api/project_container_repositories.rb index 2a0099018d94..8f2a62bc5a4e 100644 --- a/lib/api/project_container_repositories.rb +++ b/lib/api/project_container_repositories.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectContainerRepositories < Grape::API + class ProjectContainerRepositories < Grape::API::Instance include PaginationParams REPOSITORY_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge( diff --git a/lib/api/project_events.rb b/lib/api/project_events.rb index 734311e1142d..726e693826e3 100644 --- a/lib/api/project_events.rb +++ b/lib/api/project_events.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectEvents < Grape::API + class ProjectEvents < Grape::API::Instance include PaginationParams include APIGuard helpers ::API::Helpers::EventsHelpers diff --git a/lib/api/project_export.rb b/lib/api/project_export.rb index 4b35f245b8cc..d11c47f8d78e 100644 --- a/lib/api/project_export.rb +++ b/lib/api/project_export.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectExport < Grape::API + class ProjectExport < Grape::API::Instance helpers Helpers::RateLimiter before do diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb index 0e7576c9243c..7cea44e63049 100644 --- a/lib/api/project_hooks.rb +++ b/lib/api/project_hooks.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectHooks < Grape::API + class ProjectHooks < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/project_import.rb b/lib/api/project_import.rb index 17d08d14a20c..9f43c3c79939 100644 --- a/lib/api/project_import.rb +++ b/lib/api/project_import.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectImport < Grape::API + class ProjectImport < Grape::API::Instance include PaginationParams MAXIMUM_FILE_SIZE = 50.megabytes diff --git a/lib/api/project_milestones.rb b/lib/api/project_milestones.rb index 8643854a655b..2f8dd1085dcd 100644 --- a/lib/api/project_milestones.rb +++ b/lib/api/project_milestones.rb @@ -1,13 +1,11 @@ # frozen_string_literal: true module API - class ProjectMilestones < Grape::API + class ProjectMilestones < Grape::API::Instance include PaginationParams include MilestoneResponses - before do - authenticate! - end + before { authenticate! } params do requires :id, type: String, desc: 'The ID of a project' diff --git a/lib/api/project_repository_storage_moves.rb b/lib/api/project_repository_storage_moves.rb index 5de623102fbf..c318907542bb 100644 --- a/lib/api/project_repository_storage_moves.rb +++ b/lib/api/project_repository_storage_moves.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectRepositoryStorageMoves < Grape::API + class ProjectRepositoryStorageMoves < Grape::API::Instance include PaginationParams before { authenticated_as_admin! } diff --git a/lib/api/project_snapshots.rb b/lib/api/project_snapshots.rb index 175fbb2ce928..360000861fcc 100644 --- a/lib/api/project_snapshots.rb +++ b/lib/api/project_snapshots.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectSnapshots < Grape::API + class ProjectSnapshots < Grape::API::Instance helpers ::API::Helpers::ProjectSnapshotsHelpers before { authorize_read_git_snapshot! } diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index 68f4a0dcb656..d93454f949d5 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectSnippets < Grape::API + class ProjectSnippets < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/project_statistics.rb b/lib/api/project_statistics.rb index 14ee0f75513c..2196801096fc 100644 --- a/lib/api/project_statistics.rb +++ b/lib/api/project_statistics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectStatistics < Grape::API + class ProjectStatistics < Grape::API::Instance before do authenticate! authorize! :daily_statistics, user_project diff --git a/lib/api/project_templates.rb b/lib/api/project_templates.rb index cfcc7f5212db..f0fe4d85c8ff 100644 --- a/lib/api/project_templates.rb +++ b/lib/api/project_templates.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProjectTemplates < Grape::API + class ProjectTemplates < Grape::API::Instance include PaginationParams TEMPLATE_TYPES = %w[dockerfiles gitignores gitlab_ci_ymls licenses].freeze diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f4582d36f4b1..d24dab63bd92 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -3,7 +3,7 @@ require_dependency 'declarative_policy' module API - class Projects < Grape::API + class Projects < Grape::API::Instance include PaginationParams include Helpers::CustomAttributes @@ -546,7 +546,7 @@ def translate_params_for_compatibility(params) end params do optional :search, type: String, desc: 'Return list of users matching the search criteria' - optional :skip_users, type: Array[Integer], desc: 'Filter out users with the specified IDs' + optional :skip_users, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: 'Filter out users with the specified IDs' use :pagination end get ':id/users' do diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb index 1fd86d1e7207..b0a7f898eeca 100644 --- a/lib/api/protected_branches.rb +++ b/lib/api/protected_branches.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProtectedBranches < Grape::API + class ProtectedBranches < Grape::API::Instance include PaginationParams BRANCH_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(name: API::NO_SLASH_URL_PART_REGEX) diff --git a/lib/api/protected_tags.rb b/lib/api/protected_tags.rb index ee13473c8485..aaa31cb7cc6c 100644 --- a/lib/api/protected_tags.rb +++ b/lib/api/protected_tags.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ProtectedTags < Grape::API + class ProtectedTags < Grape::API::Instance include PaginationParams TAG_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(name: API::NO_SLASH_URL_PART_REGEX) diff --git a/lib/api/release/links.rb b/lib/api/release/links.rb index 07c27f395390..7e1815480a51 100644 --- a/lib/api/release/links.rb +++ b/lib/api/release/links.rb @@ -2,7 +2,7 @@ module API module Release - class Links < Grape::API + class Links < Grape::API::Instance include PaginationParams RELEASE_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS diff --git a/lib/api/releases.rb b/lib/api/releases.rb index a5bb1a44f1f3..30c5e06053e0 100644 --- a/lib/api/releases.rb +++ b/lib/api/releases.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Releases < Grape::API + class Releases < Grape::API::Instance include PaginationParams RELEASE_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS @@ -54,7 +54,7 @@ class Releases < Grape::API requires :url, type: String end end - optional :milestones, type: Array, desc: 'The titles of the related milestones', default: [] + optional :milestones, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce, desc: 'The titles of the related milestones', default: [] optional :released_at, type: DateTime, desc: 'The date when the release will be/was ready. Defaults to the current time.' end route_setting :authentication, job_token_allowed: true diff --git a/lib/api/remote_mirrors.rb b/lib/api/remote_mirrors.rb index 0808541d3c7f..d1def05808b3 100644 --- a/lib/api/remote_mirrors.rb +++ b/lib/api/remote_mirrors.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class RemoteMirrors < Grape::API + class RemoteMirrors < Grape::API::Instance include PaginationParams before do diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb index bf4f08ce3902..81702f8f02a1 100644 --- a/lib/api/repositories.rb +++ b/lib/api/repositories.rb @@ -3,7 +3,7 @@ require 'mime/types' module API - class Repositories < Grape::API + class Repositories < Grape::API::Instance include PaginationParams helpers ::API::Helpers::HeadersHelpers @@ -143,7 +143,7 @@ def assign_blob_vars! success Entities::Commit end params do - requires :refs, type: Array[String] + requires :refs, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce end get ':id/repository/merge_base' do refs = params[:refs] diff --git a/lib/api/resource_label_events.rb b/lib/api/resource_label_events.rb index 1fa6898b92c3..a8d3419528c9 100644 --- a/lib/api/resource_label_events.rb +++ b/lib/api/resource_label_events.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ResourceLabelEvents < Grape::API + class ResourceLabelEvents < Grape::API::Instance include PaginationParams helpers ::API::Helpers::NotesHelpers diff --git a/lib/api/resource_milestone_events.rb b/lib/api/resource_milestone_events.rb index e466e2363715..a8f221f8740d 100644 --- a/lib/api/resource_milestone_events.rb +++ b/lib/api/resource_milestone_events.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class ResourceMilestoneEvents < Grape::API + class ResourceMilestoneEvents < Grape::API::Instance include PaginationParams helpers ::API::Helpers::NotesHelpers diff --git a/lib/api/search.rb b/lib/api/search.rb index ac00d3682a01..092da6571c80 100644 --- a/lib/api/search.rb +++ b/lib/api/search.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Search < Grape::API + class Search < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/services.rb b/lib/api/services.rb index 5fd5c6bd9b09..9ee1822339c8 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true module API - class Services < Grape::API + class Services < Grape::API::Instance services = Helpers::ServicesHelpers.services service_classes = Helpers::ServicesHelpers.service_classes diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 0bf5eed26b43..3463e29041b2 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Settings < Grape::API + class Settings < Grape::API::Instance before { authenticated_as_admin! } helpers Helpers::SettingsHelpers @@ -49,7 +49,7 @@ def filter_attributes_using_license(attrs) optional :default_project_visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The default project visibility' optional :default_projects_limit, type: Integer, desc: 'The maximum number of personal projects' optional :default_snippet_visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The default snippet visibility' - optional :disabled_oauth_sign_in_sources, type: Array[String], desc: 'Disable certain OAuth sign-in sources' + optional :disabled_oauth_sign_in_sources, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Disable certain OAuth sign-in sources' optional :domain_blacklist_enabled, type: Boolean, desc: 'Enable domain blacklist for sign ups' optional :domain_blacklist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com' optional :domain_whitelist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'ONLY users with e-mail addresses that match these domain(s) will be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: domain.com, *.domain.com' @@ -79,7 +79,8 @@ def filter_attributes_using_license(attrs) requires :housekeeping_incremental_repack_period, type: Integer, desc: "Number of Git pushes after which an incremental 'git repack' is run." end optional :html_emails_enabled, type: Boolean, desc: 'By default GitLab sends emails in HTML and plain text formats so mail clients can choose what format to use. Disable this option if you only want to send emails in plain text format.' - optional :import_sources, type: Array[String], values: %w[github bitbucket bitbucket_server gitlab google_code fogbugz git gitlab_project gitea manifest phabricator], + optional :import_sources, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, + values: %w[github bitbucket bitbucket_server gitlab google_code fogbugz git gitlab_project gitea manifest phabricator], desc: 'Enabled sources for code import during project creation. OmniAuth must be configured for GitHub, Bitbucket, and GitLab.com' optional :max_artifacts_size, type: Integer, desc: "Set the maximum file size for each job's artifacts" optional :max_attachment_size, type: Integer, desc: 'Maximum attachment size in MB' @@ -113,13 +114,13 @@ def filter_attributes_using_license(attrs) requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha' end optional :repository_checks_enabled, type: Boolean, desc: "GitLab will periodically run 'git fsck' in all project and wiki repositories to look for silent disk corruption issues." - optional :repository_storages, type: Array[String], desc: 'Storage paths for new projects' + optional :repository_storages, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Storage paths for new projects' optional :repository_storages_weighted, type: Hash, desc: 'Storage paths for new projects with a weighted value between 0 and 100' optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to set up Two-factor authentication' given require_two_factor_authentication: ->(val) { val } do requires :two_factor_grace_period, type: Integer, desc: 'Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication' end - optional :restricted_visibility_levels, type: Array[String], desc: 'Selected levels cannot be used by non-admin users for groups, projects or snippets. If the public level is restricted, user profiles are only visible to logged in users.' + optional :restricted_visibility_levels, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Selected levels cannot be used by non-admin users for groups, projects or snippets. If the public level is restricted, user profiles are only visible to logged in users.' optional :send_user_confirmation_email, type: Boolean, desc: 'Send confirmation email on sign-up' optional :session_expire_delay, type: Integer, desc: 'Session duration in minutes. GitLab restart is required to apply changes.' optional :shared_runners_enabled, type: Boolean, desc: 'Enable shared runners for new projects' diff --git a/lib/api/sidekiq_metrics.rb b/lib/api/sidekiq_metrics.rb index 693c20cb73aa..de1373144e37 100644 --- a/lib/api/sidekiq_metrics.rb +++ b/lib/api/sidekiq_metrics.rb @@ -3,7 +3,7 @@ require 'sidekiq/api' module API - class SidekiqMetrics < Grape::API + class SidekiqMetrics < Grape::API::Instance before { authenticated_as_admin! } helpers do diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index be58b832f979..3e6ccf7c0cf3 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -2,7 +2,7 @@ module API # Snippets API - class Snippets < Grape::API + class Snippets < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/statistics.rb b/lib/api/statistics.rb index d2dce34dfa5b..3869fd3ac765 100644 --- a/lib/api/statistics.rb +++ b/lib/api/statistics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Statistics < Grape::API + class Statistics < Grape::API::Instance before { authenticated_as_admin! } COUNTED_ITEMS = [Project, User, Group, ForkNetworkMember, ForkNetwork, Issue, diff --git a/lib/api/submodules.rb b/lib/api/submodules.rb index 72d7d9941020..34d21d3d7d86 100644 --- a/lib/api/submodules.rb +++ b/lib/api/submodules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Submodules < Grape::API + class Submodules < Grape::API::Instance before { authenticate! } helpers do diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb index dfb54446ddf3..533663fb087e 100644 --- a/lib/api/subscriptions.rb +++ b/lib/api/subscriptions.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Subscriptions < Grape::API + class Subscriptions < Grape::API::Instance helpers ::API::Helpers::LabelHelpers before { authenticate! } diff --git a/lib/api/suggestions.rb b/lib/api/suggestions.rb index 05aaa8a6f419..38e96c080f2f 100644 --- a/lib/api/suggestions.rb +++ b/lib/api/suggestions.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Suggestions < Grape::API + class Suggestions < Grape::API::Instance before { authenticate! } resource :suggestions do @@ -25,7 +25,7 @@ class Suggestions < Grape::API success Entities::Suggestion end params do - requires :ids, type: Array[String], desc: "An array of suggestion ID's" + requires :ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce, desc: "An array of suggestion ID's" end put 'batch_apply' do ids = params[:ids] diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb index 51fae0e54aac..d8e0a425625d 100644 --- a/lib/api/system_hooks.rb +++ b/lib/api/system_hooks.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class SystemHooks < Grape::API + class SystemHooks < Grape::API::Instance include PaginationParams before do diff --git a/lib/api/tags.rb b/lib/api/tags.rb index 796b14506022..c1fbd3ca7c6d 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Tags < Grape::API + class Tags < Grape::API::Instance include PaginationParams TAG_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(tag_name: API::NO_SLASH_URL_PART_REGEX) diff --git a/lib/api/templates.rb b/lib/api/templates.rb index 51f357d94770..80a97aae4296 100644 --- a/lib/api/templates.rb +++ b/lib/api/templates.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Templates < Grape::API + class Templates < Grape::API::Instance include PaginationParams GLOBAL_TEMPLATE_TYPES = { diff --git a/lib/api/terraform/state.rb b/lib/api/terraform/state.rb index e7c9627c7537..6b9809c76a4f 100644 --- a/lib/api/terraform/state.rb +++ b/lib/api/terraform/state.rb @@ -4,7 +4,7 @@ module API module Terraform - class State < Grape::API + class State < Grape::API::Instance include ::Gitlab::Utils::StrongMemoize default_format :json diff --git a/lib/api/todos.rb b/lib/api/todos.rb index e36ddf212777..4a73e3e0e94f 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Todos < Grape::API + class Todos < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 65e9118e3ef1..de67a1492749 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Triggers < Grape::API + class Triggers < Grape::API::Instance include PaginationParams HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase diff --git a/lib/api/user_counts.rb b/lib/api/user_counts.rb index 8df4b381bbfe..90127ecbc736 100644 --- a/lib/api/user_counts.rb +++ b/lib/api/user_counts.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class UserCounts < Grape::API + class UserCounts < Grape::API::Instance resource :user_counts do desc 'Return the user specific counts' do detail 'Open MR Count' diff --git a/lib/api/users.rb b/lib/api/users.rb index 3f89f1c253af..3124acd511fa 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Users < Grape::API + class Users < Grape::API::Instance include PaginationParams include APIGuard include Helpers::CustomAttributes diff --git a/lib/api/validations/types/comma_separated_to_array.rb b/lib/api/validations/types/comma_separated_to_array.rb index b551878abd1a..409eb67a3d33 100644 --- a/lib/api/validations/types/comma_separated_to_array.rb +++ b/lib/api/validations/types/comma_separated_to_array.rb @@ -10,7 +10,7 @@ def self.coerce when String value.split(',').map(&:strip) when Array - value.map { |v| v.to_s.split(',').map(&:strip) }.flatten + value.flat_map { |v| v.to_s.split(',').map(&:strip) } else [] end diff --git a/lib/api/validations/types/comma_separated_to_integer_array.rb b/lib/api/validations/types/comma_separated_to_integer_array.rb new file mode 100644 index 000000000000..b8ab08b3fd40 --- /dev/null +++ b/lib/api/validations/types/comma_separated_to_integer_array.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module API + module Validations + module Types + class CommaSeparatedToIntegerArray < CommaSeparatedToArray + def self.coerce + lambda do |value| + super.call(value).map(&:to_i) + end + end + end + end + end +end diff --git a/lib/api/validations/types/labels_list.rb b/lib/api/validations/types/labels_list.rb deleted file mode 100644 index 60277b99106a..000000000000 --- a/lib/api/validations/types/labels_list.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module API - module Validations - module Types - class LabelsList - def self.coerce - lambda do |value| - case value - when String - value.split(',').map(&:strip) - when Array - value.flat_map { |v| v.to_s.split(',').map(&:strip) } - when LabelsList - value - else - [] - end - end - end - end - end - end -end diff --git a/lib/api/validations/types/safe_file.rb b/lib/api/validations/types/safe_file.rb deleted file mode 100644 index 53b5790bfa21..000000000000 --- a/lib/api/validations/types/safe_file.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -# This module overrides the Grape type validator defined in -# https://github.com/ruby-grape/grape/blob/master/lib/grape/validations/types/file.rb -module API - module Validations - module Types - class SafeFile < ::Grape::Validations::Types::File - def value_coerced?(value) - super && value[:tempfile].is_a?(Tempfile) - end - end - end - end -end diff --git a/lib/api/validations/types/workhorse_file.rb b/lib/api/validations/types/workhorse_file.rb index 18d111f65561..e65e94fc8db7 100644 --- a/lib/api/validations/types/workhorse_file.rb +++ b/lib/api/validations/types/workhorse_file.rb @@ -3,15 +3,14 @@ module API module Validations module Types - class WorkhorseFile < Virtus::Attribute - def coerce(input) - # Processing of multipart file objects - # is already taken care of by Gitlab::Middleware::Multipart. - # Nothing to do here. - input + class WorkhorseFile + def self.parse(value) + raise "#{value.class} is not an UploadedFile type" unless parsed?(value) + + value end - def value_coerced?(value) + def self.parsed?(value) value.is_a?(::UploadedFile) end end diff --git a/lib/api/variables.rb b/lib/api/variables.rb index f3def756fe8d..2a051c2adae1 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Variables < Grape::API + class Variables < Grape::API::Instance include PaginationParams before { authenticate! } diff --git a/lib/api/version.rb b/lib/api/version.rb index 2d8c90260fa3..6a480fc2bd90 100644 --- a/lib/api/version.rb +++ b/lib/api/version.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Version < Grape::API + class Version < Grape::API::Instance helpers ::API::Helpers::GraphqlHelpers include APIGuard diff --git a/lib/api/wikis.rb b/lib/api/wikis.rb index 006dc257fe1d..713136e08872 100644 --- a/lib/api/wikis.rb +++ b/lib/api/wikis.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module API - class Wikis < Grape::API + class Wikis < Grape::API::Instance helpers ::API::Helpers::WikisHelpers helpers do @@ -112,7 +112,7 @@ class Wikis < Grape::API success Entities::WikiAttachment end params do - requires :file, types: [::API::Validations::Types::SafeFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded' + requires :file, types: [Rack::Multipart::UploadedFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded' optional :branch, type: String, desc: 'The name of the branch' end post ":id/wikis/attachments" do diff --git a/rubocop/cop/api/grape_api_instance.rb b/rubocop/cop/api/grape_api_instance.rb new file mode 100644 index 000000000000..de11b9ef3f6e --- /dev/null +++ b/rubocop/cop/api/grape_api_instance.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module API + class GrapeAPIInstance < RuboCop::Cop::Cop + # This cop checks that APIs subclass Grape::API::Instance with Grape v1.3+. + # + # @example + # + # # bad + # module API + # class Projects < Grape::API + # end + # end + # + # # good + # module API + # class Projects < Grape::API::Instance + # end + # end + MSG = 'Inherit from Grape::API::Instance instead of Grape::API. ' \ + 'For more details check the https://gitlab.com/gitlab-org/gitlab/-/issues/215230.' + + def_node_matcher :grape_api_definition, <<~PATTERN + (class + (const _ _) + (const + (const nil? :Grape) :API) + ... + ) + PATTERN + + def on_class(node) + grape_api_definition(node) do + add_offense(node.children[1]) + end + end + end + end + end +end diff --git a/rubocop/cop/api/grape_array_missing_coerce.rb b/rubocop/cop/api/grape_array_missing_coerce.rb new file mode 100644 index 000000000000..3d7a6a72d818 --- /dev/null +++ b/rubocop/cop/api/grape_array_missing_coerce.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module API + class GrapeArrayMissingCoerce < RuboCop::Cop::Cop + # This cop checks that Grape API parameters using an Array type + # implement a coerce_with method: + # + # https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#ensure-that-array-types-have-explicit-coercions + # + # @example + # + # # bad + # requires :values, type: Array[String] + # + # # good + # requires :values, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce + # + # end + MSG = 'This Grape parameter defines an Array but is missing a coerce_with definition. ' \ + 'For more details, see https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#ensure-that-array-types-have-explicit-coercions' + + def_node_matcher :grape_api_instance?, <<~PATTERN + (class + (const _ _) + (const + (const + (const nil? :Grape) :API) :Instance) + ... + ) + PATTERN + + def_node_matcher :grape_api_param_block?, <<~PATTERN + (send _ {:requires :optional} + (sym _) + $_) + PATTERN + + def_node_matcher :grape_type_def?, <<~PATTERN + (sym :type) + PATTERN + + def_node_matcher :grape_array_type?, <<~PATTERN + (send + (const nil? :Array) :[] + (const nil? _)) + PATTERN + + def_node_matcher :grape_coerce_with?, <<~PATTERN + (sym :coerce_with) + PATTERN + + def on_class(node) + @grape_api ||= grape_api_instance?(node) + end + + def on_send(node) + return unless @grape_api + + match = grape_api_param_block?(node) + + return unless match.is_a?(RuboCop::AST::HashNode) + + is_array_type = false + has_coerce_method = false + + match.each_pair do |first, second| + has_coerce_method ||= grape_coerce_with?(first) + + if grape_type_def?(first) && grape_array_type?(second) + is_array_type = true + end + end + + if is_array_type && !has_coerce_method + add_offense(node) + end + end + end + end + end +end diff --git a/spec/lib/api/helpers/common_helpers_spec.rb b/spec/lib/api/helpers/common_helpers_spec.rb new file mode 100644 index 000000000000..5162d2f10001 --- /dev/null +++ b/spec/lib/api/helpers/common_helpers_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::Helpers::CommonHelpers do + include Rack::Test::Methods + + subject do + Class.new(Grape::API) do + helpers API::Helpers::CommonHelpers + + before do + coerce_nil_params_to_array! + end + + params do + requires :id, type: String + optional :array, type: Array, coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce + optional :array_of_strings, type: Array[String], coerce_with: ::API::Validations::Types::CommaSeparatedToArray.coerce + optional :array_of_ints, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce + end + get ":id" do + params.to_json + end + end + end + + def app + subject + end + + describe '.coerce_nil_params_to_array!' do + let(:json_response) { Gitlab::Json.parse(last_response.body) } + + it 'converts all nil parameters to empty arrays' do + get '/test?array=&array_of_strings=&array_of_ints=' + + expect(json_response['array']).to eq([]) + expect(json_response['array_of_strings']).to eq([]) + expect(json_response['array_of_ints']).to eq([]) + end + + it 'leaves non-nil parameters alone' do + get '/test?array=&array_of_strings=test,me&array_of_ints=1,2' + + expect(json_response['array']).to eq([]) + expect(json_response['array_of_strings']).to eq(%w(test me)) + expect(json_response['array_of_ints']).to eq([1, 2]) + end + end +end diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb index 5b2d835f89ad..63fbf6e32dd3 100644 --- a/spec/requests/api/applications_spec.rb +++ b/spec/requests/api/applications_spec.rb @@ -74,14 +74,15 @@ expect(json_response['error']).to eq('scopes is missing') end - it 'does not allow creating an application with confidential set to nil' do + it 'defaults to creating an application with confidential' do expect do post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '', confidential: nil } - end.not_to change { Doorkeeper::Application.count } + end.to change { Doorkeeper::Application.count }.by(1) - expect(response).to have_gitlab_http_status(:bad_request) + expect(response).to have_gitlab_http_status(:created) expect(json_response).to be_a Hash - expect(json_response['message']['confidential'].first).to eq('is not included in the list') + expect(json_response['callback_url']).to eq('http://application.url') + expect(json_response['confidential']).to be true end end diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 1df4d7ea9f6f..602aacb6ced9 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -62,14 +62,14 @@ default_projects_limit: 3, default_project_creation: 2, password_authentication_enabled_for_web: false, - repository_storages: ['custom'], + repository_storages: 'custom', plantuml_enabled: true, plantuml_url: 'http://plantuml.example.com', sourcegraph_enabled: true, sourcegraph_url: 'https://sourcegraph.com', sourcegraph_public_only: false, default_snippet_visibility: 'internal', - restricted_visibility_levels: ['public'], + restricted_visibility_levels: 'public', default_artifacts_expire_in: '2 days', help_page_text: 'custom help text', help_page_hide_commercial_content: true, @@ -94,7 +94,9 @@ issues_create_limit: 300, raw_blob_request_limit: 300, spam_check_endpoint_enabled: true, - spam_check_endpoint_url: 'https://example.com/spam_check' + spam_check_endpoint_url: 'https://example.com/spam_check', + disabled_oauth_sign_in_sources: 'unknown', + import_sources: 'github,bitbucket' } expect(response).to have_gitlab_http_status(:ok) @@ -135,6 +137,8 @@ expect(json_response['raw_blob_request_limit']).to eq(300) expect(json_response['spam_check_endpoint_enabled']).to be_truthy expect(json_response['spam_check_endpoint_url']).to eq('https://example.com/spam_check') + expect(json_response['disabled_oauth_sign_in_sources']).to eq([]) + expect(json_response['import_sources']).to match_array(%w(github bitbucket)) end end diff --git a/spec/rubocop/cop/api/grape_api_instance_spec.rb b/spec/rubocop/cop/api/grape_api_instance_spec.rb new file mode 100644 index 000000000000..74f175cb7071 --- /dev/null +++ b/spec/rubocop/cop/api/grape_api_instance_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rubocop' +require_relative '../../../../rubocop/cop/api/grape_api_instance' + +RSpec.describe RuboCop::Cop::API::GrapeAPIInstance do + include CopHelper + + subject(:cop) { described_class.new } + + it 'adds an offense when inheriting from Grape::API' do + inspect_source(<<~CODE) + class SomeAPI < Grape::API + end + CODE + + expect(cop.offenses.size).to eq(1) + end + + it 'does not add an offense when inheriting from Grape::API::Instance' do + inspect_source(<<~CODE) + class SomeAPI < Grape::API::Instance + end + CODE + + expect(cop.offenses.size).to be_zero + end +end diff --git a/spec/rubocop/cop/api/grape_array_missing_coerce_spec.rb b/spec/rubocop/cop/api/grape_array_missing_coerce_spec.rb new file mode 100644 index 000000000000..c7bb82553982 --- /dev/null +++ b/spec/rubocop/cop/api/grape_array_missing_coerce_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rubocop' +require_relative '../../../../rubocop/cop/api/grape_array_missing_coerce' + +RSpec.describe RuboCop::Cop::API::GrapeArrayMissingCoerce do + include CopHelper + + subject(:cop) { described_class.new } + + it 'adds an offense with a required parameter' do + inspect_source(<<~CODE) + class SomeAPI < Grape::API::Instance + params do + requires :values, type: Array[String] + end + end + CODE + + expect(cop.offenses.size).to eq(1) + end + + it 'adds an offense with an optional parameter' do + inspect_source(<<~CODE) + class SomeAPI < Grape::API::Instance + params do + optional :values, type: Array[String] + end + end + CODE + + expect(cop.offenses.size).to eq(1) + end + + it 'does not add an offense' do + inspect_source(<<~CODE) + class SomeAPI < Grape::API::Instance + params do + requires :values, type: Array[String], coerce_with: ->(val) { val.split(',').map(&:strip) } + requires :milestone, type: String, desc: 'Milestone title' + optional :assignee_id, types: [Integer, String], integer_none_any: true, + desc: 'Return issues which are assigned to the user with the given ID' + end + end + CODE + + expect(cop.offenses.size).to be_zero + end + + it 'does not add an offense for unrelated classes' do + inspect_source(<<~CODE) + class SomeClass + params do + requires :values, type: Array[String] + end + end + CODE + + expect(cop.offenses.size).to be_zero + end +end diff --git a/spec/rubocop/cop/code_reuse/worker_spec.rb b/spec/rubocop/cop/code_reuse/worker_spec.rb index bd1246ceb07d..1f502e554c42 100644 --- a/spec/rubocop/cop/code_reuse/worker_spec.rb +++ b/spec/rubocop/cop/code_reuse/worker_spec.rb @@ -31,7 +31,7 @@ def index .and_return(true) expect_offense(<<~SOURCE) - class Foo < Grape::API + class Foo < Grape::API::Instance resource :projects do get '/' do FooWorker.perform_async