Added Utility Methods to Aws::Record Model Interface #82

YashoSharma · 2018-06-19T00:53:08Z

Description of changes:
Added:

destroyed?
new_record?
persisted?
update
update!
assign_attributes

Added unit tests to test this new functionality.
Performed integration tests to ensure no regression occurred.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

to_param destroyed? new_record? persisted? update assign_attributes

awood45

Good start, some changes proposed.

awood45 · 2018-06-19T16:40:04Z

lib/aws-record/record/attributes.rb

+        hkey = public_send(self.class.hash_key)
+        if self.class.range_key
+          rkey = public_send(self.class.range_key)
+          "#{hkey}:#{rkey}"


Can we document why this is the separator? It's very possible and valid for these key values to contain the ":" character. Depending on what the API contract is, we may need to do more here.

awood45 · 2018-06-19T16:41:40Z

lib/aws-record/record/item_operations.rb

+      end
+
+      # Updates model attributes and then performs a conditional save
+      def update(opts)


We should also add an #update! method, which essentially does the same thing except it raises an exception when the validation fails. Check the implementation of #save!

awood45 · 2018-06-19T16:42:30Z

lib/aws-record/record/item_operations.rb

+          if !respond_to?("#{field}=")
+            raise ArgumentError.new "Invalid field: #{field} for model"
+          end
+          @data.set_attribute field, new_value


Style point in this repo, we explicitly use () characters around any non-empty method calls. So:

@data.set_attribute(field, new_value)

awood45 · 2018-06-19T16:42:39Z

lib/aws-record/record/item_operations.rb

+
+      # Updates model attributes and then performs a conditional save
+      def update(opts)
+        self.assign_attributes opts


Add () characters

awood45 · 2018-06-19T16:44:13Z

lib/aws-record/record/item_operations.rb

@@ -151,6 +169,10 @@ def _perform_save(opts)
            )
          end
        end
+        data = self.instance_variable_get("@data")
+        data.destroyed = false if data.destroyed


Why the "if" condition here? It looks like the end condition is always going to be false or false-equivalent.

awood45 · 2018-06-19T17:01:39Z

spec/aws-record/record/dirty_tracking_spec.rb

+      item.body = SecureRandom.uuid
+      item.save
+
+      new_key = SecureRandom.uuid


I'd strongly encourage that we use explicit values in these tests. Why do we need randomness?

awood45 · 2018-06-19T17:04:17Z

spec/aws-record/record/item_collection_spec.rb

@@ -77,6 +77,23 @@ module Record
        end
      end

+      describe "#new_record" do
+        it 'marks records fetched from a client call as not being new' do


We should go ahead and add the "obvious" test that something created by hand is marked as a new record. Part of the test coverage notion is to avoid surprise regressions later.

awood45 · 2018-06-19T17:08:13Z

spec/aws-record/record/dirty_tracking_spec.rb

@@ -236,6 +260,39 @@

  end

+  describe "#update" do


It also looks like we never actually test-call the #update method in these tests.

awood45 · 2018-06-19T22:07:14Z

spec/aws-record/record/dirty_tracking_spec.rb

@@ -266,28 +303,60 @@
      klass.configure_client(client: stub_client)
    end

-    it 'should perform a hash based attribute assignment without persisting changes' do
+    it 'assign_attributes should perform a hash based attribute assignment without persisting changes' do


We should test this with ActiveModel::Model present as well.

awood45 · 2018-06-19T22:08:23Z

lib/aws-record/record/attributes.rb

        hkey = public_send(self.class.hash_key)
        if self.class.range_key
          rkey = public_send(self.class.range_key)
-          "#{hkey}:#{rkey}"
+          "#{URI.encode(hkey)}##{URI.encode(rkey)}"


How did we come up with the # character as a separator? We may need to do something more detailed like including the key names. That way, there's something concrete we can use as start/end points of most param values.

awood45

This looks good on code. For completeness, we need to:

Get the new method documentation up to the standard of other methods in aws-record. Should have usage examples, parameters explained, and return values tagged. For example, bring the update methods in parity with the save methods.
Add Cucumber tests, especially for the update method.
Add CHANGELOG notes explaining what we added.

awood45

Very close!

awood45 · 2018-06-20T23:08:27Z

CHANGELOG.md

@@ -1,6 +1,10 @@
 Unreleased Changes
 ------------------

+* Feature - Aws::Record::DirtyTracking - Add the `persisted?`, `new_record?`, and `destroyed` methods to `Aws::Record::DirtyTracking`, which supports use cases where you'd like to see if a record has just been newly initialized, or has been deleted or was a preexisting record retrieved from DynamoDB. Note that these methods are present in `ActiveModel::Model` so you should require that module before `Aws::Record`


In the changelog, let's avoid DirtyTracking and ItemOperations. These don't mean anything to an end user, they're basically code organization. Rather, just explain that these methods will now be tacked on to your models.

Also, we may need an "UPGRADING NOTES" bit on ActiveModel, I'll talk with you about that tomorrow.

awood45 · 2018-06-20T23:09:47Z

