Arvados

The problem seems to be in manage_versioning:

1) When there are multiple fields in properties, should_preserve_version shows properties_changed? is true despite having the same values:

[req-1ja5jj5l0oq83reh1szm] properties true '{"type"=>"output", "container_request"=>"c97qk-xvhdp-sjkop3tbii0x6ov"}' '{"type"=>"
output", "container_request"=>"c97qk-xvhdp-sjkop3tbii0x6ov"}'

2) Because properties_changed? is true, should_preserve_version? returns true and it determines it should create a snapshot.

3) On the snapshot object, it sets snapshot.manifest_text = self.signed_manifest_text. However, because is_trashed is true (because this is a delete operation) it returns the manifest text unsigned.

4) When it saves the snapshot, the manifest text is not signed, so it raises the "Unsigned manifest" error.

I see a couple of issues here:

• properties_changed? is true, but it should not be
• signing the manifest is a workaround, it would work to do this?
  

        leave_modified_by_user_alone do
          act_as_system_user do
            snapshot.save!
          end
        end
  

c97qk-4zz18-3x18ksisv6vtu1y is the collection where I see this happen.

Note I'm issuing this as a federated request (home cluster 4xphq, which forwards the delete request to c97qk) but I don't believe there's anything specific to federation about this bug.

It is important to use a non-admin user, though.

Properties being incorrectly considered "dirty" may be a Rails 5 bug:

https://github.com/rails/rails/issues/28754

Updates at be66896514a5dccf6364a414d44391a3015592c5 - branch 15275-collection-delete-bugfix
Test run: https://ci.curoverse.com/job/developer-run-tests/1256/

This issue was caused by several bugs:

1) Snapshot being created when deleting a collection
2) Permission denied error when trying to create the new snapshot
3) The properties field is being detected as dirty when it isn't

This update fixes the permission denied problem and avoids create a snapshot when trashing a collection. This should be enough to unblock the federated tests.

return false if self.changes.keys.include?('is_trashed') && self.is_trashed_was == false

What's the rationale for this and not just "self.is_trashed == true" ? "Trashed collection don't create snapshots" seems like a reasonable behavior and one we could explicitly document.

  def is_past_version?
    if !new_record? && self.current_version_uuid_was != self.uuid_was

Why compare _was instead of comparing the current values?

is_past_version? can be written simpler:

def is_past_version?
   !(new_record? || self.current_version_uuid_was == self.uuid_was)
end

Updates at 082ca2b56
Test run: https://ci.curoverse.com/job/developer-run-tests/1257/

• Don't create a new version if is_trashed is true.
• Simplified code on is_path_version? method.

Lucas Di Pentima wrote:

Updates at 082ca2b56
Test run: https://ci.curoverse.com/job/developer-run-tests/1257/

• Don't create a new version if is_trashed is true.
• Simplified code on is_path_version? method.

This LGTM.

Last bugfix at 34729fcb1 - branch 15275-attribute-dirtyness-bugfix
Test run: https://ci.curoverse.com/job/developer-run-tests/1259/

Changes the way our custom types for JSONB columns check if the attributes changes by comparing the deserialized versions to avoid ordering or other problems when it comes to value comparison.

Lucas Di Pentima wrote:

Last bugfix at 34729fcb1 - branch 15275-attribute-dirtyness-bugfix
Test run: https://ci.curoverse.com/job/developer-run-tests/1259/

Changes the way our custom types for JSONB columns check if the attributes changes by comparing the deserialized versions to avoid ordering or other problems when it comes to value comparison.

This LGTM.

Sorry I take that back, we should add a few tests to confirm that hashes with nested arrays, nested hashes, or nil values are compared properly.

Updates at 5006b4b74
Test run: https://ci.curoverse.com/job/developer-run-tests/1261/

Adds several test cases for nested hashes, arrays in different ordering, etc.