Commit 7e8fba11 authored by Stan Hu's avatar Stan Hu

Add 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.
parent f31bac1d
...@@ -277,6 +277,18 @@ Gitlab/Union: ...@@ -277,6 +277,18 @@ Gitlab/Union:
- 'spec/**/*' - 'spec/**/*'
- 'ee/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: Cop/SidekiqOptionsQueue:
Enabled: true Enabled: true
Exclude: Exclude:
......
# 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
# 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
# frozen_string_literal: true
require 'fast_spec_helper'
require 'rubocop'
require_relative '../../../support/helpers/expect_offense'
require_relative '../../../../rubocop/cop/api/grape_api_instance'
describe RuboCop::Cop::API::GrapeAPIInstance do
include CopHelper
include ExpectOffense
subject(:cop) { described_class.new }
it 'adds an offense when inheriting from Grape::API' do
inspect_source(<<~CODE.strip_indent)
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.strip_indent)
class SomeAPI < Grape::API::Instance
end
CODE
expect(cop.offenses.size).to be_zero
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require 'rubocop'
require_relative '../../../support/helpers/expect_offense'
require_relative '../../../../rubocop/cop/api/grape_array_missing_coerce'
describe RuboCop::Cop::API::GrapeArrayMissingCoerce do
include CopHelper
include ExpectOffense
subject(:cop) { described_class.new }
it 'adds an offense with a required parameter' do
inspect_source(<<~CODE.strip_indent)
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.strip_indent)
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.strip_indent)
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.strip_indent)
class SomeClass
params do
requires :values, type: Array[String]
end
end
CODE
expect(cop.offenses.size).to be_zero
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment