WIP: Restore legacy ams::model behavior #1998

bf4 · 2016-12-12T05:32:45Z

Needed to make #1982 and #1984 not a breaking change.

Successfully tested in an app that broke in various ways on master without this PR, but works with it
Why this is wip? now we need tests for with and without the prepended module, for both the 'new' and 'legacy' behavior
Implementation-wise: Why it's prepended on inherited? That way, it can be easily disabled.
Next steps: I intend to do something similar for caching, so that it is optional to include caching concerns in the poro, where it doesn't usually make sense.

NullVoxPopuli

Just posted some thoughts.

NullVoxPopuli · 2016-12-12T12:42:20Z

lib/active_model_serializers/model.rb

+    # Configuration to avoid a breaking change with older versions of this class which lacked defined attributes.
+    # Previous behavior was: 1) initialized attributes were the
+    class_attribute :attributes_are_always_the_initialization_data, instance_writer: false, instance_reader: false
+    self.attributes_are_always_the_initialization_data = false # remove this commit, just for demonstration


remove this commit?

So it looks like if someone wanted the old functionality, they'd just set attributes_are_always_the_initialization_data to true at the top of their class inheriting from AMS::Model?

But, I don't see an include AttributesAreAlwaysTheInitializationData anywhere. Do you want to add tests for this?

sorry to be unclear, I just added Hey look it passes to show that I didn't break the existing test suite when 'false'. I'm going to remove that commit :)

NullVoxPopuli · 2016-12-12T12:42:54Z

test/action_controller/serialization_test.rb

@@ -456,7 +456,7 @@ def use_adapter?
        end
      end

-      def test_render_event_is_emmited


so many spelling errors, nice finds.

NullVoxPopuli · 2016-12-12T12:45:40Z

lib/active_model_serializers/model.rb

+    def self.inherited(subclass)
+      if subclass.attributes_are_always_the_initialization_data
+        unless subclass.included_modules.include?(AttributesAreAlwaysTheInitializationData)
+          subclass.prepend(AttributesAreAlwaysTheInitializationData)


This is curious. Is this changing the default include Module functionality to make the module be a kind of superclass to the class using the module?

prepend is basically the same as include except it inserts the module below instead of above the class in the inheritance chain, such that the module can override the class' methods.

oh ok, I thought it went the other way.

@NullVoxPopuli this is just a compatibility layer to avoid a breaking change. I should add some docs around setting it true vs. false and how to do both at same time and why :)

bf4 · 2016-12-27T03:43:26Z

lib/active_model_serializers/model.rb

@@ -6,7 +6,21 @@ class Model
    include ActiveModel::Serializers::JSON
    include ActiveModel::Model

-    class_attribute :attribute_names
+    # Configuration to avoid a breaking change with older versions of this class which lacked defined attributes.
+    # Previous behavior was: 1) initialized attributes were the


what in the world happened to this comment?

I think what it's trying to say is that the 'legacy' behavior of the attributes hash was that it was always the value of the attributes passed in at initialization, regardless of what attr_* methods were defined.

This change now lets you define the attributes so that 1) unset attributes are now nil and 2) you can change the attributes after initialization by using the attr_writer, rather than hackily mutating the return value of the attributes method 3) the return value of the attributes method is now frozen. since it is always derived, trying to mutate it is strongly discouraged :)

The details is that the 'old' behavior is only mixed in when inheriting and attributes_are_always_the_initialization_data is true, which is the default, which is good for backward compatibility. However, new apps probably want to change the default to false, and old apps want to create a special new model subclass where it is false for use in new models.

For now 'attributes' and associations are both treated the same, since I'm not sure that it makes sense for a PORO to have associations outside of our tests.

bf4 · 2016-12-27T03:53:09Z

test/fixtures/poro.rb

@@ -1,3 +1,4 @@
+ActiveModelSerializers::Model.attributes_are_always_the_initialization_data = false


see https://github.com/rails-api/active_model_serializers/pull/1998/files#r93893962 but the test poro superclass can't have the legacy behavior included because there's no simple way to remove it. Instead, the test poro superclass has the 'new' behavior, but, subclasses of it, by default, will have the 'old' behavior. Basically, it kind of extends the AMS::Model in the test context so that we can test it with the legacy behavior on and off.

bf4 · 2016-12-27T03:53:20Z

test/support/poro.rb

+module ActiveModelSerializersWithoutLegacyModelSupport
+  module_function
+
+  def poro_without_legacy_model_support(superklass = ActiveModelSerializers::Model, &block)


see https://github.com/rails-api/active_model_serializers/pull/1998/files#r93894191

