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

add ActiveModel::Datastore.save_all feature #27

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,18 @@ class User
end
```

To improve performance, you can save many entities at once. (Batch operation)
Copy link
Member

@bmclean bmclean Apr 20, 2018

Choose a reason for hiding this comment

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

At this position in the README we have not yet mentioned the concept of parent/ancestor. Let's move this new section down after Datastore Consistency.


```ruby
group = Group.find(params[:group_id])
users = %w{ alice bob charlie }.map{ |name| User.new(name: name) }
if ActiveModel::Datastore.save_all(users, parent: group)
Copy link
Member

Choose a reason for hiding this comment

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

I believe passing in the group model object to parent will fail. It needs to be a Google::Cloud::Datastore::Key.

format.html { redirect_to action: :index, notice: "#{users.count} users were successfully created.' }
else
format.html { render :new }
end
```

## <a name="controller"></a>Controller Example

Now on to the controller! A scaffold generated controller works out of the box:
Expand Down
10 changes: 7 additions & 3 deletions lib/active_model/datastore.rb
Original file line number Diff line number Diff line change
Expand Up @@ -193,16 +193,20 @@ def destroy
end
end

def fill_id_from_entity(entity)
self.id = entity.key.id
self.parent_key_id = entity.key.parent.id if entity.key.parent.present?
entity
end

private

def save_entity(parent = nil)
return unless valid?
run_callbacks :save do
entity = build_entity(parent)
success = self.class.retry_on_exception? { CloudDatastore.dataset.save entity }
self.id = entity.key.id if success
self.parent_key_id = entity.key.parent.id if entity.key.parent.present?
success
success ? fill_id_from_entity(entity) : false
end
end

Expand Down
39 changes: 39 additions & 0 deletions lib/active_model/datastore/batch_operation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
##
# Batch operations
#
#
# Such batch calls are faster than making separate calls for each individual entity because they
# incur the overhead for only one service call.
#
# reference: https://cloud.google.com/datastore/docs/concepts/entities#batch_operations
#
# group = Group.find(params[:group_id])
# users = %w{ alice bob charlie }.map{ |name| User.new(name: name) }
# saved_users = ActiveModel::Datastore.save_all(users, parent: group)
#
module ActiveModel::Datastore
def self.save_all(entries, parent: nil)
return if entries.reject(&:valid?).present?
entries.each_slice(500).map do |sliced_entries|
entities = []
results = nil
fn = lambda do |n|
entry = sliced_entries[n]
entry.run_callbacks(:save) do
entities << entry.build_entity(parent)
if n + 1 < sliced_entries.count
# recursive call
fn.call(n + 1)
else
# batch insert
results = entry.class.retry_on_exception? { CloudDatastore.dataset.save entities }
end
sliced_entries[n].fill_id_from_entity(results&.fetch(n))
results.present?
end
end
fn.call(0)
sliced_entries
end.flatten
end
end
Copy link
Member

Choose a reason for hiding this comment

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

What would we expect this method to return? It is currently returning an array of model objects. Should it return an array of Google::Cloud::Datastore::Entity instead?

Also, is the results.present? line needed?

1 change: 1 addition & 0 deletions lib/activemodel/datastore.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@
require 'active_model/datastore/nested_attr'
require 'active_model/datastore/property_values'
require 'active_model/datastore/track_changes'
require 'active_model/datastore/batch_operation'
require 'active_model/datastore'
78 changes: 78 additions & 0 deletions test/cases/batch_operation_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
require 'test_helper'

class ActiveModel::BatchOperationTest < ActiveSupport::TestCase
def setup
super
@mock_models = %w[alice bob charlie].map do |name|
MockModel.new(name: name, parent_key_id: MOCK_PARENT_ID)
end
end

test 'batch save' 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 also add a test where the parent is explicitly passed in.

count = MockModel.count_test_entities
assert ActiveModel::Datastore.save_all(@mock_models)
assert_equal count + @mock_models.count, MockModel.count_test_entities
@mock_models.each do |mock_model|
assert_not_nil mock_model.id
key = CloudDatastore.dataset.key 'MockModel', mock_model.id
key.parent = CloudDatastore.dataset.key('ParentMockModel', MOCK_PARENT_ID)
entity = CloudDatastore.dataset.find key
assert_equal mock_model.id, entity.key.id
assert_equal 'MockModel', entity.key.kind
assert_equal 'ParentMockModel', entity.key.parent.kind
assert_equal MOCK_PARENT_ID, entity.key.parent.id
end
end

test 'before validation callback on batch save' do
@mock_models.each do |mock_model|
class << mock_model
before_validation { self.name = nil }
end
end
refute ActiveModel::Datastore.save_all(@mock_models)
@mock_models.each do |mock_model|
assert_nil mock_model.name
end
assert_equal 0, MockModel.count_test_entities
end

test 'after validation callback on batch save' do
@mock_models.each do |mock_model|
class << mock_model
after_validation { self.name = nil }
end
end
assert ActiveModel::Datastore.save_all(@mock_models)
@mock_models.each do |mock_model|
assert_nil mock_model.name
end
assert_equal @mock_models.count, MockModel.count_test_entities
end

test 'before save callback on batch save' do
@mock_models.each do |mock_model|
class << mock_model
before_save { self.name = name.upcase }
end
end
assert ActiveModel::Datastore.save_all(@mock_models)
@mock_models.each_with_index do |mock_model, index|
assert_equal mock_model.name, %w[alice bob charlie][index].upcase
assert_equal MockModel.all[index].name, %w[alice bob charlie][index].upcase
end
end

test 'after save callback on batch save' do
@mock_models.each do |mock_model|
class << mock_model
after_save { self.name = name.upcase }
end
end
assert ActiveModel::Datastore.save_all(@mock_models)
@mock_models.each_with_index do |mock_model, index|
assert_equal mock_model.name, %w[alice bob charlie][index].upcase
assert_equal MockModel.all[index].name, %w[alice bob charlie][index]
end
end
end
1 change: 1 addition & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
require 'active_model/datastore/nested_attr'
require 'active_model/datastore/property_values'
require 'active_model/datastore/track_changes'
require 'active_model/datastore/batch_operation'
require 'active_model/datastore'
require 'action_controller/metal/strong_parameters'

Expand Down