lib/aws-record/record/item_operations.rb

@@ -91,14 +94,20 @@ def assign_attributes(opts)
        end
      end

-      # Updates model attributes and then performs a conditional save
+      # Mass assigns the attributes to the model and then performs a save


Let's add usage examples to these, since attribute assignment is involved.

awood45 · 2018-06-20T23:10:46Z

lib/aws-record/record/item_operations.rb

+      #
+      # @param [Hash] opts
+      # @raise [Aws::Record::Errors::ValidationError] if any new values
+      #  violate the models validations.
      def update!(opts = {})


Can #update! be called without any options? This seems to contradict what we did in #update. It possibly makes the most sense to require the hash to have some attributes.

awood45

Code is well formed, but some interesting questions on design form. We may at least want to update examples.

awood45 · 2018-06-21T23:09:49Z

lib/aws-record/record/item_operations.rb

+      #   model.assign_attributes(id: 7, name: "Janeß")
+      #   model.id         # => 7
+      #   model.name       # => "Jane"
+      #   model.persisted? # => false


Wait, is this true? This should be a true value if I understand persisted? properly, and per the ActiveRecord documentation of the same. Is this a mistake?

You might be thinking of the #dirty? method, which would be false after saving, and true again after this operation.

Oh I get this, we changed the key. A better example would NOT change the key, rather some non-key attributes.

awood45 · 2018-06-21T23:11:34Z

lib/aws-record/record/item_operations.rb

+      # 
+      #   model.update(name: "Janeß")
+      #   model.name       # => "Jane"
+      #   model.persisted? # => true


I think we're thinking of the dirty? method here, which would be false since the attribute changes and save were concurrent. This also would benefit from an example where we are not changing the key.

awood45 · 2018-06-21T23:13:41Z

lib/aws-record/record/item_operations.rb

      end

      # Updates model attributes and validates new values
      #
+      # You can use the +:force+ option to perform a simple put/overwrite


This actually raises some interesting semantic questions. In ActiveRecord, they generally step around this because the ID field is a semi-hidden field. Where here, #update could be a put_item operation depending on the context. I don't think this is inherently wrong, but we either want to:

Call this out more explicitly in documentation.

Prevent the changing of key attributes in #assign_attributes, ensuring a pure update style.

Thoughts?

While it initially seems wrong for the keys to be mutable, the current implementation is consistent with what happens if the user manually changes the key value and performs a save.

I agree with you. I just think we should be explicit about this, because it has a chance to be a surprise to some users.

Added methods to Aws::Record interface:

582fb49

to_param destroyed? new_record? persisted? update assign_attributes

awood45 suggested changes Jun 19, 2018

View reviewed changes

awood45 reviewed Jun 19, 2018

View reviewed changes

YashoSharma added 3 commits June 19, 2018 11:52

refactored based on cr

911118c

Handle to_param when not persisted

9bfd913

Change key seperator

ee13827

awood45 suggested changes Jun 19, 2018

View reviewed changes

YashoSharma added 6 commits June 19, 2018 15:22

Use ; delimiter for URLs

fd5c694

add am:model tests for update functionality

a7119e8

removed to_param

0a4f1fc

Fixed a bug where items found were marked as newly created

927c728

Symbolize update keys

1e7bd5c

Symbolize update keys

122330d

awood45 suggested changes Jun 20, 2018

View reviewed changes

Documentation

5262302

awood45 suggested changes Jun 20, 2018

View reviewed changes

YashoSharma added 6 commits June 21, 2018 13:20

fix update bugs

d36551d

simplified update logic

d6391c1

added usage notes

f89c95d

Updated Changelog

bfe72a6

Use top level setter in update

61c66c2

Bugfix

ad6a46c

awood45 suggested changes Jun 21, 2018

View reviewed changes

YashoSharma added 2 commits June 21, 2018 16:27

Updated usage examples

32337e6

Explicitly document update behaviour on key changes

c95a62e

awood45 approved these changes Jun 22, 2018

View reviewed changes

awood45 merged commit b4dadfc into aws:master Jun 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Utility Methods to Aws::Record Model Interface #82

Added Utility Methods to Aws::Record Model Interface #82

YashoSharma commented Jun 19, 2018 •

edited

Loading

awood45 left a comment

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 Jun 19, 2018

awood45 left a comment

awood45 left a comment

awood45 Jun 20, 2018

awood45 Jun 20, 2018

awood45 Jun 20, 2018

awood45 left a comment

awood45 Jun 21, 2018

awood45 Jun 21, 2018

awood45 Jun 21, 2018

awood45 Jun 21, 2018

awood45 Jun 21, 2018

YashoSharma Jun 21, 2018

awood45 Jun 21, 2018

Added Utility Methods to Aws::Record Model Interface #82

Added Utility Methods to Aws::Record Model Interface #82

Conversation

YashoSharma commented Jun 19, 2018 • edited Loading

awood45 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

awood45 left a comment

Choose a reason for hiding this comment

awood45 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

awood45 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

YashoSharma commented Jun 19, 2018 •

edited

Loading