An example test helper I derived from one I wrote in some unmerged commits

bf4 · 2016-12-27T03:54:21Z

test/action_controller/adapter_selector_test.rb

@@ -3,6 +3,14 @@
 module ActionController
  module Serialization
    class AdapterSelectorTest < ActionController::TestCase
+      Profile = poro_without_legacy_model_support(::Model) do


ref https://github.com/rails-api/active_model_serializers/pull/1998/files#r93894191

bf4 · 2016-12-27T03:54:44Z

lib/active_model_serializers/model.rb

+
+      # Defaults to the downcased model name.
+      def id
+        @initialized_attributes.fetch(:id) { self.class.model_name.name && self.class.model_name.name.downcase }


probably not a good default, but that was the old behavior.

bf4 · 2016-12-27T03:55:04Z

lib/active_model_serializers/model.rb

+      end
+
+      def attributes
+        @initialized_attributes


yup, they can be mutated https://github.com/rails-api/active_model_serializers/pull/1998/files#r93893962

bf4 · 2016-12-27T03:56:49Z

lib/active_model_serializers/model.rb

@@ -37,7 +51,7 @@ def initialize(attributes = {})
    # +attributes+ are returned frozen to prevent any expectations that mutation affects
    # the actual values in the model.
    def attributes
-      attribute_names.each_with_object({}) do |attribute_name, result|
+      self.class.attribute_names.each_with_object({}) do |attribute_name, result|


removed the instance reader/writer since the ActiveModel::Serialization mixin in Rails uses an attribute_names local variable, which I found because it conflicted in some scenario I've forgotten

bf4 · 2017-01-09T04:27:03Z

Closed by #2022

bf4 requested a review from NullVoxPopuli December 12, 2016 05:33

bf4 added C: Regression V: 0.10.x labels Dec 12, 2016

NullVoxPopuli requested changes Dec 12, 2016

View reviewed changes

bf4 force-pushed the restore_legacy_ams_model_behavior branch from bba724c to c641ba1 Compare December 16, 2016 19:44

bf4 force-pushed the restore_legacy_ams_model_behavior branch from c641ba1 to faed3b0 Compare December 26, 2016 15:21

bf4 commented Dec 27, 2016

View reviewed changes

bf4 mentioned this pull request Dec 27, 2016

AMS >= 0.10.3 : support ruby 2.4 #2004

Closed

bf4 mentioned this pull request Jan 6, 2017

Prepare release of 0.10.4 #2016

Merged

bf4 force-pushed the restore_legacy_ams_model_behavior branch from faed3b0 to b0bf140 Compare January 7, 2017 04:13

bf4 added 8 commits January 7, 2017 15:26

Restore legacy ams::model behavior

f508ecd

Hey look, it passes

c1e99d1

Restore tests without legacy attributes support

d27d63b

Deprecated docs

a9710db

ams model docs

fefa1f7

deprecation warning needs to be tested

ea87892

Fix style

22a7cf7

ActiveModel::Serializer::Null does not need testing

ffcc3e4

bf4 force-pushed the restore_legacy_ams_model_behavior branch from b0bf140 to ffcc3e4 Compare January 7, 2017 21:31

This was referenced Jan 8, 2017

Add Model#attributes helper; make test attributes explicit #2021

Merged

Fix model attribute accessors #2022

Merged

bf4 closed this Jan 9, 2017

bf4 deleted the restore_legacy_ams_model_behavior branch January 9, 2017 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Restore legacy ams::model behavior #1998

WIP: Restore legacy ams::model behavior #1998

bf4 commented Dec 12, 2016 •

edited

NullVoxPopuli left a comment

NullVoxPopuli Dec 12, 2016

NullVoxPopuli Dec 12, 2016

NullVoxPopuli Dec 12, 2016

bf4 Dec 13, 2016 •

edited

NullVoxPopuli Dec 12, 2016

NullVoxPopuli Dec 12, 2016

beauby Dec 12, 2016

NullVoxPopuli Dec 12, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016 •

edited

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 Dec 27, 2016

bf4 commented Jan 9, 2017

		@@ -1,3 +1,4 @@
		ActiveModelSerializers::Model.attributes_are_always_the_initialization_data = false

WIP: Restore legacy ams::model behavior #1998

WIP: Restore legacy ams::model behavior #1998

Conversation

bf4 commented Dec 12, 2016 • edited

NullVoxPopuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 Dec 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 Dec 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Jan 9, 2017

bf4 commented Dec 12, 2016 •

edited

bf4 Dec 13, 2016 •

edited

bf4 Dec 27, 2016 •

edited