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

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

Merged
merged 19 commits into from
Jun 22, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
13 changes: 13 additions & 0 deletions lib/aws-record/record/attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ def to_h
@data.hash_copy
end

# Returns a string representation of the key attributes
#
# @return [String] String of key attributes that can be used to query a model
def to_param
hkey = public_send(self.class.hash_key)
if self.class.range_key
rkey = public_send(self.class.range_key)
"#{hkey}:#{rkey}"
Copy link
Member

Choose a reason for hiding this comment

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

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.

else
"#{hkey}"
end
end

module ClassMethods

# Define an attribute for your model, providing your own attribute type.
Expand Down
12 changes: 12 additions & 0 deletions lib/aws-record/record/dirty_tracking.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,18 @@ def dirty?
@data.dirty?
end

def persisted?
@data.persisted?
end

def new_record?
@data.new_record?
end

def destroyed?
@data.destroyed?
end

# Fetches attributes for this instance of an item from Amazon DynamoDB
# using its primary key and the +find(*)+ class method.
#
Expand Down
1 change: 1 addition & 0 deletions lib/aws-record/record/item_collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def _build_items_from_response(items, model)
data.set_attribute(name, attr.extract(item))
end
data.clean!
data.new_record = false
ret << record
end
ret
Expand Down
17 changes: 17 additions & 0 deletions lib/aws-record/record/item_data.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@ def initialize(model_attributes, opts)
@model_attributes = model_attributes
@track_mutations = opts[:track_mutations]
@track_mutations = true if opts[:track_mutations].nil?
@new_record = true
@destroyed = false

populate_default_values
end

attr_accessor :new_record, :destroyed

def get_attribute(name)
@model_attributes.attribute_for(name).type_cast(@data[name])
Expand All @@ -34,6 +39,18 @@ def set_attribute(name, value)
@data[name] = value
end

def new_record?
@new_record
end

def destroyed?
@destroyed
end

def persisted?
!(new_record? || destroyed?)
end

def raw_value(name)
@data[name]
end
Expand Down
24 changes: 23 additions & 1 deletion lib/aws-record/record/item_operations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ def save(opts = {})
end
end


# Performs mass assignment of model attributes
def assign_attributes(opts)
opts.each do |field, new_value|
if !respond_to?("#{field}=")
raise ArgumentError.new "Invalid field: #{field} for model"
end
@data.set_attribute field, new_value
Copy link
Member

Choose a reason for hiding this comment

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

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

@data.set_attribute(field, new_value)

end
end

# Updates model attributes and then performs a conditional save
def update(opts)
Copy link
Member

Choose a reason for hiding this comment

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

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!

self.assign_attributes opts
Copy link
Member

Choose a reason for hiding this comment

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

Add () characters


self.save
end

# Deletes the item instance that matches the key values of this item
# instance in Amazon DynamoDB. Uses the
# {http://docs.aws.amazon.com/sdkforruby/api/Aws/DynamoDB/Client.html#delete_item-instance_method Aws::DynamoDB::Client#delete_item}
Expand All @@ -90,7 +108,7 @@ def delete!
table_name: self.class.table_name,
key: key_values
)
true
self.instance_variable_get("@data").destroyed = true
end

private
Expand Down Expand Up @@ -151,6 +169,10 @@ def _perform_save(opts)
)
end
end
data = self.instance_variable_get("@data")
data.destroyed = false if data.destroyed
Copy link
Member

Choose a reason for hiding this comment

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

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

data.new_record = false if data.new_record
true
end

def _build_item_for_save
Expand Down
16 changes: 16 additions & 0 deletions spec/aws-record/record/attributes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,22 @@ module Record
item.clever = "No problem."
expect(item.to_h).to eq({ clever: "No problem." })
end

it 'should provide a parametrized representation of the hash key' do
klass.string_attr(:mykey, hash_key: true)
item = klass.new
item.mykey = "test_key"
expect(item.to_param).to eq("test_key")
end

