Defect #34999

The result of Attachment.latest_attach is unstable if attachments have the same timestamp

Added by Pavel Rosický 6 months ago. Updated 6 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Attachments
Target version:4.2.1
Resolution:Fixed Affected version:4.2.0

Description

refs #27780

test_latest_attach_should_support_unicode_case_folding

Failure:
AttachmentTest#test_latest_attach_should_support_unicode_case_folding [c:/redmine/test/unit/attachment_test.rb:488]:
Expected: #<Attachment id: 1990, container_id: nil, container_type: nil, filename: "ā.txt", disk_filename: "210401211700_8b8e5a4d7605340440caaf5211ac5124.TXT", filesize: 32, content_type: "text/plain", digest: "c62e4615bd39e222572f3a1bf7c2132ea1e65b17ec805047bd...", downloads: 0, author_id: 1, created_on: "2021-03-31 19:17:00.000000000 +0000", description: nil, disk_directory: "2021/03">
  Actual: #<Attachment id: 1989, container_id: nil, container_type: nil, filename: "Ā.TXT", disk_filename: "210401211700_8b8e5a4d7605340440caaf5211ac5124.TXT", filesize: 32, content_type: "text/plain", digest: "c62e4615bd39e222572f3a1bf7c2132ea1e65b17ec805047bd...", downloads: 0, author_id: 1, created_on: "2021-03-31 19:17:00.000000000 +0000", description: nil, disk_directory: "2021/03">

on some platforms, sort_by isn't stable
https://bugs.ruby-lang.org/issues/1089
Note that these two attachments have exactly the same timestamp.
https://github.com/redmine/redmine/blob/3e36b5c452210da457cb6c16385551414071693f/app/models/attachment.rb#L373

however, the spec depends on the exact order and it fails if otherwise. Let's make it deterministic. This change doesn't affect the purpose of the test.

fix_attachment_test.diff Magnifier (696 Bytes) Pavel Rosický, 2021-04-01 21:42

fix_attachment_test2.diff Magnifier (615 Bytes) Pavel Rosický, 2021-04-04 13:49

Associated revisions

Revision 20923
Added by Go MAEDA 6 months ago

The result of Attachment.latest_attach is unstable if attachments have the same timestamp (#34999).

Patch by Pavel Rosický.

Revision 20924
Added by Go MAEDA 6 months ago

Merged r20923 from trunk to 4.2-stable (#34999).

History

#2 Updated by Go MAEDA 6 months ago

Thank you for pointing it out.

My understanding is that the root cause is that Attachment.latest_attach is unstable when two or more attachments have the same timestamp.

How about changing the code to sort by id instead of created_on? Even if multiple attachments have the same timestamp, latest_attach will return the expected result.

Index: app/models/attachment.rb
===================================================================
--- app/models/attachment.rb    (リビジョン 20904)
+++ app/models/attachment.rb    (作業コピー)
@@ -370,7 +370,7 @@
   def self.latest_attach(attachments, filename)
     return unless filename.valid_encoding?

-    attachments.sort_by(&:created_on).reverse.detect do |att|
+    attachments.sort_by(&:id).reverse.detect do |att|
       filename.casecmp?(att.filename)
     end
   end

#3 Updated by Pavel Rosický 6 months ago

I don't think the ID column should be used as a timestamp, but I'm ok with using it as a second option, in case both timestamps are exactly the same.

fix_attachment_test2.diff is an alternative to the original patch

#4 Updated by Go MAEDA 6 months ago

  • Subject changed from Make case-insensitive matching test more deterministic to The result of Attachment.latest_attach is unstable if attachments have the same timestamp
  • Target version set to 4.2.1

Pavel Rosický wrote:

I don't think the ID column should be used as a timestamp, but I'm ok with using it as a second option, in case both timestamps are exactly the same.

Thank you, I will commit fix_attachment_test2.diff.

Setting the target version to 4.2.1.

#5 Updated by Go MAEDA 6 months ago

  • Status changed from New to Closed
  • Assignee set to Go MAEDA
  • Resolution set to Fixed

Committed the fix. Thank you.

Also available in: Atom PDF