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.

Sign up for GitHub

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

Fix AMS::Model accessor vs. attributes mutation #1903

Closed
wants to merge 3 commits into from
Closed

Fix AMS::Model accessor vs. attributes mutation #1903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
206e71e
Add failing test for AMS::Model accessor vs. attributes mutation
bf4 Sep 1, 2016
c91f3ff
use send if the model responds_to the method, otherwise fallback to a…
NullVoxPopuli Sep 24, 2016
9ddf004
remove testing gems
NullVoxPopuli Sep 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/active_model_serializers/model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ def updated_at
end

def read_attribute_for_serialization(key)
return send(key) if respond_to?(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I'm not totally sure this would work. You'd get into this scenario:

class Post < ActiveModelSerializers::Model
  attr_accessor :title
end

post = Post.new('title' => 'my title')
post.read_attribute_for_serialization(:title) # => nil

Copy link
Contributor

Choose a reason for hiding this comment

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

that's true. I think that's just another point for the current AMS::Model being broken. :-(

Do you have a PR for your module idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I was waiting to get approval of the RFC. Because what would that module be? Would it contain the constructor logic (in which case we are in the business of overriding the superclass's constructor which is gross)? And if not, what would it include?

As far as I can tell it would just be alias read_attribute_for_serialization send, and in that case I'm not sure what the point of it is.

So, TBH AMS::Model is nonsensical to me, which means I don't think I can touch the code or enhance the documentation in a meaningful way. I created the RFC to try and improve that situation, but doesn't look like it is getting any traction.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW you can fix the current class with something like this and this. That got the tests passing, including the scenario above, though it's just POC code.


if key == :id || key == 'id'
attributes.fetch(key) { id }
else
Expand Down
43 changes: 42 additions & 1 deletion test/active_model_serializers/model_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module ActiveModelSerializers
class ModelTest < ActiveSupport::TestCase
include ActiveModel::Serializer::Lint::Tests

def setup
setup do
@resource = ActiveModelSerializers::Model.new
end

Expand All @@ -18,5 +18,46 @@ def test_initialization_with_string_keys

assert_equal model_instance.read_attribute_for_serialization(:key), value
end

def test_attributes_can_be_read_for_serialization
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this test is about a change via accessor instead of constructor. Maybe the test name could be a little more descriptive in that regard?

klass = Class.new(ActiveModelSerializers::Model) do
attr_accessor :one, :two, :three
end
original_attributes = { one: 1, two: 2, three: 3 }
instance = klass.new(original_attributes)

# Initial value
expected_attributes = { one: 1, two: 2, three: 3 }
assert_equal expected_attributes, instance.attributes
assert_equal 1, instance.one
assert_equal 1, instance.read_attribute_for_serialization(:one)

# Change via accessor
instance.one = :not_one

assert_equal :not_one, instance.one
assert_equal :not_one, instance.read_attribute_for_serialization(:one)
end

def test_id_attribute_can_be_read_for_serialization
klass = Class.new(ActiveModelSerializers::Model) do
attr_accessor :id, :one, :two, :three
end
self.class.const_set(:SomeTestModel, klass)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

original_attributes = { id: :ego, one: 1, two: 2, three: 3 }
instance = klass.new(original_attributes)

# Initial value
assert_equal 1, instance.one
assert_equal 1, instance.read_attribute_for_serialization(:one)

# Change via accessor
instance.id = :superego

assert_equal :superego, instance.id
assert_equal :superego, instance.read_attribute_for_serialization(:id)
ensure
self.class.send(:remove_const, :SomeTestModel)
end
end
end