it 'should provide a parametrized representation of hash and range keys' do
klass.string_attr(:mykey, hash_key: true)
klass.string_attr(:myrkey, range_key: true)
item = klass.new
item.mykey = "test_key"
item.myrkey = "test_range_key"
expect(item.to_param).to eq("test_key:test_range_key")
end
end

end
Expand Down
57 changes: 57 additions & 0 deletions spec/aws-record/record/dirty_tracking_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,30 @@

end

describe "persisted?" do
before(:each) do
klass.configure_client(client: stub_client)
end

it "appropriately determines whether an item is persisted" do
item = klass.new
item.mykey = SecureRandom.uuid
item.body = SecureRandom.uuid

# Test all combinations of new_recorded and destroyed
expect(item.persisted?).to be false
item.save
expect(item.persisted?).to be true
item.delete!
expect(item.persisted?).to be false
item = klass.new
item.mykey = SecureRandom.uuid
item.body = SecureRandom.uuid
item.delete!
expect(item.persisted?).to be false
end
end

describe '#rollback_[attribute]!' do

it "should restore the attribute to its last known clean value" do
Expand Down Expand Up @@ -236,6 +260,39 @@

end

describe "#update" do
Copy link
Member

Choose a reason for hiding this comment

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

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


before(:each) do
klass.configure_client(client: stub_client)
end

it 'should perform a hash based attribute assignment without persisting changes' do
item = klass.new
item.mykey = SecureRandom.uuid
item.body = SecureRandom.uuid
item.save

new_key = SecureRandom.uuid
Copy link
Member

Choose a reason for hiding this comment

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

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

new_body = SecureRandom.uuid
item.assign_attributes :mykey => new_key, :body => new_body
expect(item.mykey).to eq new_key
expect(item.body).to eq new_body
expect(item.dirty?).to be true
end

it 'should throw an argument error when you try to update an invalid attribute' do
item = klass.new
item.mykey = SecureRandom.uuid
item.body = SecureRandom.uuid
item.save

expect {
item.assign_attributes :mykey_key => SecureRandom.uuid
}.to raise_error(ArgumentError)
end

end

describe "#save" do

before(:each) do
Expand Down
17 changes: 17 additions & 0 deletions spec/aws-record/record/item_collection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

stub_client.stub_responses(:scan, non_truncated_resp)
c = ItemCollection.new(
:scan,
{ table_name: "TestTable" },
model,
stub_client
)

c.each do |record|
expect(record.new_record?).to eq false
expect(record.destroyed?).to be false
end
end
end

describe "#last_evaluated_key" do
it "points you to the client response pagination value if present" do
stub_client.stub_responses(:scan, truncated_resp)
Expand Down
16 changes: 16 additions & 0 deletions spec/aws-record/record/item_operations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ module Record
item.id = 1
item.date = '2015-12-14'
item.body = 'Hello!'
expect(item.new_record?).to be(true)
item.save
expect(item.new_record?).to be(false)
expect(api_requests).to eq([{
table_name: "TestTable",
item: {
Expand Down Expand Up @@ -420,6 +422,7 @@ module Record
item.id = 3
item.date = "2015-12-17"
expect(item.delete!).to be(true)
expect(item.destroyed?).to be(true)
expect(api_requests).to eq([{
table_name: "TestTable",
key: {
Expand All @@ -430,6 +433,19 @@ module Record
end
end

describe "save after delete scenarios" do
it 'sets destroyed to false after saving a destroyed record' do
klass.configure_client(client: stub_client)
item = klass.new
item.id = 3
item.date = "2015-12-17"
expect(item.delete!).to be(true)
expect(item.destroyed?).to be(true)
item.save
expect(item.destroyed?).to be(false)
end
end

describe 'nil persistence scenarios' do
it 'does not persist attributes that are not defined' do
klass.configure_client(client: stub_client)
Expand Down