In ActiveRecordColumns persisted mode, remove T.nilable from reflected sigs #1937
Conversation
| # It's possible that when ActiveModel::Type::Value is used, the signature being reflected on in | ||
| # ActiveModelTypeHelper.type_for(type_value) may say the type can be nilable. However, if the type is | ||
| # persisted and the column is not nullable, we can assume it's not nilable. | ||
| [as_non_nilable_type(getter_type), as_non_nilable_type(setter_type)] |
There was a problem hiding this comment.
I'm wary of adding this overhead to every type by default to cover for an edge case. I'd like to understand why changing the signature of deserialize and other methods is not an acceptable solution.
There was a problem hiding this comment.
Sure thing. So we use custom types pretty widely both for id columns and as nilable foreign keys. I'm not sure if there's a way around it for the use case where the type can be used both for nullable and non-nullable columns:
sig { params(value: T.nilable(Numeric)).returns(T.nilable(::CustomType))}
def deserialize(value)
CustomType.new(value) if value
endI could look at moving the call into just the branch of type_for_activerecord_value that covers this case. I think it would just mean needing to pass in @column_type_option and column into that method.
There was a problem hiding this comment.
(I guess just column since the other is an instance variable)
There was a problem hiding this comment.
I pushed a commit showing what that might look like
There was a problem hiding this comment.
Sorry this took so long we were away. This makes sense to me but I'd like another set of eyes before squashing the commits and merging.
| sig { params(column_type: T.untyped).returns(String) } | ||
| def type_for_activerecord_value(column_type) | ||
| sig { params(column_type: T.untyped, null: T::Boolean).returns(String) } | ||
| def type_for_activerecord_value(column_type, null:) |
There was a problem hiding this comment.
| def type_for_activerecord_value(column_type, null:) | |
| def type_for_activerecord_value(column_type, column_nullability:) |
Something like this will be better.
| # It's possible that when ActiveModel::Type::Value is used, the signature being reflected on in | ||
| # ActiveModelTypeHelper.type_for(type_value) may say the type can be nilable. However, if the type is | ||
| # persisted and the column is not nullable, we can assume it's not nilable. | ||
| return as_non_nilable_type(base_type) if @column_type_option.persisted? && !null |
There was a problem hiding this comment.
Since you've also contributed the EncryptedAttributeType case in line 136, this could affect that too right? So we should return non nilable types if possible in that case too.
There was a problem hiding this comment.
Ah yes, thanks for pointing that out 👍
alex-tan
force-pushed
the
no-nilable-for-persisted
branch
from
23e557f
to
945d06c
Compare
Motivation
We use types inheriting from
ActiveModel::Type::Valuewhere thedeserializesig says it returnsT.nilable([custom type]). However when using the new dslpersistedmode, we can assume the type won't be nilable.Implementation
Apply
as_non_nilable_typetogetter_typeandsetter_typeonly in persisted mode and when the column is not nullable.Tests
I've added tests.