From 924b4a304bb155a7789218cb8592dff69dee1438 Mon Sep 17 00:00:00 2001 From: Sebb Date: Tue, 24 Jan 2023 11:16:27 +0000 Subject: [PATCH] The == operator should be transitive This fixes #1529 --- CHANGELOG.rdoc | 1 + lib/mail/message.rb | 36 +++++++++++------------------------- spec/mail/message_spec.rb | 22 +++++++++++++--------- 3 files changed, 25 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index 4727518dd..b80c9dc9a 100644 --- a/CHANGELOG.rdoc +++ b/CHANGELOG.rdoc @@ -24,6 +24,7 @@ Bug Fixes: * Regression: Preserve message-level charset when adding parts (related to Rails ActionMailer) @shields * Regression: Adding a part should not reset the mail's charset to nil @railsbob +* == operator is now transitive; a==b and b==c implies a==c Performance: diff --git a/lib/mail/message.rb b/lib/mail/message.rb index e416df429..ee6f41710 100644 --- a/lib/mail/message.rb +++ b/lib/mail/message.rb @@ -339,37 +339,29 @@ def <=>(other) end end - # Two emails are the same if they have the same fields and body contents. One - # gotcha here is that Mail will insert Message-IDs when calling encoded, so doing - # mail1.encoded == mail2.encoded is most probably not going to return what you think - # as the assigned Message-IDs by Mail (if not already defined as the same) will ensure - # that the two objects are unique, and this comparison will ALWAYS return false. + # Two emails are the same if they have the same fields and body contents. # - # So the == operator has been defined like so: Two messages are the same if they have - # the same content, ignoring the Message-ID field, unless BOTH emails have a defined and - # different Message-ID value, then they are false. + # Note that Mail will provide Message-ID and Date (if necessary) when calling encoded, so + # mail1 == mail2 does not imply mail1.encoded == mail2.encoded + # unless both Date and Message-Id have been provided. # - # Note that Mail creates Date and Mime-Type fields if they don't exist. - # The Date field is derived from the current time, so this needs to be allowed for in comparisons. - # (Mime-type does not depend on dynamic data, so cannot affect equality) - # - # So, in practice the == operator works like this: + # The == operator works like this: # # m1 = Mail.new("Subject: Hello\r\n\r\nHello") # m2 = Mail.new("Subject: Hello\r\n\r\nHello") - # m1 == m2 #=> true + # m1 == m2 #=> true; however encoded versions will not be equal # # m1 = Mail.new("Subject: Hello\r\n\r\nHello") # m2 = Mail.new("Message-ID: <1234@test>\r\nSubject: Hello\r\n\r\nHello") - # m1 == m2 #=> true + # m1 == m2 #=> false # # m1 = Mail.new("Message-ID: <1234@test>\r\nSubject: Hello\r\n\r\nHello") # m2 = Mail.new("Subject: Hello\r\n\r\nHello") - # m1 == m2 #=> true + # m1 == m2 #=> false # # m1 = Mail.new("Message-ID: <1234@test>\r\nSubject: Hello\r\n\r\nHello") # m2 = Mail.new("Message-ID: <1234@test>\r\nSubject: Hello\r\n\r\nHello") - # m1 == m2 #=> true + # m1 == m2 #=> true; however encoded versions will not be equal if the time stamp changes # # m1 = Mail.new("Message-ID: <1234@test>\r\nSubject: Hello\r\n\r\nHello") # m2 = Mail.new("Message-ID: \r\nSubject: Hello\r\n\r\nHello") @@ -378,14 +370,8 @@ def ==(other) # TODO could be more efficient return false unless other.respond_to?(:encoded) stamp = Mail::CommonDateField.normalize_datetime('') - # Note: must always dup the inputs so they are not altered by encoded - if self.message_id && other.message_id - dup.tap { |m| m.date ||= stamp }.encoded == - other.dup.tap { |m| m.date ||= stamp }.encoded - else - dup.tap { |m| m.message_id = ''; m.date ||= stamp }.encoded == - other.dup.tap { |m| m.message_id = ''; m.date ||= stamp }.encoded - end + dup.tap { |m| m.message_id ||= ''; m.date ||= stamp }.encoded == + other.dup.tap { |m| m.message_id ||= ''; m.date ||= stamp }.encoded end def initialize_copy(original) diff --git a/spec/mail/message_spec.rb b/spec/mail/message_spec.rb index 0a22aaeb7..cb1ca7acb 100644 --- a/spec/mail/message_spec.rb +++ b/spec/mail/message_spec.rb @@ -1697,11 +1697,6 @@ def with_encoder(encoder) describe "helper methods" do describe "==" do - before(:each) do - # Ensure specs don't randomly fail due to messages being generated 1 second apart - time = DateTime.now - expect(DateTime).to receive(:now).at_least(:once).and_return(time) - end it "should be implemented" do expect { Mail.new == Mail.new }.not_to raise_error @@ -1716,19 +1711,19 @@ def with_encoder(encoder) expect(m2[:message_id]).to be_nil end - it "should ignore the message id value if self has a nil message id" do + it "should not ignore the message id value if self has a nil message id" do m1 = Mail.new("To: mikel@test.lindsaar.net\r\nSubject: Yo!\r\n\r\nHello there") m2 = Mail.new("To: mikel@test.lindsaar.net\r\nMessage-ID: <1234@test.lindsaar.net>\r\nSubject: Yo!\r\n\r\nHello there") - expect(m1).to eq m2 + expect(m1).not_to eq m2 # confirm there are no side-effects in the comparison expect(m1[:message_id]).to be_nil expect(m2[:message_id].value).to eq '<1234@test.lindsaar.net>' end - it "should ignore the message id value if other has a nil message id" do + it "should not ignore the message id value if other has a nil message id" do m1 = Mail.new("To: mikel@test.lindsaar.net\r\nMessage-ID: <1234@test.lindsaar.net>\r\nSubject: Yo!\r\n\r\nHello there") m2 = Mail.new("To: mikel@test.lindsaar.net\r\nSubject: Yo!\r\n\r\nHello there") - expect(m1).to eq m2 + expect(m1).not_to eq m2 # confirm there are no side-effects in the comparison expect(m1[:message_id].value).to eq '<1234@test.lindsaar.net>' expect(m2[:message_id]).to be_nil @@ -1742,6 +1737,15 @@ def with_encoder(encoder) expect(m1.message_id).to eq '4321@test.lindsaar.net' expect(m2.message_id).to eq '1234@test.lindsaar.net' end + + it "should be transitive" do + m1 = Mail.new("To: mikel@test.lindsaar.net\r\nMessage-ID: <1@test.lindsaar.net>\r\nSubject: Yo!\r\n\r\nHello") + m2 = Mail.new("To: mikel@test.lindsaar.net\r\nMessage-ID: <2@test.lindsaar.net>\r\nSubject: Yo!\r\n\r\nHello") + m3 = Mail.new("To: mikel@test.lindsaar.net\r\nSubject: Yo!\r\n\r\nHello") + if m1 == m3 && m2 == m3 + expect(m1).to eq m2 # Check transitivity + end + end end it "should implement the spaceship operator on the date field" do