Bug #17337

Files not visible in Arvados Workbench 2

Added by Daniel Kutyła 3 months ago. Updated 19 days 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:
-

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

Task #17403: Review 17337-files-not-visible-in-arvadosIn ProgressDaniel Kutyła


Related issues

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

Associated revisions

Revision 5385afca
Added by Daniel Kutyła 20 days ago

Merge branch '17337-files-not-visible-in-arvados'
closes #17337

Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <>

History

#1 Updated by Peter Amstutz 2 months ago

  • Target version set to 2021-02-17 sprint

#2 Updated by Peter Amstutz 2 months ago

  • Assigned To set to Daniel Kutyła

#3 Updated by Peter Amstutz 2 months ago

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

#4 Updated by Lucas Di Pentima 2 months 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.

#6 Updated by Lucas Di Pentima about 2 months 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.

#8 Updated by Peter Amstutz about 2 months ago

  • Target version changed from 2021-03-03 sprint to 2021-03-17 sprint

#9 Updated by Peter Amstutz about 2 months ago

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

#10 Updated by Lucas Di Pentima about 2 months 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()?

#11 Updated by Lucas Di Pentima about 2 months 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.

#12 Updated by Lucas Di Pentima about 1 month 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.

#13 Updated by Peter Amstutz about 1 month ago

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

#14 Updated by Daniel Kutyła 30 days ago

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

Added custom encoding functions with tests

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

#15 Updated by Lucas Di Pentima 30 days 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'

#16 Updated by Lucas Di Pentima 29 days ago

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

#17 Updated by Tom Clegg 29 days 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);

#18 Updated by Daniel Kutyła 29 days ago

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

Added % sign handling within the filename

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

#20 Updated by Lucas Di Pentima 28 days 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?

#21 Updated by Daniel Kutyła 28 days ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/bde7ca868a0c201544476da6c049a98c1188dde9
Test run: https://ci.arvados.org/job/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

#23 Updated by Lucas Di Pentima 27 days 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

#24 Updated by Daniel Kutyła 26 days ago

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

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

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

#25 Updated by Lucas Di Pentima 26 days ago

Please check why unit tests are failing at https://ci.arvados.org/job/developer-tests-workbench2/356/console. Thanks!

#27 Updated by Lucas Di Pentima 26 days 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.

#28 Updated by Peter Amstutz 21 days ago

  • Status changed from New to In Progress

#29 Updated by Peter Amstutz 21 days ago

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

#30 Updated by Daniel Kutyła 21 days ago

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

Added more unit tests and handling for non usual directory names

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

#31 Updated by Lucas Di Pentima 20 days 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!

#32 Updated by Daniel Kutyła 20 days 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

#33 Updated by Daniel Kutyła 19 days ago

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

Also available in: Atom PDF