Project

General

Profile

Actions

Defect #34999

closed

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

Added by Pavel Rosický over 3 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Attachments
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

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.


Files

fix_attachment_test.diff (696 Bytes) fix_attachment_test.diff Pavel Rosický, 2021-04-01 21:42
fix_attachment_test2.diff (615 Bytes) fix_attachment_test2.diff Pavel Rosický, 2021-04-04 13:49
Actions #2

Updated by Go MAEDA over 3 years 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
Actions #3

Updated by Pavel Rosický over 3 years 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

Actions #4

Updated by Go MAEDA over 3 years 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.

Actions #5

Updated by Go MAEDA over 3 years ago

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

Committed the fix. Thank you.

Actions

Also available in: Atom PDF