Project

General

Profile

Actions

Bug #17337

closed

Files not visible in Arvados Workbench 2

Added by Daniel Kutyła over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
02/24/2021
Due date:
% Done:

100%

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

Description

Yesterday I uploaded some FASTA and BAM files to a collection in Arvados and the files are not visible in the interface. On top it is written that the collection contains 10 files, but in the file list only 2 files are shown. When I switch to Arvados Workbench 1 then all the files are visible in the file list normally.

Side note: "can it be due to the # in the file name?"


Subtasks 1 (1 open0 closed)

Task #17403: Review 17337-files-not-visible-in-arvadosIn ProgressDaniel Kutyła02/24/2021

Actions

Related issues

Related to Arvados - Bug #17422: Determine if keep-web correctly handles '#' in pathResolvedTom Clegg03/31/2021

Actions
Actions #1

Updated by Peter Amstutz over 1 year ago

  • Target version set to 2021-02-17 sprint
Actions #2

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2021-02-17 sprint to 2021-03-03 sprint
Actions #4

Updated by Lucas Di Pentima over 1 year ago

I've just tried to reproduce the issue and I was able to see two problems:

  • A file including a # char doesn't show on wb2 (it does on wb1).
  • Renaming a preexisting file to include a # char will truncate its name up to that char.
Actions #5

Updated by Daniel Kutyła over 1 year ago

Actions #6

Updated by Lucas Di Pentima over 1 year ago

Some comments:

  • On file src/services/collection-service/collection-service-files-response.test.ts a test was added for getFileFullPath() which wasn't changed in this branch. Can you also add a test for extractFilesData()?
  • Now files with whitespaces on their names aren't displayed on the collection's files panel. Please write a test for this.
    • Please check your IDE settings, as it's modifying the indentation on the majority of the cypress tests files, and it's making me nearly impossible to understand if you added something or if it's just all the file that got re-indented. (please explain here as a comment what did you change on cypress/integration/collection.spec.js)
    • Manually trying to upload a file with a whitespace or renaming a file by adding a whitespase makes the file disappear from the listing, I'm not sure why the 'renames a file using valid names' cypress tests aren't failing.
Actions #8

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2021-03-03 sprint to 2021-03-17 sprint
Actions #9

Updated by Peter Amstutz over 1 year ago

  • Related to Bug #17422: Determine if keep-web correctly handles '#' in path added
Actions #10

Updated by Lucas Di Pentima over 1 year ago

  • On file cypress/integration/collection.spec.js it was added {force: true} on some rightclick() call (line 141), can you explain why this is needed on some and not others? (for example, line 374) - As per Cypress' documentation, force: true disables element actionability checks, is this always desirable, or in certain situations? Asking mostly to learn about Cypress testing strategies for future tests.
  • The "should display all files within the collection even with the # sign within the file name" test case seems to be superfluous, as you can add name transitions on the "renames a file using valid names" test case (file cypress/integration/collection.spec.js line 190) avoiding adding a new test case with its set-up time cost.
  • There're still some cases where this isn't working:
    • The 2nd case described at #note-4 is still happening.
    • Also when a file has # chars and whitespaces, it disappears from the listings.
    • When trying to rename a file with trailing # sign it gets a 412 error. Checking the network inspector it seems that the Destination request header is wrong.
    • I've also found similar cases with the ? sign.
    • I've added the test cases on cypress so you can make them pass. (you can use wb1 to do the manual testing -- you'll need to click on the lock button before renaming files on wb1)
  • Could you write some unit tests for escapeHashIfRequired()?
Actions #11

Updated by Lucas Di Pentima over 1 year ago

I’m reading http://xkr.us/articles/javascript/encode-compare/ and it says encodeURIComponent() will encode / and we already allow slashes on ‘Rename’ to move files to a subdir, so take that into account.

Actions #12

Updated by Lucas Di Pentima over 1 year ago

After our call about this issue, I gave it a go on the encodeURI() vs encodeURIComponent() differences:

diff --git a/src/services/collection-service/collection-service.ts b/src/services/collection-service/collection-service.ts
index c46c3e27..05f00805 100644
--- a/src/services/collection-service/collection-service.ts
+++ b/src/services/collection-service/collection-service.ts
@@ -76,9 +76,10 @@ export class CollectionService extends TrashableResourceService<CollectionResour
     }

     async moveFile(collectionUuid: string, oldPath: string, newPath: string) {
+        const encodedNewPath = newPath.split('/').map((c) => encodeURIComponent(c)).join('/');
         await this.webdavClient.move(
             `c=${collectionUuid}${oldPath}`,
-            `c=${collectionUuid}${encodeURI(newPath)}`
+            `c=${collectionUuid}/${encodedNewPath}`
         );
         await this.update(collectionUuid, { preserveVersion: true });
     }

That seems to fix the renaming issue. The other problem is making wb2 being able to properly render a file with a # char on its name. I've found that a similar problems lies on the file src/services/collection-service/collection-service-files-response.ts, inside extractFilesData(), the getTagValue() function used to get the file's url won't decode the # chars and leave them as %23, so I think a solution similar to the above diff would be necessary.

