From cb42744dccde6910d2033eeaaee10540a68d50d2 Mon Sep 17 00:00:00 2001 From: "Marcos G. Zimmermann" Date: Thu, 4 Jul 2024 14:16:03 -0300 Subject: [PATCH 1/4] fix: delete doc from old route when it changes --- Gemfile.lock | 2 +- ci/Gemfile.rails-5.2.lock | 4 +- ci/Gemfile.rails-6.0.lock | 2 +- ci/Gemfile.rails-6.1.lock | 2 +- ci/Gemfile.rails-7.0.lock | 2 +- ci/Gemfile.rails-7.1.lock | 2 +- lib/esse/active_record/model.rb | 30 ++++++++++++- lib/esse/active_record/version.rb | 2 +- spec/esse/active_record/model_spec.rb | 65 +++++++++++++++++++++++++++ 9 files changed, 102 insertions(+), 9 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 0b5fa21..cc50584 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - esse-active_record (0.2.0) + esse-active_record (0.2.1) activerecord (>= 4.2, < 8) esse (>= 0.2.3) diff --git a/ci/Gemfile.rails-5.2.lock b/ci/Gemfile.rails-5.2.lock index c125964..1be0d7d 100644 --- a/ci/Gemfile.rails-5.2.lock +++ b/ci/Gemfile.rails-5.2.lock @@ -1,7 +1,7 @@ PATH remote: .. specs: - esse-active_record (0.2.0) + esse-active_record (0.2.1) activerecord (>= 4.2, < 8) esse (>= 0.2.3) @@ -157,4 +157,4 @@ DEPENDENCIES yard BUNDLED WITH - 2.3.21 + 2.3.22 diff --git a/ci/Gemfile.rails-6.0.lock b/ci/Gemfile.rails-6.0.lock index f0c2ec1..c1ddce3 100644 --- a/ci/Gemfile.rails-6.0.lock +++ b/ci/Gemfile.rails-6.0.lock @@ -1,7 +1,7 @@ PATH remote: .. specs: - esse-active_record (0.2.0) + esse-active_record (0.2.1) activerecord (>= 4.2, < 8) esse (>= 0.2.3) diff --git a/ci/Gemfile.rails-6.1.lock b/ci/Gemfile.rails-6.1.lock index 5de2a71..abb1757 100644 --- a/ci/Gemfile.rails-6.1.lock +++ b/ci/Gemfile.rails-6.1.lock @@ -1,7 +1,7 @@ PATH remote: .. specs: - esse-active_record (0.2.0) + esse-active_record (0.2.1) activerecord (>= 4.2, < 8) esse (>= 0.2.3) diff --git a/ci/Gemfile.rails-7.0.lock b/ci/Gemfile.rails-7.0.lock index f0c2ec1..c1ddce3 100644 --- a/ci/Gemfile.rails-7.0.lock +++ b/ci/Gemfile.rails-7.0.lock @@ -1,7 +1,7 @@ PATH remote: .. specs: - esse-active_record (0.2.0) + esse-active_record (0.2.1) activerecord (>= 4.2, < 8) esse (>= 0.2.3) diff --git a/ci/Gemfile.rails-7.1.lock b/ci/Gemfile.rails-7.1.lock index 71dee63..90103a0 100644 --- a/ci/Gemfile.rails-7.1.lock +++ b/ci/Gemfile.rails-7.1.lock @@ -1,7 +1,7 @@ PATH remote: .. specs: - esse-active_record (0.2.0) + esse-active_record (0.2.1) activerecord (>= 4.2, < 8) esse (>= 0.2.3) diff --git a/lib/esse/active_record/model.rb b/lib/esse/active_record/model.rb index 04c6ab5..eae263c 100644 --- a/lib/esse/active_record/model.rb +++ b/lib/esse/active_record/model.rb @@ -38,7 +38,7 @@ def index_callbacks(index_repo_name, on: %i[create update destroy], **options, & Esse::ActiveRecord::Hooks.register_model(self) if_enabled = -> { Esse::ActiveRecord::Hooks.enabled?(index_repo_name) && Esse::ActiveRecord::Hooks.enabled_for_model?(self.class, index_repo_name) } - (on & %i[create update]).each do |event| + (on & %i[create]).each do |event| after_commit(on: event, if: if_enabled) do opts = self.class.esse_index_repos.fetch(index_repo_name).fetch(operation_name) record = opts.fetch(:record) @@ -49,6 +49,34 @@ def index_callbacks(index_repo_name, on: %i[create update destroy], **options, & true end end + (on & %i[update]).each do |event| + after_commit(on: event, if: if_enabled) do + opts = self.class.esse_index_repos.fetch(index_repo_name).fetch(operation_name) + record = opts.fetch(:record) + record = instance_exec(&record) if record.respond_to?(:call) + repo = Esse::ActiveRecord::Hooks.resolve_index_repository(index_repo_name) + document = repo.serialize(record) + next true unless document + + repo.index.index(document, **opts[:options]) + next true unless document.routing + + prev_record = self.class.new(attributes.merge(previous_changes.transform_values(&:first))).tap(&:readonly!) + prev_document = repo.serialize(prev_record) + + next true unless prev_document + next true if [prev_document.id, prev_document.routing].include?(nil) + next true if prev_document.routing == document.routing + next true if prev_document.id != document.id + + begin + repo.index.delete(prev_document, **opts[:options]) + rescue Esse::Transport::NotFoundError + end + + true + end + end (on & %i[destroy]).each do |event| after_commit(on: event, if: if_enabled) do opts = self.class.esse_index_repos.fetch(index_repo_name).fetch(operation_name) diff --git a/lib/esse/active_record/version.rb b/lib/esse/active_record/version.rb index 0252e1a..e07e50e 100644 --- a/lib/esse/active_record/version.rb +++ b/lib/esse/active_record/version.rb @@ -2,6 +2,6 @@ module Esse module ActiveRecord - VERSION = '0.2.0' + VERSION = '0.2.1' end end diff --git a/spec/esse/active_record/model_spec.rb b/spec/esse/active_record/model_spec.rb index ea12758..f8c2be1 100644 --- a/spec/esse/active_record/model_spec.rb +++ b/spec/esse/active_record/model_spec.rb @@ -37,6 +37,7 @@ _id: county.id, name: county.name, type: 'county', + routing: (county.state_id || 1), }.tap do |doc| doc[:state] = { id: county.state.id, name: county.state.name } if county.state end @@ -172,6 +173,70 @@ model.touch end + context "when the document has a routing key" do + let(:model_class) do + Class.new(County) do + include Esse::ActiveRecord::Model + index_callbacks 'geographies:county', on: %i[update] + end + end + let(:il) { create_record(State, name: 'Illinois') } + let(:ny) { create_record(State, name: 'New York') } + let(:county) { create_record(model_class, name: 'Cook', state: il) } + + it 'indexes the document in new routing and deletes the document from previous routing' do + expect(GeographiesIndex).to receive(:index).and_call_original + expect(GeographiesIndex).to esse_receive_request(:index).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: ny.id, + body: {name: 'Cook', type: 'county', state: { id: ny.id, name: ny.name }}, + ).and_return(index_ok_response) + + expect(GeographiesIndex).to receive(:delete).and_call_original + expect(GeographiesIndex).to esse_receive_request(:delete).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: il.id, + ).and_return('result' => 'deleted') + + county.update(state: ny) + end + + it 'does not delete the document when the routing key is not changed' do + expect(GeographiesIndex).to receive(:index).and_call_original + expect(GeographiesIndex).to esse_receive_request(:index).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: il.id, + body: {name: 'Cook County', type: 'county', state: { id: il.id, name: il.name }}, + ).and_return(index_ok_response) + + expect(GeographiesIndex).not_to receive(:delete) + + county.update(name: 'Cook County') + end + + it 'does not raise error when the document does not exist' do + expect(GeographiesIndex).to receive(:index).and_call_original + expect(GeographiesIndex).to esse_receive_request(:index).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: ny.id, + body: {name: 'Cook', type: 'county', state: { id: ny.id, name: ny.name }}, + ).and_return(index_ok_response) + + expect(GeographiesIndex).to receive(:delete).and_call_original + expect(GeographiesIndex).to esse_receive_request(:delete).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: il.id, + ).and_raise_http_status(404, { 'error' => { 'type' => 'not_found' } }) + + county.update(state: ny) + end + end + it 'index the associated model using the block definition' do model_class = Class.new(County) do include Esse::ActiveRecord::Model From 4368a25be172953d0722fc14a7b95a077b922236 Mon Sep 17 00:00:00 2001 From: "Marcos G. Zimmermann" Date: Thu, 4 Jul 2024 14:20:58 -0300 Subject: [PATCH 2/4] chore: fix rubocop offenses --- .rubocop.yml | 3 - spec/esse/active_record/model_spec.rb | 128 +++++++++++++------------- 2 files changed, 64 insertions(+), 67 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index c8a7804..8a6e61c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -31,9 +31,6 @@ RSpec/MessageSpies: RSpec/FilePath: Enabled: false -RSpec/SpecFilePathFormat: - Enabled: false - Layout/SpaceInsideHashLiteralBraces: Enabled: false diff --git a/spec/esse/active_record/model_spec.rb b/spec/esse/active_record/model_spec.rb index f8c2be1..8a2920d 100644 --- a/spec/esse/active_record/model_spec.rb +++ b/spec/esse/active_record/model_spec.rb @@ -173,70 +173,6 @@ model.touch end - context "when the document has a routing key" do - let(:model_class) do - Class.new(County) do - include Esse::ActiveRecord::Model - index_callbacks 'geographies:county', on: %i[update] - end - end - let(:il) { create_record(State, name: 'Illinois') } - let(:ny) { create_record(State, name: 'New York') } - let(:county) { create_record(model_class, name: 'Cook', state: il) } - - it 'indexes the document in new routing and deletes the document from previous routing' do - expect(GeographiesIndex).to receive(:index).and_call_original - expect(GeographiesIndex).to esse_receive_request(:index).with( - id: county.id, - index: GeographiesIndex.index_name, - routing: ny.id, - body: {name: 'Cook', type: 'county', state: { id: ny.id, name: ny.name }}, - ).and_return(index_ok_response) - - expect(GeographiesIndex).to receive(:delete).and_call_original - expect(GeographiesIndex).to esse_receive_request(:delete).with( - id: county.id, - index: GeographiesIndex.index_name, - routing: il.id, - ).and_return('result' => 'deleted') - - county.update(state: ny) - end - - it 'does not delete the document when the routing key is not changed' do - expect(GeographiesIndex).to receive(:index).and_call_original - expect(GeographiesIndex).to esse_receive_request(:index).with( - id: county.id, - index: GeographiesIndex.index_name, - routing: il.id, - body: {name: 'Cook County', type: 'county', state: { id: il.id, name: il.name }}, - ).and_return(index_ok_response) - - expect(GeographiesIndex).not_to receive(:delete) - - county.update(name: 'Cook County') - end - - it 'does not raise error when the document does not exist' do - expect(GeographiesIndex).to receive(:index).and_call_original - expect(GeographiesIndex).to esse_receive_request(:index).with( - id: county.id, - index: GeographiesIndex.index_name, - routing: ny.id, - body: {name: 'Cook', type: 'county', state: { id: ny.id, name: ny.name }}, - ).and_return(index_ok_response) - - expect(GeographiesIndex).to receive(:delete).and_call_original - expect(GeographiesIndex).to esse_receive_request(:delete).with( - id: county.id, - index: GeographiesIndex.index_name, - routing: il.id, - ).and_raise_http_status(404, { 'error' => { 'type' => 'not_found' } }) - - county.update(state: ny) - end - end - it 'index the associated model using the block definition' do model_class = Class.new(County) do include Esse::ActiveRecord::Model @@ -302,6 +238,70 @@ end end + context 'when on :update with a the document that has a routing key' do + let(:model_class) do + Class.new(County) do + include Esse::ActiveRecord::Model + index_callbacks 'geographies:county', on: %i[update] + end + end + let(:il) { create_record(State, name: 'Illinois') } + let(:ny) { create_record(State, name: 'New York') } + let(:county) { create_record(model_class, name: 'Cook', state: il) } + + it 'indexes the document in new routing and deletes the document from previous routing' do + expect(GeographiesIndex).to receive(:index).and_call_original + expect(GeographiesIndex).to esse_receive_request(:index).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: ny.id, + body: {name: 'Cook', type: 'county', state: { id: ny.id, name: ny.name }}, + ).and_return(index_ok_response) + + expect(GeographiesIndex).to receive(:delete).and_call_original + expect(GeographiesIndex).to esse_receive_request(:delete).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: il.id, + ).and_return('result' => 'deleted') + + county.update(state: ny) + end + + it 'does not delete the document when the routing key is not changed' do + expect(GeographiesIndex).to receive(:index).and_call_original + expect(GeographiesIndex).to esse_receive_request(:index).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: il.id, + body: {name: 'Cook County', type: 'county', state: { id: il.id, name: il.name }}, + ).and_return(index_ok_response) + + expect(GeographiesIndex).not_to receive(:delete) + + county.update(name: 'Cook County') + end + + it 'does not raise error when the document does not exist' do + expect(GeographiesIndex).to receive(:index).and_call_original + expect(GeographiesIndex).to esse_receive_request(:index).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: ny.id, + body: {name: 'Cook', type: 'county', state: { id: ny.id, name: ny.name }}, + ).and_return(index_ok_response) + + expect(GeographiesIndex).to receive(:delete).and_call_original + expect(GeographiesIndex).to esse_receive_request(:delete).with( + id: county.id, + index: GeographiesIndex.index_name, + routing: il.id, + ).and_raise_http_status(404, { 'error' => { 'type' => 'not_found' } }) + + county.update(state: ny) + end + end + context 'when on destroy' do let(:delete_ok_response) { { 'result' => 'deleted' } } From 01a2bfeaf3c7c2bbe84ab67c1e09eb0381b42892 Mon Sep 17 00:00:00 2001 From: "Marcos G. Zimmermann" Date: Thu, 4 Jul 2024 14:21:54 -0300 Subject: [PATCH 3/4] chore: fix broken specs --- .rubocop.yml | 3 +++ spec/esse/active_record/model_spec.rb | 1 + 2 files changed, 4 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index 8a6e61c..9d09794 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -46,3 +46,6 @@ Style/TrailingCommaInHashLiteral: Style/StringLiterals: Enabled: true EnforcedStyle: single_quotes + +RSpec/MultipleMemoizedHelpers: + Max: 10 diff --git a/spec/esse/active_record/model_spec.rb b/spec/esse/active_record/model_spec.rb index 8a2920d..ed2d380 100644 --- a/spec/esse/active_record/model_spec.rb +++ b/spec/esse/active_record/model_spec.rb @@ -239,6 +239,7 @@ end context 'when on :update with a the document that has a routing key' do + let(:index_ok_response) { { 'result' => 'indexed' } } let(:model_class) do Class.new(County) do include Esse::ActiveRecord::Model From 109391c5e281df8671ffc8da2e38271bdd46af24 Mon Sep 17 00:00:00 2001 From: "Marcos G. Zimmermann" Date: Thu, 4 Jul 2024 14:24:56 -0300 Subject: [PATCH 4/4] chore: fix rubocop offenses --- spec/esse/active_record/model_spec.rb | 2 +- spec/esse/plugin/active_record/collection_definition_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/esse/active_record/model_spec.rb b/spec/esse/active_record/model_spec.rb index ed2d380..ca69b70 100644 --- a/spec/esse/active_record/model_spec.rb +++ b/spec/esse/active_record/model_spec.rb @@ -37,7 +37,7 @@ _id: county.id, name: county.name, type: 'county', - routing: (county.state_id || 1), + routing: county.state_id || 1, }.tap do |doc| doc[:state] = { id: county.state.id, name: county.state.name } if county.state end diff --git a/spec/esse/plugin/active_record/collection_definition_spec.rb b/spec/esse/plugin/active_record/collection_definition_spec.rb index ecfb02a..c601f2c 100644 --- a/spec/esse/plugin/active_record/collection_definition_spec.rb +++ b/spec/esse/plugin/active_record/collection_definition_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Esse::Plugins::ActiveRecord, '.collection' do +RSpec.describe Esse::Plugins::ActiveRecord, '.collection' do # rubocop:disable RSpec/SpecFilePathFormat describe 'index collection definition' do context 'when the type is a model class' do it 'define collection with only class name without raise an error' do