Bug #15275

[API] Collection DELETE call fail when versioning is active

Added by Lucas Di Pentima about 2 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
05/24/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

req-5qyagztkh70zbk8bkvwj] Missing signature on locator a04da5d2fc3101a71e19146b1a1b238b+36
#<ArvadosModel::PermissionDeniedError: ArvadosModel::PermissionDeniedError>
Error 1558631657+1fe7d861: 403
{"method":"DELETE","path":"/arvados/v1/collections/c97qk-4zz18-3x18ksisv6vtu1y","format":"json","controller":"Arvados::V1::CollectionsController","action":"destroy","status":403,"duration":43.67,"view":0.41,"db":11.57,"request_id":"req-5qyagztkh70zbk8bkvwj","client_ipaddr":"54.209.184.185","client_auth":"4xphq-gj3su-fwroumji2gsa9tv","params":{"_profile":"true","help":"false","uuid_given":"true"},"@timestamp":"2019-05-23T17:14:17.400122232Z","@version":"1","message":"[403] DELETE /arvados/v1/collections/c97qk-4zz18-3x18ksisv6vtu1y (Arvados::V1::CollectionsController#destroy)"}

Previous investigation done by Peter points the cause of this to the manage_versioning callback. It should not try to make a snapshot when deleting a collection.


Subtasks

Task #15277: Review 15275-collection-delete-bugfixResolvedPeter Amstutz

Task #15280: Review 15275-attribute-dirtyness-bugfixResolvedPeter Amstutz


Related issues

Related to Arvados - Bug #15261: run-tests-federation failing on jenkinsResolved

Associated revisions

Revision cdc0e1ac
Added by Lucas Di Pentima about 2 months ago

Merge branch '15275-collection-delete-bugfix'. Refs #15275

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 2ea6b118
Added by Lucas Di Pentima about 2 months ago

Merge branch '15275-attribute-dirtyness-bugfix'
Closes #15275

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Peter Amstutz about 2 months ago

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
    

#2 Updated by Lucas Di Pentima about 2 months ago

  • Description updated (diff)

#3 Updated by Peter Amstutz about 2 months ago

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.

#4 Updated by Peter Amstutz about 2 months ago

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

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

#5 Updated by Peter Amstutz about 2 months ago

  • Related to Bug #15261: run-tests-federation failing on jenkins added

#6 Updated by Peter Amstutz about 2 months ago

  • Assigned To set to Lucas Di Pentima

#7 Updated by Lucas Di Pentima about 2 months ago

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.

#8 Updated by Peter Amstutz about 2 months ago

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?

#9 Updated by Peter Amstutz about 2 months ago

is_past_version? can be written simpler:

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

#10 Updated by Lucas Di Pentima about 2 months ago

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.

#11 Updated by Peter Amstutz about 2 months ago

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.

#12 Updated by Lucas Di Pentima about 2 months ago

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.

#13 Updated by Peter Amstutz about 2 months ago

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.

#14 Updated by Lucas Di Pentima about 2 months ago

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.

#15 Updated by Lucas Di Pentima about 2 months ago

  • % Done changed from 50 to 100
  • Status changed from In Progress to Resolved

Also available in: Atom PDF