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`: ruby-grape/grape#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.
terrchen · Jun 29, 2020 · 3a0c6dd · 3a0c6dd
1 parent d997e1e
commit 3a0c6dd
Show file tree

Hide file tree

Showing 185 changed files with 695 additions and 341 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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