Actions #13

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2021-03-17 sprint to 2021-03-31 sprint
Actions #14

Updated by Daniel Kutyła over 1 year ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/8297f0f273e326e64145a48266805b0b3d073c32
Test run: developer-tests-workbench2: #340

Added custom encoding functions with tests

branch: 17337-files-not-visible-in-arvados

Actions #15

Updated by Lucas Di Pentima over 1 year ago

  • An only() call wasn't removed from the cypress collections test suite.
  • The above issue masked a failing cypress test called 'renames a file to a different directory'
Actions #16

Updated by Lucas Di Pentima over 1 year ago

I've pushed arvados-workbench2|ef1017fb adding an additional failing file rename case: % chars.

Actions #17

Updated by Tom Clegg over 1 year ago

In src/common/url.ts, surely this

encodeURIComponent(path.replace(/%2F/g, '/'))

was meant to be

encodeURIComponent(path).replace(/%2F/g, '/')

...and this

decodeURIComponent(path.replace(/\//g, '%2F'));

could be written more simply as

decodeURIComponent(path);
Actions #18

Updated by Daniel Kutyła over 1 year ago

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

Added % sign handling within the filename

branch: 17337-files-not-visible-in-arvados

Actions #20

Updated by Lucas Di Pentima over 1 year ago

  • At file src/common/url.test.ts
    • Lines 33 & 47: Test suites with equal names are confusing.
    • Lines 34 & 48: I think those tests need better naming.
  • Sorry I didn't got this other case earlier, but: wb2 doesn't seem to list files with %2F on their names. I've added a test case for that on arvados-workbench2|3ecdcece
  • At file src/common/url.ts lines 24 & 32: is there a reason to catch & ignore errors on these cases? If so, could we add test cases for them?
Actions #21

Updated by Daniel Kutyła over 1 year ago

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

Added %2F handling, changed the way encode and decode is performed to avoid issues with the '/' characters

branch: 17337-files-not-visible-in-arvados

Actions #23

Updated by Lucas Di Pentima over 1 year ago

I've found some more file name displaying issues. It seems that string literals like %22 get decoded when displaying file names and also this incorrectly decoded file name is used as a source path when the user tries to change the file name to another one.

I've added some cases in Cypress at arvados-workbench2|5436892f

Actions #24

Updated by Daniel Kutyła over 1 year ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/5642c48285c296aa3b77760ed5d1431ccdb0f2f3
Test run: developer-tests-workbench2: #356

Added more tests to handle edge cases, fixed double decoding issue

branch: 17337-files-not-visible-in-arvados

Actions #25

Updated by Lucas Di Pentima over 1 year ago

Please check why unit tests are failing at developer-tests-workbench2: #356 /console. Thanks!

Actions #26

Updated by Daniel Kutyła over 1 year ago

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

Fixed failing unit test

branch: 17337-files-not-visible-in-arvados

Actions #27

Updated by Lucas Di Pentima over 1 year ago

I feel we're almost there! :)

  • When I try to download a file with a name that includes a literal string %20 (for example), workbench2 doesn't encode it as %2520 so the resulting download URL points to an inexistent file, getting a 404.
  • I've updated the src/services/collection-service/collection-service-files-response.test.ts file to coalesce the 2 tests into one with better XML indentation and more test cases, including the one I mentioned yesterday: the double-encoded file name. In those test cases is evident that the "expected url" is the decoded form of the URL, that then needs to be re-encoded to be able to use it, this may be the reason of the issue described on the previous bulletpoint.
Actions #28

Updated by Peter Amstutz over 1 year ago

  • Status changed from New to In Progress
Actions #29

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2021-03-31 sprint to 2021-04-14 sprint
Actions #30

Updated by Daniel Kutyła over 1 year ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/5a7d3c08744413b659fd24be59992fc1daa486e5
Test run: developer-tests-workbench2: #360

Added more unit tests and handling for non usual directory names

branch: 17337-files-not-visible-in-arvados

Actions #31

Updated by Lucas Di Pentima over 1 year ago

Just 2 small comments before merging:

  • File src/services/collection-service/collection-service-files-response.ts lines 33-36: I believe this is used to remove any trailing slashes on the url, correct? If that's the case, wouldn't you think that it could be simpler to add a .replace(/\/$/, '') call instead? We already make several replacements to the url, so it would be convenient for code consistency's sake.
  • File src/services/collection-service/collection-service-files-response.ts lines 46 & 50: Is there a reason to use the deprecated unescape() function instead of decodeURI() or decodeURIComponent()? https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/unescape

With those fixed, it LGTM!

Actions #32

Updated by Daniel Kutyła over 1 year ago

  • lines 33-36 removes last item from the url to get the parent dir and since directories ends with / first pop for them will return empty string that is why there is a second one
Actions #33

Updated by Daniel Kutyła over 1 year ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions #34

Updated by Peter Amstutz over 1 year ago

  • Release set to 38
Actions

Also available in: Atom PDF