From 95ed4d519437859a6466b345a23d07e7cb4be942 Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Fri, 24 Dec 2021 00:21:09 +0300 Subject: [PATCH 01/10] Implement UnusedArgument cop --- config/default.yml | 4 + lib/rubocop/cop/graphql/unused_argument.rb | 129 ++++++++++++++++++ lib/rubocop/cop/graphql_cops.rb | 1 + .../cop/graphql/unused_argument_spec.rb | 105 ++++++++++++++ 4 files changed, 239 insertions(+) create mode 100644 lib/rubocop/cop/graphql/unused_argument.rb create mode 100644 spec/rubocop/cop/graphql/unused_argument_spec.rb diff --git a/config/default.yml b/config/default.yml index 020edb5..6a462f8 100644 --- a/config/default.yml +++ b/config/default.yml @@ -104,4 +104,8 @@ GraphQL/OrderedFields: VersionAdded: '0.80' Description: 'Fields should be alphabetically sorted within groups' +GraphQL/UnusedArgument: + Enabled: true + Description: 'Arguments should either be listed explicitly or **rest should be in the resolve signature' + diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb new file mode 100644 index 0000000..54af11c --- /dev/null +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module GraphQL + # Arguments should either be listed explicitly or **rest should be in the resolve signature. + # + # @example + # # good + # + # class SomeResolver < Resolvers::Base + # argument :arg1, String, required: true + # argument :arg2, String, required: true + # + # def resolve(arg1:, arg2:); end + # end + # + # # good + # + # class SomeResolver < Resolvers::Base + # argument :arg1, String, required: true + # argument :arg2, String, required: true + # + # def resolve(arg1:, **rest); end + # end + # + # # good + # + # class SomeResolver < Resolvers::Base + # type SomeType, null: false + # + # argument :arg1, String, required: true + # argument :arg2, String, required: true + # + # def resolve(args); end + # end + # + # # bad + # + # class SomeResolver < Resolvers::Base + # type SomeType, null: false + # + # argument :arg1, String, required: true + # argument :arg2, String, required: true + # + # def resolve(arg1:); end + # end + # + # # bad + # + # class SomeResolver < Resolvers::Base + # type SomeType, null: false + # + # argument :arg1, String, required: true + # argument :arg2, String, required: true + # + # def resolve; end + # end + # + class UnusedArgument < Base + extend AutoCorrector + + MSG = "Argument%s `%s` should be listed in the resolve signature." + + def on_class(node) + resolve_method_node = find_resolve_method_node(node) + return if resolve_method_node.nil? + return if resolve_method_node.arguments.any?(&:arg_type?) + return if resolve_method_node.arguments.any?(&:kwrestarg_type?) + + declared_args_nodes = argument_declarations(node) + return unless declared_args_nodes.any? + + unresolved_args = find_unresolved_args(resolve_method_node, declared_args_nodes) + if unresolved_args.any? + register_offense(resolve_method_node, unresolved_args) + end + end + + private + + def find_resolve_method_node(node) + resolve_method_nodes = resolve_method_definition(node) + return unless resolve_method_nodes.any? + + resolve_method_nodes.to_a.last + end + + def find_unresolved_args(actual_resolve_method_node, declared_args_nodes) + resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map(&:name).to_set + declared_args = declared_args_nodes.map(&:first_argument).map(&:value) + + declared_args.select { |declared_arg| !resolve_method_kwargs.include?(declared_arg) } + end + + def register_offense(node, unresolved_args) + unresolved_args_source = unresolved_args.map{|v| "#{v}:"}.join(', ') + + message = format( + self.class::MSG, + unresolved_args: unresolved_args_source, + ending: unresolved_args.size == 1 ? '' : 's' + ) + + add_offense(node, message: message) do |corrector| + if node.arguments? + corrector.insert_after(arg_end(node.arguments.last), ", #{unresolved_args_source}") + else + corrector.insert_before(args_begin(node), "(#{unresolved_args_source})") + end + end + end + + def arg_end(node) + node.loc.expression.end + end + + def_node_search :argument_declarations, <<~PATTERN + (send nil? :argument (:sym _) ...) + PATTERN + + def_node_search :resolve_method_definition, <<~PATTERN + (def :resolve + (args ...) ...) + PATTERN + end + end + end +end diff --git a/lib/rubocop/cop/graphql_cops.rb b/lib/rubocop/cop/graphql_cops.rb index c25e522..07c371f 100644 --- a/lib/rubocop/cop/graphql_cops.rb +++ b/lib/rubocop/cop/graphql_cops.rb @@ -16,3 +16,4 @@ require_relative "graphql/object_description" require_relative "graphql/ordered_arguments" require_relative "graphql/ordered_fields" +require_relative "graphql/unused_argument" diff --git a/spec/rubocop/cop/graphql/unused_argument_spec.rb b/spec/rubocop/cop/graphql/unused_argument_spec.rb new file mode 100644 index 0000000..f98d06b --- /dev/null +++ b/spec/rubocop/cop/graphql/unused_argument_spec.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::GraphQL::UnusedArgument do + subject(:cop) { described_class.new(config) } + + let(:config) { RuboCop::Config.new } + + context "when all args are used" do + it "not registers an offense" do + expect_no_offenses(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:, arg2:); end + end + RUBY + end + end + + context "when first arg and splats arg are used" do + it "not registers an offense" do + expect_no_offenses(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:, **rest); end + end + RUBY + end + end + + context "when hash arg is used" do + it "not registers an offense" do + expect_no_offenses(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(args); end + end + RUBY + end + end + + context "when splats arg is used" do + it "not registers an offense" do + expect_no_offenses(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(**args); end + end + RUBY + end + end + + context "when not all args are used" do + it "registers an offense" do + expect_offense(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:); end + ^^^^^^^^^^^^^^^^^^^^^^^ Argument `arg2:` should be listed in the resolve signature. + end + RUBY + + expect_correction(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:, arg2:); end + end + RUBY + end + end + + context "when args are not used at all" do + it "registers an offense" do + expect_offense(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve; end + ^^^^^^^^^^^^^^^^ Arguments `arg1:, arg2:` should be listed in the resolve signature. + end + RUBY + + expect_correction(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:, arg2:); end + end + RUBY + end + end +end From 60b96a6efc74f5c79998d108ff5eaa0f126a1a9c Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Fri, 24 Dec 2021 12:10:35 +0300 Subject: [PATCH 02/10] Fix rubocop compatibility --- lib/rubocop/cop/graphql/unused_argument.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 54af11c..17bd374 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -68,10 +68,14 @@ def on_class(node) return if resolve_method_node.arguments.any?(&:arg_type?) return if resolve_method_node.arguments.any?(&:kwrestarg_type?) - declared_args_nodes = argument_declarations(node) - return unless declared_args_nodes.any? + declared_arg_nodes = argument_declarations(node) + return unless declared_arg_nodes.any? - unresolved_args = find_unresolved_args(resolve_method_node, declared_args_nodes) + declared_args = declared_arg_nodes.map do |declared_arg_node| + RuboCop::GraphQL::Argument.new(declared_arg_node) + end + + unresolved_args = find_unresolved_args(resolve_method_node, declared_args) if unresolved_args.any? register_offense(resolve_method_node, unresolved_args) end @@ -86,11 +90,9 @@ def find_resolve_method_node(node) resolve_method_nodes.to_a.last end - def find_unresolved_args(actual_resolve_method_node, declared_args_nodes) + def find_unresolved_args(actual_resolve_method_node, declared_args) resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map(&:name).to_set - declared_args = declared_args_nodes.map(&:first_argument).map(&:value) - - declared_args.select { |declared_arg| !resolve_method_kwargs.include?(declared_arg) } + declared_args.map(&:name).select { |declared_arg| !resolve_method_kwargs.include?(declared_arg) } end def register_offense(node, unresolved_args) From ff8ee85f433ef7a182acceabaa1859cb59ec4b1d Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Fri, 24 Dec 2021 12:11:13 +0300 Subject: [PATCH 03/10] Fix twice declared agrs case for UnusedArgument cop --- lib/rubocop/cop/graphql/unused_argument.rb | 2 +- .../cop/graphql/unused_argument_spec.rb | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 17bd374..750c109 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -92,7 +92,7 @@ def find_resolve_method_node(node) def find_unresolved_args(actual_resolve_method_node, declared_args) resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map(&:name).to_set - declared_args.map(&:name).select { |declared_arg| !resolve_method_kwargs.include?(declared_arg) } + declared_args.map(&:name).uniq.select { |declared_arg| !resolve_method_kwargs.include?(declared_arg) } end def register_offense(node, unresolved_args) diff --git a/spec/rubocop/cop/graphql/unused_argument_spec.rb b/spec/rubocop/cop/graphql/unused_argument_spec.rb index f98d06b..b7930b7 100644 --- a/spec/rubocop/cop/graphql/unused_argument_spec.rb +++ b/spec/rubocop/cop/graphql/unused_argument_spec.rb @@ -80,6 +80,31 @@ def resolve(arg1:, arg2:); end end end + context "when not all args are used, but some args declared twice" do + it "registers an offense" do + expect_offense(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:); end + ^^^^^^^^^^^^^^^^^^^^^^^ Argument `arg2:` should be listed in the resolve signature. + end + RUBY + + expect_correction(<<~RUBY) + class SomeResolver < Resolvers::Base + argument :arg1, String, required: true + argument :arg2, String, required: true + argument :arg2, String, required: true + + def resolve(arg1:, arg2:); end + end + RUBY + end + end + context "when args are not used at all" do it "registers an offense" do expect_offense(<<~RUBY) From f68b0de38e6ef88772de8530e59dd148a77d9204 Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 00:13:57 +0300 Subject: [PATCH 04/10] Fix twice arguments loop Co-authored-by: Dmitry Tsepelev --- lib/rubocop/cop/graphql/unused_argument.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 750c109..667af90 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -64,9 +64,8 @@ class UnusedArgument < Base def on_class(node) resolve_method_node = find_resolve_method_node(node) - return if resolve_method_node.nil? - return if resolve_method_node.arguments.any?(&:arg_type?) - return if resolve_method_node.arguments.any?(&:kwrestarg_type?) + return if resolve_method_node.nil? || + resolve_method_node.arguments.any? { |arg| arg.arg_type? || arg.kwrestarg_type? } declared_arg_nodes = argument_declarations(node) return unless declared_arg_nodes.any? From ad4684b739acf613b94c38067718708de1341877 Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 00:14:32 +0300 Subject: [PATCH 05/10] Remove reverse logic Co-authored-by: Dmitry Tsepelev --- lib/rubocop/cop/graphql/unused_argument.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 667af90..9c7407c 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -68,7 +68,7 @@ def on_class(node) resolve_method_node.arguments.any? { |arg| arg.arg_type? || arg.kwrestarg_type? } declared_arg_nodes = argument_declarations(node) - return unless declared_arg_nodes.any? + return if declared_arg_nodes.empty? declared_args = declared_arg_nodes.map do |declared_arg_node| RuboCop::GraphQL::Argument.new(declared_arg_node) From 3aa8a26a4ca73c17ce808d84b90372294fbb7f44 Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 00:14:39 +0300 Subject: [PATCH 06/10] Remove reverse logic Co-authored-by: Dmitry Tsepelev --- lib/rubocop/cop/graphql/unused_argument.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 9c7407c..d964618 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -84,7 +84,7 @@ def on_class(node) def find_resolve_method_node(node) resolve_method_nodes = resolve_method_definition(node) - return unless resolve_method_nodes.any? + return if resolve_method_nodes.empty? resolve_method_nodes.to_a.last end From 42e8816ba88234a1d069b2cf5a92b3ea8903b1a0 Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 00:14:50 +0300 Subject: [PATCH 07/10] Remove reverse logic Co-authored-by: Dmitry Tsepelev --- lib/rubocop/cop/graphql/unused_argument.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index d964618..f0a447f 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -91,7 +91,7 @@ def find_resolve_method_node(node) def find_unresolved_args(actual_resolve_method_node, declared_args) resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map(&:name).to_set - declared_args.map(&:name).uniq.select { |declared_arg| !resolve_method_kwargs.include?(declared_arg) } + declared_args.map(&:name).uniq.reject { |declared_arg| resolve_method_kwargs.include?(declared_arg) } end def register_offense(node, unresolved_args) From 47cc678a773445da7c48ddf81e3af46a52c242fd Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 00:16:35 +0300 Subject: [PATCH 08/10] Fix backward rubocop gem compatibility --- lib/rubocop/cop/graphql/unused_argument.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index f0a447f..1565e39 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -90,7 +90,7 @@ def find_resolve_method_node(node) end def find_unresolved_args(actual_resolve_method_node, declared_args) - resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map(&:name).to_set + resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map {|node| node.node_parts[0] }.to_set declared_args.map(&:name).uniq.reject { |declared_arg| resolve_method_kwargs.include?(declared_arg) } end @@ -107,11 +107,15 @@ def register_offense(node, unresolved_args) if node.arguments? corrector.insert_after(arg_end(node.arguments.last), ", #{unresolved_args_source}") else - corrector.insert_before(args_begin(node), "(#{unresolved_args_source})") + corrector.insert_after(method_name(node), "(#{unresolved_args_source})") end end end + def method_name(node) + node.loc.keyword.end.resize(node.method_name.to_s.size + 1) + end + def arg_end(node) node.loc.expression.end end From 09f456102a96053efc0db8784484ec68099a5dfa Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 17:56:53 +0300 Subject: [PATCH 09/10] Fix backward rubocop compatibility --- lib/rubocop/cop/graphql/unused_argument.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 1565e39..205cfce 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -68,7 +68,7 @@ def on_class(node) resolve_method_node.arguments.any? { |arg| arg.arg_type? || arg.kwrestarg_type? } declared_arg_nodes = argument_declarations(node) - return if declared_arg_nodes.empty? + return unless declared_arg_nodes.any? declared_args = declared_arg_nodes.map do |declared_arg_node| RuboCop::GraphQL::Argument.new(declared_arg_node) @@ -84,9 +84,7 @@ def on_class(node) def find_resolve_method_node(node) resolve_method_nodes = resolve_method_definition(node) - return if resolve_method_nodes.empty? - - resolve_method_nodes.to_a.last + resolve_method_nodes.to_a.last if resolve_method_nodes.any? end def find_unresolved_args(actual_resolve_method_node, declared_args) From 73324ffb5eafa2b3ac14bbac43159af752c91795 Mon Sep 17 00:00:00 2001 From: Alexandr Borisov Date: Wed, 29 Dec 2021 18:02:40 +0300 Subject: [PATCH 10/10] Fix Ruby code style issues --- lib/rubocop/cop/graphql/unused_argument.rb | 29 +++++++++++----------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/rubocop/cop/graphql/unused_argument.rb b/lib/rubocop/cop/graphql/unused_argument.rb index 205cfce..6473089 100644 --- a/lib/rubocop/cop/graphql/unused_argument.rb +++ b/lib/rubocop/cop/graphql/unused_argument.rb @@ -65,19 +65,15 @@ class UnusedArgument < Base def on_class(node) resolve_method_node = find_resolve_method_node(node) return if resolve_method_node.nil? || - resolve_method_node.arguments.any? { |arg| arg.arg_type? || arg.kwrestarg_type? } + resolve_method_node.arguments.any? do |arg| + arg.arg_type? || arg.kwrestarg_type? + end declared_arg_nodes = argument_declarations(node) return unless declared_arg_nodes.any? - declared_args = declared_arg_nodes.map do |declared_arg_node| - RuboCop::GraphQL::Argument.new(declared_arg_node) - end - - unresolved_args = find_unresolved_args(resolve_method_node, declared_args) - if unresolved_args.any? - register_offense(resolve_method_node, unresolved_args) - end + unresolved_args = find_unresolved_args(resolve_method_node, declared_arg_nodes) + register_offense(resolve_method_node, unresolved_args) if unresolved_args.any? end private @@ -87,18 +83,23 @@ def find_resolve_method_node(node) resolve_method_nodes.to_a.last if resolve_method_nodes.any? end - def find_unresolved_args(actual_resolve_method_node, declared_args) - resolve_method_kwargs = actual_resolve_method_node.arguments.select(&:kwarg_type?).map {|node| node.node_parts[0] }.to_set - declared_args.map(&:name).uniq.reject { |declared_arg| resolve_method_kwargs.include?(declared_arg) } + def find_unresolved_args(method_node, declared_arg_nodes) + resolve_method_kwargs = method_node.arguments.select(&:kwarg_type?).map do |node| + node.node_parts[0] + end.to_set + declared_args = declared_arg_nodes.map { |node| RuboCop::GraphQL::Argument.new(node) } + declared_args.map(&:name).uniq.reject do |declared_arg| + resolve_method_kwargs.include?(declared_arg) + end end def register_offense(node, unresolved_args) - unresolved_args_source = unresolved_args.map{|v| "#{v}:"}.join(', ') + unresolved_args_source = unresolved_args.map { |v| "#{v}:" }.join(", ") message = format( self.class::MSG, unresolved_args: unresolved_args_source, - ending: unresolved_args.size == 1 ? '' : 's' + ending: unresolved_args.size == 1 ? "" : "s" ) add_offense(node, message: message) do |corrector|