Project

General

Profile

Actions

Bug #17016

closed

"Remove file" deletes whole collection?

Added by Peter Amstutz over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Story points:
-

Description

  1. Create a collection with arv-put. It consists of two files in a subdirectory.
  2. Go to workbench2
  3. Select the first file
  4. Click on the ... menu on the right
  5. Choose "Remove"
  6. Hit ok
  7. Instead of just one file, all the files are gone.

This behaves inconsistently. I've also tried to delete a single item in a folder and it deleted the whole folder (but not the whole collection).


Subtasks 1 (0 open1 closed)

Task #17025: Review 17016-delete-single-file-deletes-whole-collectionResolvedLucas Di Pentima12/01/2020Actions

Related issues

Related to Arvados - Bug #15771: Deleting a selection of files failsResolvedDaniel Kutyła01/12/2021Actions
Actions #1

Updated by Peter Amstutz over 3 years ago

  • Subject changed from Remove file deletes whole project? to "Remove file" deletes whole collection?
Actions #2

Updated by Peter Amstutz over 3 years ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz over 3 years ago

  • Related to Bug #15771: Deleting a selection of files fails added
Actions #4

Updated by Peter Amstutz over 3 years ago

  • Assigned To set to Daniel Kutyła
Actions #5

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-11-04 Sprint to 2020-11-18
Actions #6

Updated by Peter Amstutz over 3 years ago

  • Description updated (diff)
Actions #7

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-11-18 to 2020-12-02 Sprint
Actions #8

Updated by Daniel Kutyła over 3 years ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/36d0bf2c656c5ff79c94216a8bfbed015bc84f7e
Test run: developer-tests-workbench2: #203

Fixed duplicated file names, added sorting in order to avoid of folder being removed before the files within them

Actions #9

Updated by Daniel Kutyła over 3 years ago

  • Status changed from New to In Progress
Actions #10

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-12-02 Sprint to 2020-12-16 Sprint
Actions #11

Updated by Daniel Kutyła over 3 years ago

Code mentioned above should also fix #15771 as this bug is closely related

Actions #12

Updated by Lucas Di Pentima over 3 years ago

Reviewing arvados-workbench2|36d0bf2 - branch 17016-delete-single-file-deletes-whole-collection

  • The remove feature seems to work correctly now, thanks! Would you add an integration test with a case similar to what this ticket describes? (remove file in subdir) You could make one of those by creating a collection with a synthetic manifest_text like other cypress test already do, so that you don't need to upload real files.
  • From reading the code and doing some testing on my own, the received paths are sorted by length descending, I suppose this is to avoid getting errors when trying to remove files that were inside subdirectories already removed. This looks ok at first but when selecting several parts of a very deep and/or populated file tree, this would make the wb2 issue too many webdav requests by individually removing many files that could be just removed in one sweep because the parent directory is already scheduled for removal. Could we make the opposite? Like, starting removing the shorter paths and once they’re gone, avoid sending delete requests to dirs/files inside the already removed ones?
Actions #13

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2020-12-16 Sprint to 2021-01-06 Sprint
Actions #15

Updated by Peter Amstutz over 3 years ago

  • Target version changed from 2021-01-06 Sprint to 2021-01-20 Sprint
Actions #16

Updated by Lucas Di Pentima over 3 years ago

Danny: Not sure if this is ready for review, but I took a look at it anyways.

  • The integration tests look good. One suggestion, though: You can modify the manifest_text on the second test to directly create a collection with files inside a subdirectory by replacing . with ./subdir on the file entries, that way you could simplify a lot the test because you won't have to make it move the file. You can read more about the manifest text format here: https://doc.arvados.org/v2.1/architecture/manifest-format.html
  • My second bulletpoint at note-12 is still pending to be addressed.
Actions #18

Updated by Lucas Di Pentima over 3 years ago

This works great, thanks!

As one last request: Could you write a unit test that test this last optimization? I think it would be really useful to have automated warranties that this keeps working as expected in the future.

Actions #20

Updated by Lucas Di Pentima over 3 years ago

This LGTM, please merge. Thanks!

Actions #21

Updated by Daniel Kutyła over 3 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF