Project

General

Profile

Actions

Bug #15275

closed

[API] Collection DELETE call fail when versioning is active

Added by Lucas Di Pentima almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
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 2 (0 open2 closed)

Task #15277: Review 15275-collection-delete-bugfixResolvedPeter Amstutz05/24/2019Actions
Task #15280: Review 15275-attribute-dirtyness-bugfixResolvedPeter Amstutz05/24/2019Actions

Related issues

Related to Arvados - Bug #15261: run-tests-federation failing on jenkinsResolvedPeter AmstutzActions
Actions #1

Updated by Peter Amstutz almost 5 years 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
    
Actions #2

Updated by Lucas Di Pentima almost 5 years ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz almost 5 years 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.

Actions #4

Updated by Peter Amstutz almost 5 years ago

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

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

Actions #5

Updated by Peter Amstutz almost 5 years ago

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

Updated by Peter Amstutz almost 5 years ago

  • Assigned To set to Lucas Di Pentima
Actions #7

Updated by Lucas Di Pentima almost 5 years 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.

Actions #8

Updated by Peter Amstutz almost 5 years 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?

Actions #9

Updated by Peter Amstutz almost 5 years ago

is_past_version? can be written simpler:

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

Updated by Lucas Di Pentima almost 5 years 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.
Actions #11

Updated by Peter Amstutz almost 5 years 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.

Actions #12

Updated by Lucas Di Pentima almost 5 years 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.

Actions #13

Updated by Peter Amstutz almost 5 years 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.

Actions #14

Updated by Lucas Di Pentima almost 5 years 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.

Actions #15

Updated by Lucas Di Pentima almost 5 years ago

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

Also available in: Atom PDF