Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ActiveRecordScope Compiler to not duplicate relation methods of superclass #1453

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

codingarchitect-wq
Copy link
Contributor

@codingarchitect-wq codingarchitect-wq commented Mar 25, 2023

Motivation

We encountered a strange situation where CI was reporting that a rbi file has changed and needs to be regenerated, but running bin/tapioca dsl locally on the dev environment would not generate any changes.

It seems on the CI build, a method in one of the rbi files is generated twice while on dev environment not, leading to CI failing all the time we regenerate the rbis. It is unclear why this is happening, the same version of tapioca is used in both cases and there is no obvious reason why it is happening.

As a workaround we had to undo the changes to that rbi file on each branch, leading to confusion within the team.

After investigation I found that the ActiveRecordScope Compiler gathers scope methods until it hits "ActiveRecord::Base" and concatenates the methods in an array. The problem is it does not remove duplicates, if a method is defined twice along the class hierarchy chain.

Given the class hierarchy

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  default_scope { max_execution_time(100) }
end

class ConfigurationRecord < ApplicationRecord
  self.abstract_class = true

  scope :post_scope, -> { where.not(kind: 'private') }

  default_scope { max_execution_time(200) }
end

class Post < ConfigurationRecord
  scope :post_scope, -> { where.not(kind: 'private') }
end

Before this PR:

class Post
  extend GeneratedRelationMethods

  module GeneratedAssociationRelationMethods
    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
    def post_scope(*args, &blk); end

    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
    def post_scope(*args, &blk); end
  end

  module GeneratedRelationMethods
    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
    def post_scope(*args, &blk); end

    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
    def post_scope(*args, &blk); end
  end
end

After this PR:

class Post
  extend GeneratedRelationMethods

  module GeneratedAssociationRelationMethods
    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateAssociationRelation) }
    def post_scope(*args, &blk); end
  end

  module GeneratedRelationMethods
    sig { params(args: T.untyped, blk: T.untyped).returns(PrivateRelation) }
    def post_scope(*args, &blk); end
  end
end

Implementation

Removed duplicates from ActiveRecordScope Compiler while gathering scope methods of the class hierarchy chain.

Tests

Added a test that asserts that no methods defined twice in the class hierarchy are duplicated in the rbi file.

@codingarchitect-wq codingarchitect-wq requested a review from a team as a code owner March 25, 2023 12:35
Copy link

@samuelgiles samuelgiles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 I wonder if there are other areas with similar behaviour

Copy link

@joelpinheiro joelpinheiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@codingarchitect-wq is there anything holding this to be merged?

@codingarchitect-wq
Copy link
Contributor Author

@KaanOzkan merging is blocked for me, should I just /shipit? I noticed previous PRs seem to have been manually merged by someone from the maintainers team.

@Shinomix Shinomix merged commit eb4ab4d into main Apr 12, 2023
@Shinomix Shinomix deleted the fix-active-record-scope-compiler-duplicate-methods branch April 12, 2023 09:53
@shopify-shipit shopify-shipit bot temporarily deployed to production April 12, 2023 17:19 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants