Defect #34999
closed
The result of Attachment.latest_attach is unstable if attachments have the same timestamp
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.
Files
Updated by Pavel Rosický over 3 years ago
- File fix_attachment_test.diff fix_attachment_test.diff added
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
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
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.
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.