tapioca icon indicating copy to clipboard operation
tapioca copied to clipboard

RBI: getter methods on belongs_to associations have returns(T.nilable(...)) as sig but belongs_to reflection is not optional

Open rubyleth opened this issue 1 year ago • 2 comments

I noticed the returns signature of any belongs_to association on a model is always nilable in the RBI file, even if the belongs_to is not optional. Rails will raise an error if a nil value is present for such an association. I don't find it optimal to add T.must everywhere to change this in application code.

I changed Tapioca::Dsl::Compilers::ActiveRecordAssociations#populate_single_assoc_getter_setter and it seems to work:

association_type = if !reflection.options[:optional] && reflection.is_a?(ActiveRecord::Reflection::BelongsToReflection)
            association_class
          else
            as_nilable_type(association_class)
          end

What do you think ?

rubyleth avatar Sep 06 '24 16:09 rubyleth

I have a PR open (and approved) to address this issue: https://github.com/Shopify/tapioca/pull/1993

stathis-alexander avatar Sep 12 '24 09:09 stathis-alexander

RE: https://github.com/Shopify/tapioca/pull/1996#pullrequestreview-2300921400

@paracycle I understand wanting to rely solely on the database column nullability and not ActiveRecord validators (especially given that validators can have ifs on them). For belongs_to associations, I think it would make sense to mark the association non-nilable, if the foreign key column has a NOT NULL constraint.

class Product
  belongs_to :shop
end

So, given this. If shop_id has a NOT NULL constraint, then shop's type is Shop, but if to does not have a NOT NULL constraint, then the type is T.nilable(Shop) (even if ActiveRecord won't actually let you save that, due to the lack of optional: true).

One slightly complicated part of this is a polymorphic belongs_to, where you have the shop_type and shop_id column to consider. Perhaps in that situation, then you just check if both columns have a NOT NULL constraint. If they're both NOT NULL, then you can call it Shop, but if at least 1 of the 2 is nullable, then T.nilable(Shop) it is.

natematykiewicz avatar Mar 21 '25 20:03 natematykiewicz