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

Revert "Make scopes available for abstract classes" #1431

Merged

Conversation

paracycle
Copy link
Member

@paracycle paracycle commented Mar 7, 2023

Reverts #1250

I've been thinking about the problem that this change has been causing with AR relations (as pointed out by #1413 that was trying to fix this forward), and I've come to the conclusion that it is better for Tapioca to NOT support this case at all, for a few reasons.

I will explain my reasons based on the following code example (adapted from the one in the original PR #1250)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.datetime :deleted_at, nilable: true
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

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

  scope :deleted, -> { where.not(deleted_at: nil) }

  def self.count_deleted
    deleted.count
  end
end

class Post < ApplicationRecord
  has_many :comments
end

class SubPost < Post
end

class Comment < ApplicationRecord
  belongs_to :post
end
  1. Given the current level of RBI signature generation there is no good way to type the return value of the deleted method. It is supposed to the an ActiveRecord::Relation subtype, but the actual type would depend on what it was called on. For example:
    Post.deleted.class    #=> Post::ActiveRecord_Relation
    SubPost.deleted.class #=> SubPost::ActiveRecord_Relation
    Comment.deleted.class #=> Comment::ActiveRecord_Relation
    and we are not able to currently type this appropriately.
  2. More importantly than the previous point, there is not generic way to type the deleted call being made inside the count_deleted method, since depending on what model type count_deleted is being called on, it could be returning any of the types listed in the previous point. As a result, the deleted call being made inside that method is not type safe anyway, and I would really prefer if that was explicit using a T.unsafe explicitly.
  3. Finally, and probably, most importantly, I think the pattern that is being suggested is unsafe to begin with. You can easily see that by trying to make a Comment.count_deleted call and seeing that it raises an error with the message no such column: comments.deleted_at. Since the abstract base class cannot enforce the definition of a set of columns on child models, it should not try to act like it can ensure that it can make these kinds of queries either. If applications still want to use this pattern, it is better for them to explicitly use T.unsafe to communicate that the method could be unsafe in this respect as well.

So, I think it is best to revert the original PR and disallow abstract base classes from DSL generation for scopes.

@paracycle paracycle merged commit 2c45d4d into main Mar 7, 2023
@paracycle paracycle deleted the revert-1250-include_abstract_classes_in_active_record_scope branch March 7, 2023 21:52
@shopify-shipit shopify-shipit bot temporarily deployed to production March 10, 2023 16:06 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.

2 participants