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

Port BeforeTypeCast to Active Model #44534

Merged

Conversation

jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented Feb 23, 2022

This commit ports ActiveRecord::AttributeMethods::BeforeTypeCast to ActiveModel::BeforeTypeCast and includes it in ActiveModel::Attributes. Thus, classes that include ActiveModel::Attributes will now automatically define methods such as *_before_type_cast, just as Active Record models do.

The ActiveRecord::AttributeMethods::BeforeTypeCast module is kept for backward compatibility, but it now merely includes ActiveModel::BeforeTypeCast.

@jonathanhefner
Copy link
Member Author

jonathanhefner commented Feb 23, 2022

The build failure is due to BeforeTypeCast assuming @attributes has been set (as an AttributeSet). That assumption holds for Active Record, but, in Active Model, ActiveModel::Attributes#initialize is responsible for setting @attributes, and ActiveModel::AttributeMethods may be included independently of ActiveModel::Attributes.

As a solution, we could check if @attributes is nil in BeforeTypeCast, but that feels a little fragile.

Perhaps it would be better to move ActiveModel::AttributeMethods::BeforeTypeCast to ActiveModel::BeforeTypeCast, and then automatically include it via ActiveModel::Attributes instead of ActiveModel::AttributeMethods (i.e. require that both ActiveModel::Attributes and ActiveModel::AttributeMethods are present when using ActiveModel::BeforeTypeCast)?

(Note that we already include ActiveModel::AttributeMethods in ActiveModel::Attributes, so this would just add include ActiveModel::BeforeTypeCast right below that.)

@jonathanhefner
Copy link
Member Author

Another thought: since ActiveRecord::AttributeMethods::BeforeTypeCast is a public module, should we keep it as a placeholder that simply includes Active Model's BeforeTypeCast?

This commit ports `ActiveRecord::AttributeMethods::BeforeTypeCast` to
`ActiveModel::BeforeTypeCast` and includes it in `ActiveModel::Attributes`.
Thus, classes that include `ActiveModel::Attributes` will now
automatically define methods such as `*_before_type_cast`, just as
Active Record models do.

The `ActiveRecord::AttributeMethods::BeforeTypeCast` module is kept for
backward compatibility, but it now merely includes
`ActiveModel::BeforeTypeCast`.

Co-authored-by: Petrik <petrik@deheus.net>
@jonathanhefner jonathanhefner mentioned this pull request Nov 4, 2023
4 tasks
@jonathanhefner jonathanhefner merged commit e400b8a into rails:main Nov 4, 2023
4 checks passed
@rafaelfranca
Copy link
Member

I'm reverting this PR. I see no justification to move this to Active Model anywhere in the PR description. Sure, it is nice to have, but why would you want those methods in a Active Model class. And it is automatically adding to Attributes, it is no opt-in like the other things in Active Model.

@jonathanhefner
Copy link
Member Author

BeforeTypeCast is the only public API for accessing pre-type-cast attribute values. Currently, if a library or application is building on top of Active Model and wants to access those values, it must include ActiveRecord::AttributeMethods::BeforeTypeCast which does not feel correct and is potentially fragile.

One example of using BeforeTypeCast is defining Active Model objects that can be used with form helpers:

def value_before_type_cast
return unless object
method_before_type_cast = @method_name + "_before_type_cast"
if value_came_from_user? && object.respond_to?(method_before_type_cast)
object.public_send(method_before_type_cast)

options["value"] = options.fetch("value") { value_before_type_cast } unless field_type == "file"

Another example of how BeforeTypeCast can be useful is checking whether an attribute was originally specified as nil vs an invalid value that was cast to nil.

@rafaelfranca
Copy link
Member

I get the theoretical problem, but I want to see examples where applications and libraries are needing those features. Active Model doesn't not have the goal of matching all the Active Record API and features. It provides what is useful, not everything.

For example, did you find any form that uses Active Model object that actually need to use the value_before_type_cast?

@jonathanhefner
Copy link
Member Author

I originally submitted this PR because it was an issue I encountered in a gem that I wrote. It's been a long time since I wrote the code, but I think the issue was related to attribute types where the to_s output is different than what the user originally input. For example, with :date attributes. When a form is re-rendered, a text input field will display the to_s value (e.g. "Wed, 08 Nov 2023") rather than the user's original input (e.g. "2023-11-08").

@rafaelfranca
Copy link
Member

Got it. Can we make it work without bringing that entire API? For example, can we just move _before_type_cast and not _for_database and _came_from_user?.

@jonathanhefner
Copy link
Member Author

Form helpers also need _came_from_user?:

if value_came_from_user? && object.respond_to?(method_before_type_cast)
object.public_send(method_before_type_cast)
else
value
end
end
def value_came_from_user?
method_name = "#{@method_name}_came_from_user?"
!object.respond_to?(method_name) || object.public_send(method_name)
end

As for _for_database, I actually did consider leaving it out, but _for_database is really just the serialized value. It is useful whenever you want to serialize / persist model attributes, not just to the database. For example, if you want to build your own MessagePack encoder similar to ours:

def build_entry(record)
[
ActiveSupport::MessagePack::Extensions.dump_class(record.class),
record.attributes_for_database,
record.new_record?
]
end

Since Active Model is the framework that actually implements the _for_database logic (rather than Active Record), I concluded it was appropriate to leave it in.

@rafaelfranca
Copy link
Member

Form helpers also need _came_from_user?

I think my question is more, do they need to? If they do need to, them we should be moving all those methods to the ActiveModel::API even if you don't have ActiveModel::Attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants