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

Integrate with Active Model Attributes #410

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanpdoyle
Copy link

The schema { ... } interface pre-dates the Active Model Attributes API (defined as early as v5.2.0), but clearly draws inspiration from Active Record's Database Schema and Attribute casting (which was extracted into ActiveModel::Attributes).

However, the type information captured in schema { ... } blocks or assigned as Hash arguments to schema= is purely inert metadata.

Proposal

This commit aims to integrate with ActiveModel::Model and ActiveModel::Attributes. Through the introduction of both modules, subclasses of ActiveResource::Base can benefit from type casting attributes and constructing instances with default values.

This commit makes minimally incremental changes, prioritizing backwards compatibility. The reliance on #respond_to_missing? and #method_missing is left largely unchanged. Similarly, the Schema interface continues to provide metadata about its attributes through the Schema#attr method (instead of reading from
ActiveModel::Attributes#attribute_names or
ActiveModel::Attributes.attribute_types).

API Changes

To cast values to their specified types, declare the Schema with the :cast_values set to true.

class Person < ActiveResource::Base
  schema cast_values: true do
    integer 'age'
  end
end

p = Person.new
p.age = "18"
p.age # => 18

To configure inheriting resources to cast values, set the cast_values class attribute:

class ApplicationResource < ActiveResource::Base
  self.cast_values = true
end

class Person < ApplicationResource
  schema do
    integer 'age'
  end
end

p = Person.new
p.age = "18"
p.age # => 18

To set all resources application-wide to cast values, set config.active_resource.cast_values:

  # config/application.rb
  config.active_resource.cast_values = true

Comment on lines +475 to +482
# TODO uses private APIs -- mostly for test harness resetting
undefine_attribute_methods
if respond_to?(:reset_default_attributes)
reset_default_attributes
elsif respond_to?(:attribute_types=) && respond_to?(:_default_attributes=)
self.attribute_types = Hash.new(ActiveModel::Type.default_value)
self._default_attributes = ActiveModel::AttributeSet.new({})
end
Copy link
Author

Choose a reason for hiding this comment

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

This block is an attempt to reverse any attribute-defined methods.

The current attempt is messy and relies on private APIs. The conditionals aim to support the range of Active Model versions.

I hope there's a better way to reverse these declarations -- ideally in a test harness-only way, since the main motivation is to support the setup and teardown blocks in https://github.com/rails/activeresource/blob/main/test/cases/base/schema_test.rb.

Does Active Resource explicitly support calling .schema { ... } or .schema= multiple times for the same ActiveResource::Base subclass?

Copy link
Author

Choose a reason for hiding this comment

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

CI tests are flaky across the matrix because teardown isn't entirely working and state is leaking between runs.

Comment on lines +1287 to +1289
# TODO could @attributes.clone suffice?
cloned = ActiveModel::AttributeSet.new({})
@attributes.each_value { |v| cloned[v.name] = v.clone if !(v.name == self.class.primary_key.to_s || v.value.is_a?(ActiveResource::Base)) }
Copy link
Author

Choose a reason for hiding this comment

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

What is the history behind excluding primary keys and ActiveResource::Base instances?

Would calling ActiveModel::AttributeSet#clone (through @attributes.clone) be sufficient?

If not, what is another way to efficiently clone the attributes in a way that meets the requirements?

Comment on lines +1333 to +1336
# TODO remove: indifferent_access causes duplicates values and loss of reference
def attributes
super.with_indifferent_access
end
Copy link
Author

Choose a reason for hiding this comment

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

There are existing tests asserting that the Hash returned by #attributes supports indifferent access. The value returned by ActiveModel::Attributes#attributes only supports String keys.

Transforming the value returned from super with a call to Hash#with_indifferent_access preserves the behavior covered by the tests, but deeply transforms the Hash, which has unintended side effects for attributes that are Hash instances or Hash-like.

Is there a way to limit the indifferent access to be top-level only, leaving nested values unaffected?

Comment on lines +1338 to +1344
# TODO remove: ActiveModel::AttributeSet is private API
def attributes=(value)
case value
when ActiveModel::AttributeSet then @attributes = value
else super
end
end
Copy link
Author

Choose a reason for hiding this comment

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

The ActiveModel::Attributes#attributes= interface relies on ActiveModel::AttributeAssignment#attributes= for mass assignment.

This method override is for the sake of the ActiveResource::Base#dup implementation:

def dup
self.class.new.tap do |resource|
resource.attributes = @attributes
resource.prefix_options = @prefix_options
end
end

If #dup were implemented differently, the case statement wouldn't be necessary and the override could be removed to completely rely on the behavior provided by its super.

Comment on lines 1761 to 1768
def attribute=(name, value)
super
rescue ActiveModel::MissingAttributeError
# TODO uses private API to assign to unknown attributes without a schema
@attributes[name] = ActiveModel::Attribute.from_user(name, value, ActiveModel::Type.default_value)

retry
end
Copy link
Author

Choose a reason for hiding this comment

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

Since ActiveResource::Base supports writing to and reading from attributes that are not part of any Schema declaration, this change adds them to the Attribute Set as they are assigned.

As an alternative to this approach (which relies on private APIs), known and unknown attributes could be represented by different instance variables (ActiveModel-backed @attributes and a raw @unknown_attributes Hash instance, maybe).

person.non_ar_hash = value
person.non_ar_hash[:not] = "changed"

skip "TODO: Failing due to indifferent access"
Copy link
Author

Choose a reason for hiding this comment

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

This will hopefully pass once the Hash#with_indifferent_access call is removed from #attributes.

@@ -1138,6 +1138,7 @@ def test_complex_clone
assert_equal matz.non_ar_hash, matz_c.non_ar_hash
assert_equal matz.non_ar_arr, matz_c.non_ar_arr

skip "TODO failing due to indifferent access"
Copy link
Author

Choose a reason for hiding this comment

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

Similarly, this will also pass once the Hash#with_indifferent_access call is no longer part of #attributes.

@seanpdoyle seanpdoyle force-pushed the attributes-without-casting branch from c28389b to d1d2bca Compare November 27, 2024 14:18
The `schema { ... }` interface pre-dates the Active Model Attributes API
(defined as early as [v5.2.0][]), but clearly draws inspiration from
Active Record's Database Schema and Attribute casting (which was
extracted into `ActiveModel::Attributes`).

However, the type information captured in `schema { ... }` blocks or
assigned as `Hash` arguments to `schema=` is purely inert metadata.

Proposal
---

This commit aims to integrate with [ActiveModel::Model][] and
[ActiveModel::Attributes][]. Through the introduction of both modules,
subclasses of `ActiveResource::Base` can benefit from type casting
attributes and constructing instances with default values.

This commit makes minimally incremental changes, prioritizing backwards
compatibility. The reliance on `#respond_to_missing?` and
`#method_missing` is left largely unchanged. Similarly, the `Schema`
interface continues to provide metadata about its attributes through the
`Schema#attr` method (instead of reading from
`ActiveModel::Attributes#attribute_names` or
`ActiveModel::Attributes.attribute_types`).

API Changes
---

To cast values to their specified types, declare the Schema with the
`:cast_values` set to true.

```ruby
class Person < ActiveResource::Base
  schema cast_values: true do
    integer 'age'
  end
end

p = Person.new
p.age = "18"
p.age # => 18
```

To configure inheriting resources to cast values, set the `cast_values`
class attribute:

```ruby
class ApplicationResource < ActiveResource::Base
  self.cast_values = true
end

class Person < ApplicationResource
  schema do
    integer 'age'
  end
end

p = Person.new
p.age = "18"
p.age # => 18
```

To set all resources application-wide to cast values, set
`config.active_resource.cast_values`:

```ruby
  # config/application.rb
  config.active_resource.cast_values = true
```

[v5.2.0]: https://api.rubyonrails.org/v5.2.0/classes/ActiveModel/Attributes/ClassMethods.html
[ActiveModel::Model]: https://api.rubyonrails.org/classes/ActiveModel/Model.html
[ActiveModel::Attributes]: https://api.rubyonrails.org/classes/ActiveModel/Attributes/ClassMethods.html
@seanpdoyle seanpdoyle force-pushed the attributes-without-casting branch from d1d2bca to d789946 Compare November 27, 2024 14:24
@jlurena
Copy link

jlurena commented Nov 29, 2024

Is this a draft? Once you are done maybe we can combine both our feature enhancements (mine is here: #409) and create a new major version of this gem.

@rails @byroot thoughts?

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

Successfully merging this pull request may close these issues.

2 participants