Make test attributes explicit #1984

bf4 · 2016-11-21T16:52:36Z

Follows #1982

bf4 · 2016-11-21T16:57:03Z

lib/active_model_serializers/model.rb

+    end
+
+    def _assign_attribute(k, v)
+      fail UnknownAttributeError.new(self, k) unless respond_to?("#{k}=")


TODO: check vs. attribute_names

bf4 · 2016-11-21T16:57:49Z

lib/active_model_serializers/model.rb

+    end
+
+    # Raised when unknown attributes are supplied via mass assignment.
+    class UnknownAttributeError < NoMethodError


copied from Rails 5 ActiveModel::AttributeAssignment

bf4 · 2016-11-21T16:58:12Z

test/action_controller/adapter_selector_test.rb

@@ -15,7 +15,7 @@ def render_using_adapter_override
        end

        def render_skipping_adapter
-          @profile = Profile.new(name: 'Name 1', description: 'Description 1', comments: 'Comments 1')
+          @profile = Profile.new(id: 'render_skipping_adapter_id', name: 'Name 1', description: 'Description 1', comments: 'Comments 1')


probably a good thing that id, which is required, is now included

ref https://github.com/rails-api/active_model_serializers/pull/1984/files#r89365145

bf4 · 2016-11-21T16:58:37Z

test/action_controller/namespace_lookup_test.rb

@@ -50,7 +49,7 @@ class LookupTestController < ActionController::Base

            def implicit_namespaced_serializer
              writer = Writer.new(name: 'Bob')
-              book = Book.new(title: 'New Post', body: 'Body', writer: writer, chapters: [])
+              book = Book.new(id: 'bookid', title: 'New Post', body: 'Body', writer: writer, chapters: [])


bf4 · 2016-11-21T17:02:08Z

test/action_controller/namespace_lookup_test.rb

@@ -205,7 +204,7 @@ def namespace_set_by_request_headers

        assert_serializer ActiveModel::Serializer::Null

-        expected = { 'title' => 'New Post', 'body' => 'Body' }
+        expected = { 'id' => 'invalid_namespace_book_id', 'title' => 'New Post', 'body' => 'Body', 'writer'=>nil, 'chapters'=>nil}


see #1985 which would aim to have the the PORO model attributes/associations map better to the Serializer attributes/associations

mention-bot · 2016-11-21T17:02:35Z

@bf4, thanks for your PR! By analyzing the history of the files in this pull request, we identified @NullVoxPopuli, @bolshakov and @kevintyll to be potential reviewers.

bf4 · 2016-11-21T17:03:07Z

test/cache_test.rb

@@ -323,15 +328,21 @@ def test_cache_digest_definition
    end

    def test_object_cache_keys
+      class << @comment
+        def cache_key; "comment/#{id}"; end
+      end


Is hard-coded in test/poro/fixtures.rb but we're not using that anymore

class Comment < Model # Uses a custom non-time-based cache key def cache_key "#{self.class.name.downcase}/#{id}" end end

bf4 · 2016-11-21T17:05:31Z

lib/active_model_serializers/model.rb

+    include ActiveModel::Validations
+    include ActiveModel::Conversion
+    extend ActiveModel::Naming
+    extend ActiveModel::Translation


code adapted from ActiveModel::Model which is has good logic in it, but doesn't still has a few cases we want to handle differently

what are those use cases?

NullVoxPopuli · 2016-11-21T21:13:14Z

LGTM, ready for merge?

bf4 · 2016-11-21T22:03:52Z

@NullVoxPopuli I want to finish cleaning up some of the test poro attributes -> associations

bf4 · 2016-11-21T23:43:33Z

test/action_controller/json_api/fields_test.rb

-                             publish_at: '2020-03-16T03:55:25.291Z')
+            @post = PostWithPublishAt.new(id: 1337, title: 'Title 1', body: 'Body 1',
+                                          author: @author, comments: [@comment1, @comment2],
+                                          publish_at: '2020-03-16T03:55:25.291Z')


¯\_(ツ)_/¯ rubocop

bf4 · 2016-11-21T23:43:50Z

test/action_controller/serialization_test.rb

@@ -135,7 +135,7 @@ def render_fragment_changed_object_with_relationship
          like = Like.new(id: 1, likeable: comment, time: 3.days.ago)

          generate_cached_serializer(like)
-          like.likable = comment2
+          like.likeable = comment2


well, caught it because it raised an error, undefined attribute.. makes me wonder if it was being used...

bf4 · 2016-11-21T23:45:12Z

test/fixtures/poro.rb

-    else
-      super
-    end
+  # At this time, just for organization of intent


probably good to promote to parent class once we add some additional behavior to the attributes method

bf4 · 2016-11-22T01:36:19Z

test/active_model_serializers/model_test.rb

+      assert_equal expected_attributes, instance.attributes
+      assert_equal :not_one, instance.one
+      assert_equal :not_one, instance.read_attribute_for_serialization(:one)
+    end


