GraphQL/ObjectDescription Reports False Positives

[jblacker]

This issue is related to #42, but it doesn't seem that issue was properly resolved. As mentioned in that issue this cop triggers for any class in the graphql folder, but this should not be how it functions. Due to this it also triggers on implementations of GraphQL::Analysis::AST::* classes as well as GraphQL::Schema::Validator implementations. However, neither of these classes implement the description method that RuboCop is saying to add here. Sure it's easy to add an Excludes to the RuboCop configuration, but this seems like a bug that should be resolved.

[DmitryTsepelev]

Hey @jblacker! Yeah, we can add it to the exclude if we know the exact file. I guess files you mentioned can be literally anywhere, so it makes sense to just check for the base class.

So my proposal is:

• add IgnoredBaseClasses to the rubocop config;
• change cop to skip files that match the condition above

[jblacker]

@DmitryTsepelev Originally I was thinking the same, but I did a deep dive into some of the base classes of the graphql-ruby library and think instead we should update that cop to only react based on the mixin that's used to add the description method. If the cop only triggers for classes that include this mixin then we'll make sure that the cop never issues false positives and non-breaking changes in the library that add new classes using this mixin or adds that mixin to existing classes then it will continue to work correctly.

Whereas if we go down the route using exclusion rather than inclusion then we run the risk of this cop being more fragile and potentially breaking upon "non-breaking changes" to the main repo.

This module that provides the description method is named BaseDSLMethods and is located in the "namespace" GraphQL::Schema::Member. Here's a link to the file in the base branch of the graphql-ruby repo: https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/schema/member/base_dsl_methods.rb

We can then update the cop to only be applied to classes that include this mixin and it should be far more accurate moving forward.

[DmitryTsepelev]

The only problem is that we do not have a runtime in cops, it's just an AST, so we won't be able to check if BaseDSLMethods is included. We can find include BaseDSLMethods or inheritance, but won't be able to detect a case when someone inherits from a custom class that is actully including BaseDSLMethods

[jblacker]

Ah, I didn't realize that Rubocop only works on AST (I'm relatively new to the Ruby world). I'm guessing it's not really feasible to read all of the extension / include / extend and recurse through the files/gems to determine if it's included or not.

[DmitryTsepelev]

Yeah, we can only rely on conventions (i.e., if we make agreement to include everything explicitly or inherit from a specific class)

[severin]

I also see this issue happening in our code base, actually it's even worse:

• We have quite some custom analyzers, all of them get a false positive offense report
• We often use inner classes for custom error classes, all of them get a false positive offense report

Example:

class Mutations::ApplyPromotionToCurrentOrder < GraphQL::Schema::Resolver
  description '...'

  class OrderNotFound < Errors::PreconditionFailed; end # rubocop:todo GraphQL/ObjectDescription
  class PaymentAlreadyExists < Errors::PreconditionFailed; end # rubocop:todo GraphQL/ObjectDescription
  
  ...
end