From e5da4d4cbac2d6a2c9517cc4417d5af66b3129a3 Mon Sep 17 00:00:00 2001 From: Joe Yates Date: Sun, 12 May 2024 15:17:25 +0200 Subject: [PATCH] Refresh flags inside the transaction --- lib/imap/backup/account/folder_backup.rb | 27 ++++++------- lib/imap/backup/serializer.rb | 15 +------- .../serializer/delayed_metadata_serializer.rb | 38 ++++++++++++++++--- lib/imap/backup/serializer/imap.rb | 26 +++++++++++-- spec/features/backup_spec.rb | 32 ++++++++++++---- spec/features/migrate_spec.rb | 1 + spec/features/support/20_test_email_server.rb | 12 +++--- spec/unit/serializer_spec.rb | 30 ++------------- 8 files changed, 105 insertions(+), 76 deletions(-) diff --git a/lib/imap/backup/account/folder_backup.rb b/lib/imap/backup/account/folder_backup.rb index 4e06d655..cb1d14b2 100644 --- a/lib/imap/backup/account/folder_backup.rb +++ b/lib/imap/backup/account/folder_backup.rb @@ -29,11 +29,13 @@ def run serializer.apply_uid_validity(folder.uid_validity) - download_serializer.transaction do + serializer.transaction do downloader.run + FlagRefresher.new(folder, serializer).run if account.mirror_mode || refresh end - - clean_up + # After the transaction the serializer will have any appended messages + # so we can check differences between the server and the local backup + LocalOnlyMessageDeleter.new(folder, raw_serializer).run if account.mirror_mode end private @@ -55,34 +57,29 @@ def folder_ok? true end - def clean_up - LocalOnlyMessageDeleter.new(folder, serializer).run if account.mirror_mode - FlagRefresher.new(folder, serializer).run if account.mirror_mode || refresh - end - def downloader @downloader ||= Downloader.new( folder, - download_serializer, + serializer, multi_fetch_size: account.multi_fetch_size, reset_seen_flags_after_fetch: account.reset_seen_flags_after_fetch ) end - def download_serializer - @download_serializer ||= + def serializer + @serializer ||= case account.download_strategy when "direct" - serializer + raw_serializer when "delay_metadata" - Serializer::DelayedMetadataSerializer.new(serializer: serializer) + Serializer::DelayedMetadataSerializer.new(serializer: raw_serializer) else raise "Unknown download strategy '#{account.download_strategy}'" end end - def serializer - @serializer ||= Serializer.new(account.local_path, folder.name) + def raw_serializer + @raw_serializer ||= Serializer.new(account.local_path, folder.name) end end end diff --git a/lib/imap/backup/serializer.rb b/lib/imap/backup/serializer.rb index 55a9b2df..d3aeb298 100644 --- a/lib/imap/backup/serializer.rb +++ b/lib/imap/backup/serializer.rb @@ -28,6 +28,7 @@ def self.folder_path_for(path:, folder:) extend Forwardable def_delegator :mbox, :pathname, :mbox_pathname + def_delegator :imap, :update # Get message metadata # @param uid [Integer] a message UID @@ -77,7 +78,7 @@ def initialize(path, folder) @validated = nil end - # Calls the supplied block. + # Calls the supplied block without implementing transactional behaviour. # This method is present so that this class implements the same # interface as {DelayedMetadataSerializer} # @param block [block] the block that is wrapped by the transaction @@ -177,18 +178,6 @@ def append(uid, message, flags) appender.append(uid: uid, message: message, flags: flags) end - # Updates a messages flags - # @param uid [Integer] the message's UID - # @param flags [Array] the flags to set on the message - # @return [void] - def update(uid, flags: nil) - message = imap.get(uid) - return if !message - - message.flags = flags if flags - imap.save - end - # Enumerates over a series of messages. # When called without a block, returns an Enumerator # @param required_uids [Array] the UIDs of the message to enumerate over diff --git a/lib/imap/backup/serializer/delayed_metadata_serializer.rb b/lib/imap/backup/serializer/delayed_metadata_serializer.rb index c643b0a9..06e9b814 100644 --- a/lib/imap/backup/serializer/delayed_metadata_serializer.rb +++ b/lib/imap/backup/serializer/delayed_metadata_serializer.rb @@ -10,7 +10,7 @@ module Imap; end module Imap::Backup class Serializer; end - # Wraps the Serializer, delaying metadata appends + # Wraps the Serializer, delaying metadata changes class Serializer::DelayedMetadataSerializer extend Forwardable @@ -31,7 +31,7 @@ def initialize(serializer:) def transaction(&block) tsx.fail_in_transaction!(:transaction, message: "nested transactions are not supported") - tsx.begin({metadata: []}) do + tsx.begin({appends: [], updates: []}) do mbox.transaction do block.call @@ -42,7 +42,21 @@ def transaction(&block) end end - # Appends a message to the mbox file and adds the metadata + # Sets the folder's UID validity via the serializer + # + # @param uid_validity [Integer] the UID validity to apply + # @raise [RuntimeError] if called inside a transaction + # @return [void] + def apply_uid_validity(uid_validity) + tsx.fail_in_transaction!( + :transaction, + message: "UID validity cannot be changed in a transaction" + ) + + serializer.apply_uid_validity(uid_validity) + end + + # Appends a message to the mbox file and adds the appended message's metadata # to the transaction # # @param uid [Integer] the UID of the message @@ -53,10 +67,21 @@ def append(uid, message, flags) tsx.fail_outside_transaction!(:append) mboxrd_message = Email::Mboxrd::Message.new(message) serialized = mboxrd_message.to_serialized - tsx.data[:metadata] << {uid: uid, length: serialized.bytesize, flags: flags} + tsx.data[:appends] << {uid: uid, length: serialized.bytesize, flags: flags} mbox.append(serialized) end + # Stores changes to a message's metadata for later update + # + # @param uid [Integer] the UID of the message + # @param length [Integer] the length of the message + # @param flags [Array] the flags for the message + # @return [void] + def update(uid, length: nil, flags: nil) + tsx.fail_outside_transaction!(:update) + tsx.data[:updates] << {uid: uid, length: length, flags: flags} + end + private attr_reader :serializer @@ -64,9 +89,12 @@ def append(uid, message, flags) def commit # rubocop:disable Lint/RescueException imap.transaction do - tsx.data[:metadata].each do |m| + tsx.data[:appends].each do |m| imap.append m[:uid], m[:length], flags: m[:flags] end + tsx.data[:updates].each do |m| + imap.update m[:uid], length: m[:length], flags: m[:flags] + end rescue Exception => e Logger.logger.error "#{self.class} handling #{e.class}" imap.rollback diff --git a/lib/imap/backup/serializer/imap.rb b/lib/imap/backup/serializer/imap.rb index bd9ee742..173f2343 100644 --- a/lib/imap/backup/serializer/imap.rb +++ b/lib/imap/backup/serializer/imap.rb @@ -97,11 +97,27 @@ def append(uid, length, flags: []) save end - # Get message metadata + # Updates a message's length and/or flags + # @param uid [Integer] the existing message's UID + # @param length [Integer] the length of the message (as stored on disk) + # @param flags [Array[Symbol]] the message's flags + # @raise [RuntimeError] if the UID does not exist + # @return [void] + def update(uid, length: nil, flags: nil) + index = messages.find_index { |m| m.uid == uid } + raise "UID #{uid} not found" if !index + + messages[index].length = length if length + messages[index].flags = flags if flags + save + end + + # Get a copy of message metadata # @param uid [Integer] a message UID # @return [Serializer::Message] def get(uid) - messages.find { |m| m.uid == uid } + message = messages.find { |m| m.uid == uid } + message&.dup end # Deletes the metadata file @@ -158,11 +174,15 @@ def uids messages.map(&:uid) end - # Update a message's metadata, replacing its UID + # Update a message's UID # @param old [Integer] the existing message UID # @param new [Integer] the new UID to apply to the message + # @raise [RuntimeError] if the new UID already exists # @return [void] def update_uid(old, new) + existing = messages.find_index { |m| m.uid == new } + raise "UID #{new} already exists" if existing + index = messages.find_index { |m| m.uid == old } return if index.nil? diff --git a/spec/features/backup_spec.rb b/spec/features/backup_spec.rb index cd30f81d..c50216ae 100644 --- a/spec/features/backup_spec.rb +++ b/spec/features/backup_spec.rb @@ -100,7 +100,6 @@ write_config backup.run test_server.set_flags folder, [1], [:Seen] - super() end it "updates flags" do @@ -182,13 +181,13 @@ context "in mirror mode" do let(:account_config) { super().merge(mirror_mode: true) } - let(:imap_path) { File.join(account_config[:local_path], "Foo.imap") } - let(:mbox_path) { File.join(account_config[:local_path], "Foo.mbox") } + let(:foo_imap_path) { File.join(account_config[:local_path], "Foo.imap") } + let(:foo_mbox_path) { File.join(account_config[:local_path], "Foo.mbox") } let!(:setup) do create_directory account_config[:local_path] - File.write(imap_path, "existing imap") - File.write(mbox_path, "existing mbox") + File.write(foo_imap_path, "existing imap") + File.write(foo_mbox_path, "existing mbox") super() end @@ -196,13 +195,32 @@ it "deletes .imap files" do run_command_and_stop command - expect(File.exist?(imap_path)).to be false + expect(File.exist?(foo_imap_path)).to be false end it "deletes .mbox files" do run_command_and_stop command - expect(File.exist?(mbox_path)).to be false + expect(File.exist?(foo_mbox_path)).to be false + end + end + + context "with messages that have been deleted from the server" do + let(:setup) do + super() + backup.run + end + let(:mbox_two) { to_mbox_entry(**message_two) } + let(:mbox_three) { to_mbox_entry(**message_three) } + + it "deletes messages from the local backup" do + expect(mbox_content(email, folder)).to eq(messages_as_mbox) + test_server.delete_email folder, 1 + test_server.send_email folder, **message_three + + run_command_and_stop command + + expect(mbox_content(email, folder)).to eq(mbox_two + mbox_three) end end end diff --git a/spec/features/migrate_spec.rb b/spec/features/migrate_spec.rb index 49443f1a..ce0b2d45 100644 --- a/spec/features/migrate_spec.rb +++ b/spec/features/migrate_spec.rb @@ -27,6 +27,7 @@ after do destination_server.delete_folder source_folder destination_server.disconnect + test_server.disconnect end context "when a config path is supplied" do diff --git a/spec/features/support/20_test_email_server.rb b/spec/features/support/20_test_email_server.rb index fdb5bd20..2983ecbd 100644 --- a/spec/features/support/20_test_email_server.rb +++ b/spec/features/support/20_test_email_server.rb @@ -34,14 +34,12 @@ def reconnect def disconnect return if !@imap - if !imap.disconnected? - begin - imap.logout - rescue EOFError - # ignore occasional error when closing connection - end - imap.disconnect + begin + imap.logout + rescue IOError + # ignore occasional error when closing connection end + imap.disconnect @imap = nil end diff --git a/spec/unit/serializer_spec.rb b/spec/unit/serializer_spec.rb index a3f984be..766349b5 100644 --- a/spec/unit/serializer_spec.rb +++ b/spec/unit/serializer_spec.rb @@ -107,7 +107,8 @@ module Imap::Backup rename: nil, save: nil, uid_validity: existing_uid_validity, - "uid_validity=": nil + "uid_validity=": nil, + update: nil ) end let(:mbox) do @@ -247,36 +248,13 @@ module Imap::Backup describe "#update" do let(:flags) { [:Foo] } - let(:message) { instance_double(Serializer::Message, "flags=": nil) } before do - allow(imap).to receive(:get) { message } - subject.update(33, flags: flags) end - it "updates the message flags" do - expect(message).to have_received(:flags=).with(flags) - end - - it "saves the .imap file" do - expect(imap).to have_received(:save) - end - - context "when no flags are supplied" do - let(:flags) { nil } - - it "does not update the message flags" do - expect(message).to_not have_received(:flags=) - end - end - - context "when the UID is not known" do - let(:message) { nil } - - it "does not save" do - expect(imap).to_not have_received(:save) - end + it "updates the .imap file" do + expect(imap).to have_received(:update).with(33, flags: flags) end end