is fixed #1903

ref #1877

bug ref #1857 (comment)

bf4 · 2016-11-22T01:49:39Z

cc @richmolj

beauby

I'm lacking a bit of context on this one, but the code looks good. See a few minor remarks.

beauby · 2016-11-22T16:42:53Z

lib/active_model_serializers/model.rb


    def self.attributes(*names)
      attr_accessor(*names)
+      self.attribute_names = attribute_names | names.map(&:to_sym)


self.attribute-names |= names.map(&:to_sym)

beauby · 2016-11-22T16:44:18Z

lib/active_model_serializers/model.rb

      @errors = ActiveModel::Errors.new(self)
      super
    end

    # Defaults to the downcased model name.
    def id
-      attributes.fetch(:id) { self.class.name.downcase }
+      @id ||= self.class.name && self.class.name.downcase


The id method should return a unique identifier for a resource, no? Not sure if providing such a default is more useful than dangerous.

@beauby I agree this is a somewhat problematic implementation. Also, I think only JSON API requires an 'id'.

In that case, I'd suggest trying to read the :id attribute, and if none available, fall back to object_id.

beauby · 2016-11-22T16:46:13Z

lib/active_model_serializers/model.rb

    end

    # Defaults to the downcased model name and updated_at
    def cache_key
-      attributes.fetch(:cache_key) { "#{self.class.name.downcase}/#{id}-#{updated_at.strftime('%Y%m%d%H%M%S%9N')}" }
+      ActiveSupport::Cache.expand_cache_key([
+        self.class.name && self.class.name.downcase,


How about defaulting to self.class.object_id for anonymous classes? That way you avoid colliding all anonymous classes in the cache.

I thought about that, or hash, but I'm not sure that's any better, as it's not really a unique identifier. We could hash the attributes, I guess. If we just used ActiveModel::Name then it would blow up on the anonymous class and we could rely on it's exception message.

self.class.object_id is a unique identifier for the class. For the the instances, I don't see any better solution than hashing the attributes, which would suck because it would either be complicated or fail for hashes. I'd vote for no caching for those models.

Should I just remove 'id' as required at the Model level?

Yeah, people who use JSON API should be aware of the first 5 lines of the spec.

beauby · 2016-11-22T16:49:08Z

lib/active_model_serializers/model.rb

    end

    # Defaults to the time the serializer file was modified.
    def updated_at
-      attributes.fetch(:updated_at) { File.mtime(__FILE__) }
+      defined?(@updated_at) ? @updated_at : File.mtime(__FILE__)


It is safe to do @updated_at || File.mtime(__FILE__).

@beauby yeah, but give ruby warning undefined ivar

bf4 · 2016-11-23T12:25:16Z

lib/active_model_serializers/model.rb

+    def attributes
+      attribute_names.each_with_object({}) do |attribute_name, result|
+        result[attribute_name] = public_send(attribute_name)
+      end.with_indifferent_access


bf4 · 2016-11-23T12:27:20Z

lib/active_model_serializers/model/caching.rb

+      def cache_key
+        ActiveSupport::Cache.expand_cache_key([
+          self.class.model_name.name.downcase,
+          "#{id}-#{updated_at.strftime('%Y%m%d%H%M%S%9N')}"


This will blow up if updated_at isn't a 'Time', but that will only happen when users set updated_at, since the default is File.mtime

note that cache_key isn't included in attribute_names and has no defined cache_key= setter.

bf4 · 2016-11-25T04:00:04Z

@groyoh Just looks for consensus since there are some important changes in here.

bf4 · 2016-11-27T05:45:29Z

@rails-api/ams @rails-api/commit ready for final review. Should I break this up? Good enough?

groyoh · 2016-11-27T11:37:33Z

lib/active_model_serializers/model/caching.rb

      def cache_key
        ActiveSupport::Cache.expand_cache_key([
          self.class.model_name.name.downcase,
          "#{id}-#{updated_at.strftime('%Y%m%d%H%M%S%9N')}"
        ].compact)
      end

-      # Defaults to the time the serializer file was modified.
+      # When no set, defaults to the time the file was modified.


s/no/not/ ?

groyoh · 2016-11-27T11:41:09Z

Isn't there too many changes here?

groyoh · 2016-11-27T11:43:42Z

test/serializers/associations_test.rb

+            instance_options
+          end
+        end
+        assert comment_serializer.custom_options


Shoudn't that be assert comment_serializer.custom_options[:custom_options]? or the above method be instance_options[:custom_options]? You're currently testing assert {} I think.

thanks, you're right! I lost the custom_options[:custom_options]

bf4 · 2016-11-27T16:17:57Z

@groyoh

Isn't there too many changes here?

Yes, but I'm not sure how I'd like to break it up, since they go well together... part of the issue is that making the test attributes explicit, we need to decide when id or updated_at should be in the attributes hash, IIRC

I'm open to any suggestions how best to do it. That's why I'm not comfortable merging just yet.

bf4 · 2016-11-27T16:20:21Z

test/serializers/attribute_test.rb

@@ -98,7 +98,9 @@ def test_virtual_attribute_block

      # rubocop:disable Metrics/AbcSize
      def test_conditional_associations
-        model = ::Model.new(true: true, false: false)
+        model = Class.new(::Model) do


use poro builder

groyoh · 2016-11-28T08:42:37Z

lib/active_model_serializers/model.rb

-      else
-        attributes[key]
-      end
+    # The the fields in +attribute_names+ determines the returned hash.


typo The the

- Organize test poros with associations and by serializer - Freeze derived attributes/associations against mutation - Cleanup PORO fixtures

bf4 · 2016-12-05T01:35:52Z

@rails-api/ams @rails-api/commit ready for review again

NullVoxPopuli · 2016-12-05T02:18:16Z

test/action_controller/json_api/transform_test.rb

-          class TopComment < ::Model; end
+          class Post < ::Model
+            attributes :title, :body, :publish_at
+            associations :author, :top_comments


i'm certainly a fan of this being more explicit

NullVoxPopuli · 2016-12-05T02:18:51Z

test/active_model_serializers/model_test.rb

@@ -18,5 +18,52 @@ 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


should new tests be using test '...' do?

oh, I guess so. I guess I haven't completely switch to activesupport form. I mostly just wanted the stable interface vs. minitest

NullVoxPopuli · 2016-12-05T02:21:53Z

I'm mostly 👍

bf4 commented Nov 21, 2016

View reviewed changes

bf4 mentioned this pull request Nov 21, 2016

WIP Test poro pseudo associations #1985

Closed

bf4 commented Nov 21, 2016

View reviewed changes

bf4 force-pushed the make_test_attributes_explicit branch 3 times, most recently from 0df26d0 to be534ef Compare November 21, 2016 19:32

bf4 force-pushed the make_test_attributes_explicit branch 2 times, most recently from f00d678 to 6ae54ee Compare November 21, 2016 23:42

bf4 commented Nov 21, 2016

View reviewed changes

bf4 commented Nov 22, 2016

View reviewed changes

bf4 added S: Bug C: Cleanup V: 0.10.x labels Nov 22, 2016

beauby reviewed Nov 22, 2016

View reviewed changes

bf4 force-pushed the make_test_attributes_explicit branch from 1e6f7cd to 8dc1124 Compare November 22, 2016 20:47

bf4 mentioned this pull request Nov 22, 2016

Relationships with ActiveModelSerializers::Model #1961

Closed

bf4 force-pushed the make_test_attributes_explicit branch from 8dc1124 to 37f85b3 Compare November 22, 2016 21:47

bf4 commented Nov 23, 2016

View reviewed changes

bf4 added the S: Changelog Missing label Nov 25, 2016

bf4 mentioned this pull request Nov 27, 2016

Fix relationship links doc #1981

Merged

bf4 force-pushed the make_test_attributes_explicit branch 3 times, most recently from d604d13 to 539683f Compare November 27, 2016 05:20

groyoh reviewed Nov 27, 2016

View reviewed changes

bf4 commented Nov 27, 2016

View reviewed changes

groyoh reviewed Nov 28, 2016

View reviewed changes

bf4 force-pushed the make_test_attributes_explicit branch 2 times, most recently from d4eba71 to f36943a Compare December 5, 2016 01:30

bf4 added 3 commits December 4, 2016 19:33

Make test attributes explicit

80af763

- Organize test poros with associations and by serializer - Freeze derived attributes/associations against mutation - Cleanup PORO fixtures

Add failing test for AMS::Model accessor vs. attributes mutation

f8ca912

Add CHANGELOG

3b84845

bf4 force-pushed the make_test_attributes_explicit branch from f36943a to 3b84845 Compare December 5, 2016 01:33

NullVoxPopuli reviewed Dec 5, 2016

View reviewed changes

bf4 removed the S: Changelog Missing label Dec 5, 2016

bf4 merged commit 847f50a into rails-api:master Dec 6, 2016

bf4 deleted the make_test_attributes_explicit branch December 6, 2016 20:53

This was referenced Dec 6, 2016

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

Closed

WIP: Restore legacy ams::model behavior #1998

Closed

This was referenced Jan 8, 2017

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

Merged

Fix model attribute accessors #2022

Merged

Make test attributes explicit #1984

Make test attributes explicit #1984

Conversation

bf4 commented Nov 21, 2016 • edited Loading

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

mention-bot commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Nov 21, 2016

bf4 commented Nov 21, 2016

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 Nov 22, 2016

beauby 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

bf4 commented Nov 25, 2016

bf4 commented Nov 27, 2016

Choose a reason for hiding this comment

groyoh commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Dec 5, 2016

bf4 commented Nov 21, 2016 •

edited

Loading

bf4 Nov 23, 2016 •

edited

Loading