diff --git a/db/migrate/create_outboxer_messages.rb b/db/migrate/create_outboxer_messages.rb index 3ba45ab1..be4ad8ba 100644 --- a/db/migrate/create_outboxer_messages.rb +++ b/db/migrate/create_outboxer_messages.rb @@ -1,6 +1,8 @@ class CreateOutboxerMessages < ActiveRecord::Migration[6.1] def up create_table :outboxer_messages do |t| + t.integer :lock_version, null: false, default: 0 + t.string :status, limit: 255, null: false t.string :messageable_id, limit: 255, null: false diff --git a/lib/outboxer/message.rb b/lib/outboxer/message.rb index 958b0aa7..601a8f83 100644 --- a/lib/outboxer/message.rb +++ b/lib/outboxer/message.rb @@ -119,10 +119,7 @@ def queue(messageable: nil, messageable_type: nil, messageable_id: nil, { id: message.id, - status: message.status, - messageable_type: message.messageable_type, - messageable_id: message.messageable_id, - updated_at: message.updated_at + lock_version: message.lock_version } end rescue MissingPartition @@ -203,7 +200,9 @@ def publish(logger: nil, time: ::Time) begin yield message rescue StandardError => error - publishing_failed(id: message[:id], error: error, time: time) + publishing_failed( + id: message[:id], lock_version: message[:lock_version], + error: error, time: time) logger&.error( "Outboxer message publishing failed id=#{message[:id]} " \ @@ -211,7 +210,8 @@ def publish(logger: nil, time: ::Time) "#{error.class}: #{error.message}\n" \ "#{error.backtrace.join("\n")}") rescue Exception => error - publishing_failed(id: message[:id], error: error, time: time) + publishing_failed( + id: message[:id], lock_version: message[:lock_version], error: error, time: time) logger&.fatal( "Outboxer message publishing failed id=#{message[:id]} " \ @@ -221,7 +221,7 @@ def publish(logger: nil, time: ::Time) raise else - published(id: message[:id], time: time) + published(id: message[:id], lock_version: message[:lock_version], time: time) logger&.info( "Outboxer message published id=#{message[:id]} " \ @@ -254,7 +254,7 @@ def publishing(time: ::Time) ActiveRecord::Base.connection_pool.with_connection do ActiveRecord::Base.transaction do message = Models::Message - .select(:id, :messageable_type, :messageable_id) + .select(:id, :lock_version, :messageable_type, :messageable_id) .where(status: Status::QUEUED) .order(:id) .limit(1) @@ -265,6 +265,7 @@ def publishing(time: ::Time) nil else message.update!( + lock_version: message.lock_version, status: Status::PUBLISHING, updated_at: current_utc_time, publishing_at: current_utc_time) @@ -284,9 +285,10 @@ def publishing(time: ::Time) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) { - id: message[:id], - messageable_type: message[:messageable_type], - messageable_id: message[:messageable_id] + id: message.id, + lock_version: message.lock_version, + messageable_type: message.messageable_type, + messageable_id: message.messageable_id } end end @@ -306,7 +308,7 @@ def publishing(time: ::Time) # @note Executes within a transaction using a `SELECT ... FOR UPDATE` lock. # @example # Outboxer::Message.published(id: message[:id], time: Time) - def published(id:, time: ::Time) + def published(id:, lock_version:, time: ::Time) current_utc_time = time.now.utc ActiveRecord::Base.connection_pool.with_connection do @@ -317,6 +319,8 @@ def published(id:, time: ::Time) raise Error, "Status must be publishing" end + message.update!(lock_version: lock_version, updated_at: Time.now.utc) + message.exceptions.each { |exception| exception.frames.each(&:delete) } message.exceptions.delete_all message.delete @@ -335,7 +339,7 @@ def published(id:, time: ::Time) .where(status: Status::PUBLISHED, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) - nil + {} end end end @@ -354,7 +358,7 @@ def published(id:, time: ::Time) # @note Runs inside a transaction and uses `SELECT ... FOR UPDATE` locking. # @example # Outboxer::Message.publishing_failed(id: message[:id], time: Time) - def publishing_failed(id:, error: nil, time: ::Time) + def publishing_failed(id:, lock_version:, error: nil, time: ::Time) current_utc_time = time.now.utc ActiveRecord::Base.connection_pool.with_connection do @@ -366,6 +370,7 @@ def publishing_failed(id:, error: nil, time: ::Time) end message.update!( + lock_version: lock_version, status: Status::FAILED, updated_at: current_utc_time, failed_at: current_utc_time) @@ -392,7 +397,9 @@ def publishing_failed(id:, error: nil, time: ::Time) .where(status: Status::FAILED, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) - nil + { + lock_version: message.lock_version + } end end end @@ -400,6 +407,7 @@ def publishing_failed(id:, error: nil, time: ::Time) # Serializes message attributes into a hash. # # @param id [Integer] message ID. + # @param lock_version [Integer] message lock_version. # @param status [String] message status. # @param messageable_type [String] type of the messageable entity. # @param messageable_id [String] ID of the messageable entity. @@ -411,11 +419,12 @@ def publishing_failed(id:, error: nil, time: ::Time) # @param publisher_id [Integer, nil] optional publisher ID. # @param publisher_name [String, nil] optional publisher name. # @return [Hash] serialized message details. - def serialize(id:, status:, messageable_type:, messageable_id:, updated_at:, + def serialize(id:, lock_version:, status:, messageable_type:, messageable_id:, updated_at:, queued_at: nil, publishing_at: nil, published_at: nil, failed_at: nil, publisher_id: nil, publisher_name: nil) { id: id, + lock_version: lock_version, status: status, messageable_type: messageable_type, messageable_id: messageable_id, @@ -445,6 +454,7 @@ def find_by_id(id:) { id: message.id, + lock_version: message.lock_version, status: message.status, messageable_type: message.messageable_type, messageable_id: message.messageable_id, @@ -479,12 +489,13 @@ def find_by_id(id:) # Deletes a message by ID. # @param id [Integer] the ID of the message to delete. # @return [Hash] details of the deleted message. - def delete(id:, time: ::Time) + def delete(id:, lock_version:, time: ::Time) current_utc_time = time.now.utc ActiveRecord::Base.connection_pool.with_connection do ActiveRecord::Base.transaction do message = Models::Message.includes(exceptions: :frames).lock.find_by!(id: id) + message.update!(lock_version: lock_version, updated_at: current_utc_time) message.exceptions.each { |exception| exception.frames.each(&:delete) } message.exceptions.delete_all @@ -516,7 +527,7 @@ def can_requeue?(status:) # @param publisher_name [String, nil] the name of the publisher. # @param time [Time] current time context used to update timestamps. # @return [Hash] updated message details. - def requeue(id:, publisher_id: nil, publisher_name: nil, + def requeue(id:, lock_version:, publisher_id: nil, publisher_name: nil, time: ::Time) current_utc_time = time.now.utc @@ -526,11 +537,10 @@ def requeue(id:, publisher_id: nil, publisher_name: nil, partition = calculate_partition(id: message.id) - Models::MessageCount - .where(status: message.status, partition: partition) - .update_all(["value = value - ?, updated_at = ?", 1, current_utc_time]) + original_status = message.status message.update!( + lock_version: lock_version, status: Message::Status::QUEUED, updated_at: current_utc_time, queued_at: current_utc_time, @@ -541,11 +551,15 @@ def requeue(id:, publisher_id: nil, publisher_name: nil, publisher_name: publisher_name) Models::MessageCount - .where(status: Status::QUEUED, partition: partition) + .where(status: original_status, partition: partition) + .update_all(["value = value - ?, updated_at = ?", 1, current_utc_time]) + + Models::MessageCount + .where(status: message.status, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) Models::MessageTotal - .where(status: Status::QUEUED, partition: partition) + .where(status: message.status, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) { id: id } @@ -631,6 +645,7 @@ def list(status: LIST_STATUS_DEFAULT, messages: paginated_messages.map do |message| { id: message.id, + lock_version: message.lock_version, status: message.status.to_sym, messageable_type: message.messageable_type, messageable_id: message.messageable_id, diff --git a/lib/outboxer/web.rb b/lib/outboxer/web.rb index c0394447..005edd67 100755 --- a/lib/outboxer/web.rb +++ b/lib/outboxer/web.rb @@ -494,56 +494,68 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end post "/messages/update" do - ids = params[:selected_ids].map(&:to_i) flash = {} + selected_messages = params.fetch("selected_messages", []).map do |pair| + id, lock_version = pair.split(":").map(&:to_i) + { id: id, lock_version: lock_version } + end + case params[:action] when "requeue_by_ids" - requeued_count = 0 - failed_count = 0 + requeued_message_count = 0 + failed_message_count = 0 - ids.each do |id| - Outboxer::Message.requeue(id: id) - requeued_count += 1 + selected_messages.each do |selected_message| + Outboxer::Message.requeue( + id: selected_message[:id], + lock_version: selected_message[:lock_version] + ) + requeued_message_count += 1 rescue StandardError => error settings.logger.error( - "[Outboxer::Web] Failed to requeue message id=#{id}\n" \ + "[Outboxer::Web] Failed to requeue message id=#{selected_message[:id]}\n" \ "error_class=#{error.class}\n" \ "error_message=#{error.message.inspect}" ) - failed_count += 1 + failed_message_count += 1 end - if requeued_count.positive? - flash[:success] = "Requeued #{pluralise(requeued_count, "message")}" + if requeued_message_count.positive? + flash[:success] = "Requeued #{pluralise(requeued_message_count, "message")}" end - if failed_count.positive? - flash[:danger] = "Requeue failed for #{pluralise(failed_count, "message")}" + if failed_message_count.positive? + flash[:danger] = "Requeue failed for #{pluralise(failed_message_count, "message")}" end + when "delete_by_ids" - deleted_count = 0 - failed_count = 0 + deleted_message_count = 0 + failed_message_count = 0 - ids.each do |id| - Outboxer::Message.delete(id: id) - deleted_count += 1 + selected_messages.each do |selected_message| + Outboxer::Message.delete( + id: selected_message[:id], + lock_version: selected_message[:lock_version] + ) + deleted_message_count += 1 rescue StandardError => error settings.logger.error( - "[Outboxer::Web] Failed to delete message id=#{id}\n" \ + "[Outboxer::Web] Failed to delete message id=#{selected_message[:id]}\n" \ "error_class=#{error.class}\n" \ "error_message=#{error.message.inspect}" ) - failed_count += 1 + failed_message_count += 1 end - if deleted_count.positive? - flash[:success] = "Deleted #{pluralise(deleted_count, "message")}" + if deleted_message_count.positive? + flash[:success] = "Deleted #{pluralise(deleted_message_count, "message")}" end - if failed_count.positive? - flash[:danger] = "Delete failed for #{pluralise(failed_count, "message")}" + if failed_message_count.positive? + flash[:danger] = "Delete failed for #{pluralise(failed_message_count, "message")}" end + else raise "Unknown action: #{params[:action]}" end @@ -656,7 +668,7 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end post "/message/:id/requeue" do - Message.requeue(id: params[:id]) + Message.requeue(id: params[:id], lock_version: params[:lock_version]) denormalised_query_params = denormalise_query_params( status: params[:status], @@ -679,7 +691,7 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end post "/message/:id/delete" do - Message.delete(id: params[:id]) + Message.delete(id: params[:id], lock_version: params[:lock_version]) denormalised_query_params = denormalise_query_params( status: params[:status], diff --git a/lib/outboxer/web/views/message.erb b/lib/outboxer/web/views/message.erb index 055d3c1d..119bf69a 100644 --- a/lib/outboxer/web/views/message.erb +++ b/lib/outboxer/web/views/message.erb @@ -4,25 +4,33 @@

Message <%= message[:id] %>

-
" method="post" style="display: inline;"> - <% normalised_query_params.each do |key, param| %> - - <% end %> - + " method="post" + style="display: inline;"> + <% normalised_query_params.each do |key, param| %> + + <% end %> + + +
+ -
" method="post" style="display: inline;" + " method="post" + style="display: inline;" onsubmit="return confirm('Are you sure you want to delete this message?');"> + <% normalised_query_params.each do |key, param| %> + + <% end %> + - <% normalised_query_params.each do |key, param| %> - - <% end %> - +
diff --git a/lib/outboxer/web/views/messages.erb b/lib/outboxer/web/views/messages.erb index 88cb3060..32581d73 100644 --- a/lib/outboxer/web/views/messages.erb +++ b/lib/outboxer/web/views/messages.erb @@ -14,11 +14,12 @@
" method="post" class="me-2"> <% normalised_query_params.each do |key, param| %> - + <% end %> - <% messages.each do |message| %> - + <% messages.each_with_index do |message, i| %> + "> <% end %>
+
<% if Outboxer::Message.can_requeue?(status: denormalised_query_params[:status]) %> @@ -48,8 +51,9 @@ <% end %> - <% messages.each do |message| %> - + <% messages.each_with_index do |message, i| %> + "> <% end %>
@@ -75,29 +80,29 @@
" method="post"> -
-
-
-
- <% normalised_query_params.each do |key, param| %> - - <% end %> +
+
+
+
+ <% normalised_query_params.each do |key, param| %> + + <% end %> - - + - - -
-
+
+
+
diff --git a/spec/lib/outboxer/message/delete_spec.rb b/spec/lib/outboxer/message/delete_spec.rb index 7b3938eb..bf3613a1 100644 --- a/spec/lib/outboxer/message/delete_spec.rb +++ b/spec/lib/outboxer/message/delete_spec.rb @@ -13,7 +13,7 @@ module Outboxer let!(:exception) { create(:outboxer_exception, message: message) } let!(:frame) { create(:outboxer_frame, exception: exception) } - let!(:result) { Message.delete(id: message.id) } + let!(:result) { Message.delete(id: message.id, lock_version: message.lock_version) } it "deletes the message" do expect(Models::Message).not_to exist(message.id) @@ -42,7 +42,7 @@ module Outboxer let!(:exception) { create(:outboxer_exception, message: message) } let!(:frame) { create(:outboxer_frame, exception: exception) } - let!(:result) { Message.delete(id: message.id) } + let!(:result) { Message.delete(id: message.id, lock_version: message.lock_version) } it "deletes the message" do expect(Models::Message).not_to exist(message.id) diff --git a/spec/lib/outboxer/message/list/filter_spec.rb b/spec/lib/outboxer/message/list/filter_spec.rb index 2debbdca..79008b6a 100644 --- a/spec/lib/outboxer/message/list/filter_spec.rb +++ b/spec/lib/outboxer/message/list/filter_spec.rb @@ -51,6 +51,7 @@ module Outboxer messages: [ { id: message_1.id, + lock_version: message_1.lock_version, status: message_1.status.to_sym, messageable_type: message_1.messageable_type, messageable_id: message_1.messageable_id, @@ -65,6 +66,7 @@ module Outboxer }, { id: message_4.id, + lock_version: message_4.lock_version, status: message_4.status.to_sym, messageable_type: message_4.messageable_type, messageable_id: message_4.messageable_id, @@ -89,6 +91,7 @@ module Outboxer messages: [ { id: message_5.id, + lock_version: message_5.lock_version, status: message_5.status.to_sym, messageable_type: message_5.messageable_type, messageable_id: message_5.messageable_id, @@ -113,6 +116,7 @@ module Outboxer messages: [ { id: message_2.id, + lock_version: message_2.lock_version, status: message_2.status.to_sym, messageable_type: message_2.messageable_type, messageable_id: message_2.messageable_id, diff --git a/spec/lib/outboxer/message/list/no_arguments_spec.rb b/spec/lib/outboxer/message/list/no_arguments_spec.rb index 7dd90b61..4ba176b3 100644 --- a/spec/lib/outboxer/message/list/no_arguments_spec.rb +++ b/spec/lib/outboxer/message/list/no_arguments_spec.rb @@ -61,6 +61,7 @@ module Outboxer messages: [ { id: message_1.id, + lock_version: message_1.lock_version, status: message_1.status.to_sym, messageable_type: message_1.messageable_type, messageable_id: message_1.messageable_id, @@ -75,6 +76,7 @@ module Outboxer }, { id: message_2.id, + lock_version: message_2.lock_version, status: message_2.status.to_sym, messageable_type: message_2.messageable_type, messageable_id: message_2.messageable_id, @@ -89,6 +91,7 @@ module Outboxer }, { id: message_3.id, + lock_version: message_3.lock_version, status: message_3.status.to_sym, messageable_type: message_3.messageable_type, messageable_id: message_3.messageable_id, @@ -103,6 +106,7 @@ module Outboxer }, { id: message_4.id, + lock_version: message_4.lock_version, status: message_4.status.to_sym, messageable_type: message_4.messageable_type, messageable_id: message_4.messageable_id, @@ -117,6 +121,7 @@ module Outboxer }, { id: message_5.id, + lock_version: message_5.lock_version, status: message_5.status.to_sym, messageable_type: message_5.messageable_type, messageable_id: message_5.messageable_id, diff --git a/spec/lib/outboxer/message/publish_spec.rb b/spec/lib/outboxer/message/publish_spec.rb index 66074ef3..0c45b6d6 100644 --- a/spec/lib/outboxer/message/publish_spec.rb +++ b/spec/lib/outboxer/message/publish_spec.rb @@ -23,11 +23,10 @@ module Outboxer end expect(yielded_id).to eq(queued_message.id) - expect(result).to eq({ + expect(result).to include( id: queued_message.id, messageable_type: queued_message.messageable_type, - messageable_id: queued_message.messageable_id - }) + messageable_id: queued_message.messageable_id) expect(Models::Message.published.count).to eql(0) end @@ -37,7 +36,7 @@ module Outboxer raise StandardError, "temporary failure" end - expect(result).to eq({ + expect(result).to include({ id: queued_message.id, messageable_type: queued_message.messageable_type, messageable_id: queued_message.messageable_id diff --git a/spec/lib/outboxer/message/requeue_spec.rb b/spec/lib/outboxer/message/requeue_spec.rb index 29bb4d8c..9cea628c 100644 --- a/spec/lib/outboxer/message/requeue_spec.rb +++ b/spec/lib/outboxer/message/requeue_spec.rb @@ -6,7 +6,9 @@ module Outboxer context "when failed message" do let!(:failed_message) { create(:outboxer_message, :failed) } - let!(:queued_message) { Message.requeue(id: failed_message.id) } + let!(:queued_message) do + Message.requeue(id: failed_message.id, lock_version: failed_message.lock_version) + end it "returns queued message" do expect(queued_message[:id]).to eq(failed_message.id) @@ -24,7 +26,7 @@ module Outboxer let!(:updated_at) { queued_message.updated_at } it "modifies updated_at" do - Message.requeue(id: queued_message.id) + Message.requeue(id: queued_message.id, lock_version: queued_message.lock_version) queued_message.reload diff --git a/spec/lib/outboxer/message/serialize_spec.rb b/spec/lib/outboxer/message/serialize_spec.rb index 3777c5d5..14da995d 100644 --- a/spec/lib/outboxer/message/serialize_spec.rb +++ b/spec/lib/outboxer/message/serialize_spec.rb @@ -7,6 +7,7 @@ module Outboxer expect( Message.serialize( id: 1, + lock_version: 2, status: "publishing", messageable_type: "Invoice", messageable_id: "INV-001", @@ -20,6 +21,7 @@ module Outboxer ).to eq( { id: 1, + lock_version: 2, status: "publishing", messageable_type: "Invoice", messageable_id: "INV-001", diff --git a/spec/lib/outboxer/web/message/delete_spec.rb b/spec/lib/outboxer/web/message/delete_spec.rb index cd4951cc..0ebf7a23 100644 --- a/spec/lib/outboxer/web/message/delete_spec.rb +++ b/spec/lib/outboxer/web/message/delete_spec.rb @@ -15,7 +15,7 @@ def app before do header "Host", "localhost" - post "/message/#{message[:id]}/delete" + post "/message/#{message[:id]}/delete", { lock_version: message[:lock_version] } follow_redirect! end diff --git a/spec/lib/outboxer/web/message/requeue_spec.rb b/spec/lib/outboxer/web/message/requeue_spec.rb index dd51c574..3d932d1c 100644 --- a/spec/lib/outboxer/web/message/requeue_spec.rb +++ b/spec/lib/outboxer/web/message/requeue_spec.rb @@ -15,7 +15,7 @@ def app before do header "Host", "localhost" - post "/message/#{message[:id]}/requeue" + post "/message/#{message[:id]}/requeue", { lock_version: message[:lock_version] } follow_redirect! end diff --git a/spec/lib/outboxer/web/messages/update_spec.rb b/spec/lib/outboxer/web/messages/update_spec.rb index 715bc669..af5c305c 100644 --- a/spec/lib/outboxer/web/messages/update_spec.rb +++ b/spec/lib/outboxer/web/messages/update_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" - require_relative "../../../../../lib/outboxer/web" RSpec.describe "POST /messages/update", type: :request do @@ -24,16 +23,16 @@ def app before do message_2.update!(status: Outboxer::Message::Status::PUBLISHING) message_3.update!(status: Outboxer::Message::Status::FAILED) - header "Host", "localhost" end context "when action is requeue_by_ids" do - let(:ids) { [message_2.id, message_3.id] } - before do post "/messages/update", { - selected_ids: ids, + selected_messages: [ + "#{message_2.id}:#{message_2.lock_version}", + "#{message_3.id}:#{message_3.lock_version}" + ], action: "requeue_by_ids", status: :failed, page: 1, @@ -42,12 +41,11 @@ def app order: :desc, time_zone: "Australia/Sydney" } - follow_redirect! end it "queues selected messages" do - expect(Outboxer::Models::Message.queued.pluck(:id)).to eq([ + expect(Outboxer::Models::Message.queued.pluck(:id)).to match_array([ message_1.id, message_2.id, message_3.id ]) end @@ -55,19 +53,20 @@ def app it "redirects with flash message" do expect(last_response).to be_ok expect(last_request.url).to include( - "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney") - + "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney" + ) expected_flash = URI.encode_www_form_component("success:Requeued 2 messages") expect(last_request.url).to include("flash=#{expected_flash}") end end context "when action is delete_by_ids" do - let(:ids) { [message_2.id, message_3.id] } - before do post "/messages/update", { - selected_ids: ids, + selected_messages: [ + "#{message_2.id}:#{message_2.lock_version}", + "#{message_3.id}:#{message_3.lock_version}" + ], action: "delete_by_ids", status: :failed, page: 1, @@ -76,7 +75,6 @@ def app order: :desc, time_zone: "Australia/Sydney" } - follow_redirect! end @@ -87,18 +85,20 @@ def app it "redirects with flash message" do expect(last_response).to be_ok expect(last_request.url).to include( - "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney") + "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney" + ) expected_flash = URI.encode_www_form_component("success:Deleted 2 messages") expect(last_request.url).to include("flash=#{expected_flash}") end end context "with invalid action" do - let(:ids) { [message_2.id, message_3.id] } - before do post "/messages/update", { - selected_ids: ids, + selected_messages: [ + "#{message_2.id}:#{message_2.lock_version}", + "#{message_3.id}:#{message_3.lock_version}" + ], action: "invalid", status: :failed, page: 1,