GraphQL/ObjectDescription is flagging some things that it shouldn't

[andrewmarkle]

Hey there!

Thanks so much for publishing this gem! We've been using it in our production app in the last while and it's been great! One issue that we found though is with the GraphQL/ObjectDescription rule. This seems to flag any class within the graphql folder—sometimes erroneously.

For example:

 class OrderStatusSync < Mutations::BaseMutation
      graphql_name "PatientOrderStatusSync"
      description "Fetch an updated tracking status for the given order"

      class TrackingInfoNotAvailable < StandardError; end
end

In this case the TrackingInfoNotAvailable class gets flagged as violating the rule and it needs a description (which of course, StandardError does not provide).

Just wondering what your thoughts are. Would a simple return unless klass.respond_to?(:description) be enough here? Happy to open a PR if you think that's a good idea. Thanks!

[DmitryTsepelev]

Hi @andrewmarkle! I feel like return unless klass.respond_to?(:description) won't work here since we're working with AST and do not have the runtime here. I wonder what's the best way to handle that, there are options:

1. do nothing and make developers ignore such cases manually (annoying)
2. check classes on the top level (will give us false negatives)
3. check the if the base class type/mutation/etc (error prone cause it's hard to understand if the class we inherit from is inheriting from GQL class)

Thoughts?

[andrewmarkle]

That's a super good point. I completely forgot that we're just working with the AST. .respond_to? will definitely not work.

Hrmmm. Not a lot of great options here. #2 could potentially work. Curious why you think this will give us false negatives? I think in general the top level class should be the only one we care about.

I looked through our (very large) rails app and it seems like, for the most part, the only times we are adding other classes are:

• class methods (which the library already handles well)

class << self
  some_stuff
end

• inheriting from StandardError

class ErrorClass < StandardError; end

• occasional new class (say in Pundit or something—not done in GQL)

class ApplicationPolicy < ApplicationPolicy
  class Scope
  end
end

If another class gets defined afterwards then I feel like it's probably safe to ignore. But I'm probably not thinking of something!

Other random ideas could be to:

• only check for a description once per file
• Ignore classes that are inherited from known false positives (StandardError, Exception, etc.) (Although the challenge with this list is it will never be perfect or fit all use-cases). However I do think the pattern of creating an error class on the fly is a fairly common practice.

[DmitryTsepelev]

Curious why you think this will give us false negatives?

Often times I put input objects right to the mutation class (cause I'm not going to reuse them anywhere else) and in case of option 2 rubocop won't yell on me for the missing description in the nested class.

class SomeMutation < BaseMutation
  # rubocop warning
  class SomeMutationInput < BaseInput
    # no rubocop warning
  end
end

Ignore classes that are inherited from known false positives (StandardError, Exception, etc.)

Or maybe check classes that are inheriting from base classes generated by graphql-ruby assuming that most of times people do not add another level of hierarchy. Thoughts?

[TomasBarry]

Why not check the ancestors of the class? If it contains GraphQL::Schema::Object or something else relevant?

[DmitryTsepelev]

We cannot access ancestors because we work with AST, not with the instance of Class itself 🙂 Rubocop does not even require any dependencies of the code it inspects

[ojab]

Also

app/controllers/graphql/internal_controller.rb:3:9: C: GraphQL/ObjectDescription: Missing type description
  class InternalController < BaseController
        ^^^^^^^^^^^^^^^^^^

[DmitryTsepelev]

@ojab That's because we turn checks on for everything inside any graphql folder. I guess in your case it will be enough to just exclude conrtollers/graphql/**/*