Remote file paths #4537

mifi · 2023-06-29T22:02:17Z

implemented similar to local files (meta.relativePath)

related #2605

to avoid confusion with "folders"

dschmidt · 2023-06-30T07:18:52Z

Thanks for catching up with this, @mifi!

I have played around with it and was a bit surprised by the relativePath values especially as you write in a comment:
// add relativePath similar to non-remote files:

I would argue we could make "your" relativePath quite a bit more similar to non-remote/local files 😇

local files have the filename in the relativePath -> your remote files too, yay!
local files have relativePath set only if they are inside a selected folder, otherwise the value is null -> your remote files always have relativePath set
local files have a leading / in relativePath -> your remote files don't
most importantly: the relativePath of local files is relative to the selected folder and contains the selected folder -> your remote files always have the absolute path from the remote root
(basically if you have root/a/b/c/foobar.jpg and you select folder b, I expect foobar.jpg to have a relativePath of /b/c/foobar.jpg)

IMHO it would be nice to have the absolute path available as well, but the relativePath should resemble the behavior of local files as much as possible.

(c.f. #4486 (comment))

mifi · 2023-06-30T11:17:29Z

Thanks for your feedback.

local files have relativePath set only if they are inside a selected folder, otherwise the value is null -> your remote files always have relativePath set

IMO because the relativePath also includes the file's name, e.g. folder/fileName.jpg, I think it makes sense for it to simply say "fileName.jpg" when there is no folder, to ease implementations for developers using the path. Conceptually, the file still has a relative path, even though it's the root path (just the file's name), so I don't think null makes sense.

local files have a leading / in relativePath -> your remote files don't

I don't think relative path should have a leading /, because:

I don't always see this behavior for local files. If you look at webkitRelativePath docs and try their example, you will see that there is no leading slash
I think in general relative paths are not supposed to start with a slash (absolute paths are.) see for example UNIX.

I think I will change the code for local files so it also removes the leading slash. The reasoning is that the current behaviour is inconsistent (sometimes leading slash, sometimes none, depending on browser): The fallback for relativePath for local files is webkitRelativePath which does not have a leading slash, as can be seen here:

uppy/packages/@uppy/dashboard/src/Dashboard.jsx

Line 389 in c05a8c4

relativePath: file.relativePath || file.webkitRelativePath || null,

this could lead to bugs in implementations, if the developer always thinks that there is a leading slash when in fact there isn't always one.

most importantly: the relativePath of local files is relative to the selected folder and contains the selected folder -> your remote files always have the absolute path from the remote root
(basically if you have root/a/b/c/foobar.jpg and you select folder b, I expect foobar.jpg to have a relativePath of /b/c/foobar.jpg)

IMHO it would be nice to have the absolute path available as well, but the relativePath should resemble the behavior of local files as much as possible.

I agree. will implement this.

also: - implement absolutePath - always remove leading slash from local files (not only for webkitRelativePath)

dschmidt · 2023-06-30T13:07:18Z

Thanks for your feedback.

You're welcome, I'm happy if we come to a solution, we are all happy with :)

local files have relativePath set only if they are inside a selected folder, otherwise the value is null -> your remote files always have relativePath set

IMO because the relativePath also includes the file's name, e.g. folder/fileName.jpg, I think it makes sense for it to simply say "fileName.jpg" when there is no folder, to ease implementations for developers using the path. Conceptually, the file still has a relative path, even though it's the root path (just the file's name), so I don't think null makes sense.

Not sure, I would prioritize making ~~remote~~ single files behave just like ~~local~~ folder files. If I'm interested in the filename I can also use meta.name. Having null there makes it easy to check, whether a file is from a folder - we use that to assign additional meta data to files from folders.

In the end it's not too important for us, as we can probably workaround it rather easily, but easing implementations seems like a rather weak argument to introduce inconsistency here IMHO, as it's required to handle this for local files anyways.

local files have a leading / in relativePath -> your remote files don't

I don't think relative path should have a leading /, because:

I don't always see this behavior for local files. If you look at webkitRelativePath docs and try their example, you will see that there is no leading slash

I think in general relative paths are not supposed to start with a slash (absolute paths are.) see for example UNIX.

I think I will change the code for local files so it also removes the leading slash. The reasoning is that the current behaviour is inconsistent (sometimes leading slash, sometimes none, depending on browser): The fallback for relativePath for local files is webkitRelativePath which does not have a leading slash, as can be seen here:

uppy/packages/@uppy/dashboard/src/Dashboard.jsx

Line 389 in c05a8c4

relativePath: file.relativePath || file.webkitRelativePath || null,

this could lead to bugs in implementations, if the developer always thinks that there is a leading slash when in fact there isn't always one.

Consistency is good, if you think it's worth introducing this in this PR - we will deal with it ;-)

most importantly: the relativePath of local files is relative to the selected folder and contains the selected folder -> your remote files always have the absolute path from the remote root
(basically if you have root/a/b/c/foobar.jpg and you select folder b, I expect foobar.jpg to have a relativePath of /b/c/foobar.jpg)

IMHO it would be nice to have the absolute path available as well, but the relativePath should resemble the behavior of local files as much as possible.

I agree. will implement this.

Thanks, this was our most important concern. Latest commit works like a charm for me!

arturi · 2023-07-05T22:22:47Z

Having tested this, I must say I was about to write the same points as @dschmidt did in the beginning. Here we have nesting in absolutePath, but I was puzzled for a moment if relativePath is a path and not a name. A leading / would have helped me here.

Played with webkitRelativePath example, there's no leading / indeed, but it only accepts folders, so I was never confused whether this is just a file name, it always looks like a path.

IMO because the relativePath also includes the file's name, e.g. folder/fileName.jpg, I think it makes sense for it to simply say "fileName.jpg" when there is no folder, to ease implementations for developers using the path. Conceptually, the file still has a relative path, even though it's the root path (just the file's name), so I don't think null makes sense.

But when I select a single file on that demo page, webkitRelativePath is empty:

Having null there makes it easy to check, whether a file is from a folder - we use that to assign additional meta data to files from folders.

Also it does indeed sound like null is easier to check for, if relativePath: null, then the file was not selected within a folder. Currently, you'd need to look for / in the string, etc. Except what Mikael means, if I understood correctly, is you don't need to check, you just always set path to relativePath on the server, and that includes the file name, so fs.copyFileSync(tempDir, relativePath).

mifi · 2023-07-06T09:51:56Z

If we want to change the behaviour so that when selecting a file that is not in a folder, relativePath=null, then I think we should also change the behaviour so that relativePath does not include the name of the file itself. Then people won't get confused by whether or not relativePath is a path, because it won't contain the name of the file. However that's a breaking change if we want it to be consistent with local files relativePath.

Except what Mikael means, if I understood correctly, is you don't need to check, you just always set path to relativePath on the server, and that includes the file name, so fs.copyFileSync(tempDir, relativePath).

Yes, this is what I mean. If relativePath is never null, then the developer using it can simply always just treat is as a valid path and construct their resulting directory structure directly from it, without needing to add a special check for null.

cp uppyFile output-directory/$relativePath

But when I select a single file on that demo page, webkitRelativePath is empty:

How do you select a single file on that demo page? did you remove the webkitdirectory attribute? I think in our case if webkitRelativePath is empty (""), it makes sense to append the file name to relativePath so that it's consistent with everything else.

arturi · 2023-07-08T15:15:38Z

linking #3083 here as well

dschmidt · 2023-07-09T13:02:08Z

If we want to change the behaviour so that when selecting a file that is not in a folder, relativePath=null, then I think we should also change the behaviour so that relativePath does not include the name of the file itself. Then people won't get confused by whether or not relativePath is a path, because it won't contain the name of the file. However that's a breaking change if we want it to be consistent with local files relativePath.

But it will confuse people because it breaks with standard semantics known from browsers (and Uppy until now, it would be a breaking change and require e.g. us to adapt our code).
I understand your point that you want to unify leading slashes (removing or adding, I don't mind, maybe removing is actually nice because then relativePath and name can be ||ed), because it actually helps users to avoid writing code that works in one browser but not another, I second this change.
Completely changing the semantics of relativePath by removing the filename or setting it for files that are not from folders seems like a completely needless break with standard semantics.
IMHO for all the examples you bring up, one can find an example where it's making things more complicated - so although I'm repeating myself: I think we should just stick to standard semantics.
The mental model basically is: relativePath is a relative path to a selected folder. If there is no folder, there is no relativePath. Simple as that.

Yes, this is what I mean. If relativePath is never null, then the developer using it can simply always just treat is as a valid path and construct their resulting directory structure directly from it, without needing to add a special check for null.
cp uppyFile output-directory/$relativePath

Yes, having a relativePath for non folder files, we don't need

${targetDir}/${file.relativePath || file.name}

to reliably concat paths, but for checking if a file is from a folder we need
if (!file.relativePath.contains('/')) which is worse IMHO.

But when I select a single file on that demo page, webkitRelativePath is empty:

How do you select a single file on that demo page? did you remove the webkitdirectory attribute? I think in our case if webkitRelativePath is empty (""), it makes sense to append the file name to relativePath so that it's consistent with everything else.

From my pov it's making two things consistent that are not the same.
I think consistency should be achieved across browsers for things that are the same: e.g. always a/no leading slash for paths, always null/empty string for files from folders, local and remote files to some extent.
Making files from folders and single files indistinguishable makes certain use cases harder, is a breaking change and the only benefit I see is not having to write file.relativePath || file.name in certain use cases. If one really wants file.relativePath to contain the file name for folder files, it can be easily set in onBeforeFilesAdded hook, but imho it should not be the default

arturi · 2023-07-10T14:54:21Z

We've decided to keep relativePath: null when file was not selected with a folder, the current way, but unify file paths. Thanks for the input @dschmidt!

dschmidt · 2023-07-10T14:57:59Z

Okay, that is good news!

What does unify file paths exactly mean?
Remove leading slashes? Are you keeping the filename in the relativePath?

arturi · 2023-07-10T14:59:49Z

Yes, remove leading slashes. The filenames should stay as is, yes.

Murderlon

Mostly LGTM, couple comments

packages/@uppy/provider-views/src/Breadcrumbs.jsx

Murderlon · 2023-07-12T06:47:00Z

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

@@ -99,32 +129,34 @@ export default class ProviderView extends View {
  }

  /**
-   * Based on folder ID, fetch a new folder and update it to state
+   * Select a folder based on its id: fetches the folder and then updates state with its contents
+   * TODO rename to something better like selectFolder or navigateToFolder (breaking change?)


I don't think we need this todo? Name is not that bad.

getX sounds to me like pure function (no side effects) that simply returns something. but this function does a whole lot more than that: it changes the breadcrumbs, it lists folders/files and then updates the state

Unfortunately, almost everything in Uppy is state and almost all functions perform things on state. But yes since it's a public function I don't think we can change it now.

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

Murderlon · 2023-07-12T06:49:32Z

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

+          ...newItem,
+          // calculate the file's path relative to the user's selected item's path
+          // see https://github.com/transloadit/uppy/pull/4537#issuecomment-1614236655
+          relDirPath: newItem.absDirPath.replace(selectedItem.absDirPath, '').replace(/^\//, ''),


since you added absDirPath yourself, can't we handle this in addPath?

not sure how that would work

Murderlon · 2023-07-12T06:51:28Z

packages/@uppy/provider-views/src/View.js

@@ -92,6 +92,11 @@ export default class View {
      if (file.author.url) tagFile.meta.authorUrl = file.author.url
    }

+    // add relativePath similar to non-remote files: https://github.com/transloadit/uppy/pull/4486#issuecomment-1579203717
+    if (file.relDirPath != null) tagFile.meta.relativePath = file.relDirPath ? `${file.relDirPath}/${tagFile.name}` : tagFile.name


we currently change paths in three places: in addPath, mutation in recursivelyListAllFiles, and here. Would it be possible to have it centralised?

should be possible to set the paths in recursivelyListAllFiles - c.f. owncloud@9f111c5

I'm not handling absolute paths, but in @mifi 's implementation the absPath is available in recursivelyListAllFiles too.

I think we need to set paths in multiple places, because not all files are loaded by recursivelyListAllFiles

# Conflicts: # packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

Murderlon

Tested locally and it works well 👍

One Q, the difference between absolute and relative path is not clear to me in the value in provides. The only difference here is a leading slash. When are these two different and why would you want to use the absolute path?

{
  absolutePath: "/Two/9BSaOek-cowboy-bebop-backgrounds.jpg"
  relativePath: "Two/9BSaOek-cowboy-bebop-backgrounds.jpg"
}

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

mifi · 2023-07-13T09:31:33Z

One Q, the difference between absolute and relative path is not clear to me in the value in provides. The only difference here is a leading slash.
When are these two different and why would you want to use the absolute path?

if you in the UI select (checkbox) a directory inside a directory, they will be different. example:
folder1 > folder2 > file
if you select folder1, they will be the same (with a slash difference)
if you select folder2, rleativePath will be folder2/file, but absolutePath will be /folder1/folder2/file

Murderlon · 2023-07-13T10:35:23Z

Okay makes sense. We should document that here: https://uppy.io/docs/uppy/#working-with-uppy-files.

related transloadit/uppy#4537

mifi · 2023-07-13T12:42:26Z

created a pr to document metadata: transloadit/uppy.io#138

| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3-multipart | 3.5.0 | @uppy/locales | 3.2.3 | | @uppy/box | 2.1.2 | @uppy/onedrive | 3.1.2 | | @uppy/companion | 4.7.0 | @uppy/provider-views | 3.4.0 | | @uppy/companion-client | 3.2.1 | @uppy/react | 3.1.3 | | @uppy/core | 3.3.1 | @uppy/status-bar | 3.2.2 | | @uppy/dashboard | 3.4.2 | @uppy/transloadit | 3.2.0 | | @uppy/dropbox | 3.1.2 | @uppy/utils | 5.4.1 | | @uppy/google-drive | 3.2.0 | uppy | 3.12.0 | - @uppy/transloadit: fix error message (Antoine du Hamel / #4572) - @uppy/provider-views: add support for remote file paths (Mikael Finstad / #4537) - @uppy/transloadit: implement Server-sent event API (Antoine du Hamel / #4098) - @uppy/aws-s3-multipart: add support for signing on the client (Antoine du Hamel / #4519) - @uppy/react: allow `id` from props (Merlijn Vos / #4570) - @uppy/aws-s3-multipart: fix lint warning (Antoine du Hamel / #4569) - @uppy/status-bar: listen to `upload` event instead of button click (Antoine du Hamel / #4563) - @uppy/aws-s3-multipart: fix support for non-multipart PUT upload (Antoine du Hamel / #4568) - @uppy/companion: fix esm imports in production/transpiled builds (Dominik Schmidt / #4561) - @uppy/locales: fix expression and spelling errors in es_ES (Rubén / #4567) - meta: upgrade dev dependencies (dependabot\[bot\]) - meta: Don't use triage label (Artur Paikin / #4552) - meta: update Cypress (Antoine du Hamel / #4562) - @uppy/box,@uppy/companion,@uppy/dropbox,@uppy/google-drive,@uppy/onedrive,@uppy/provider-views: Load Google Drive / OneDrive lists 5-10x faster & always load all files (Merlijn Vos / #4513) - @uppy/locales: Add missing pt-BR locales for ImageEditor plugin (Mateus Cruz / #4558)

dschmidt · 2023-07-13T16:37:41Z

Hoooray! 🎉 Thanks a lot, everyone

* main: @uppy/aws-s3-multipart: refresh file before calling user-defined functions (#4557) Release: uppy@3.13.0 (#4595) Add i18n to CONTRIBUTING.md (#4591) Add VirtualList to ProviderView (#4566) @uppy/provider-views: fix race conditions with folder loading (#4578) @uppy/status-bar: fix ETA when status bar is installed during upload (#4588) @uppy/provider-views: fix infinite folder loading (#4590) examples/aws: client-side signing (#4463) Bump word-wrap from 1.2.3 to 1.2.4 (#4586) e2e: increase `requestTimeout` to 16s (#4587) @uppy/locales: update zh_TW translation (#4583) @uppy/aws-s3-multipart: fix crash on pause/resume (#4581) @uppy/aws-s3-multipart: do not access `globalThis.crypto` on the top-level (#4584) Release: uppy@3.12.0 (#4574) @uppy/transloadit: fix error message (#4572) @uppy/provider-views: add support for remote file paths (#4537) @uppy/transloadit: implement Server-sent event API (#4098)

mifi · 2023-08-06T20:40:56Z

I noticed a side-effect of null relativePaths is that the relativePath metadata for files uploaded to s3 will be the literal string "null":

mifi added 2 commits June 29, 2023 14:27

rename directories to directoryStack

02037c1

to avoid confusion with "folders"

implement relativePath for remote files

c05a8c4

closes #4486 #4034 related #2605

make relativePath work like with local files

f3fc489

also: - implement absolutePath - always remove leading slash from local files (not only for webkitRelativePath)

arturi self-requested a review July 3, 2023 13:16

This comment was marked as outdated.

Sign in to view

Murderlon reviewed Jul 12, 2023

View reviewed changes

Murderlon mentioned this pull request Jul 12, 2023

@uppy/companion: add parent folder path in onedrive provider #4486

Closed

mifi added 4 commits July 13, 2023 00:29

Merge branch 'main' into remote-file-paths

6409eee

# Conflicts: # packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx

apple code review suggestions

76b9f85

fix merge conflict issue

0de8c06

restore relativePath null behavior

32cc925

Murderlon reviewed Jul 13, 2023

View reviewed changes

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx Outdated Show resolved Hide resolved

packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx Outdated Show resolved Hide resolved

Apply suggestions from code review

002fe47

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

Murderlon approved these changes Jul 13, 2023

View reviewed changes

mifi added a commit to transloadit/uppy.io that referenced this pull request Jul 13, 2023

document meta

22c7431

related transloadit/uppy#4537

mifi mentioned this pull request Jul 13, 2023

Document the meta property on uppy files transloadit/uppy.io#138

Merged

aduh95 merged commit e054a25 into main Jul 13, 2023
14 of 15 checks passed

aduh95 deleted the remote-file-paths branch July 13, 2023 16:12

github-actions bot mentioned this pull request Jul 13, 2023

Release: uppy@3.12.0 #4574

Merged

dschmidt mentioned this pull request Jul 21, 2023

Get the source file name including directory when uploading from google drive #3621

Closed

dschmidt mentioned this pull request Aug 14, 2023

Recreate directory tree from box provider #4423

Closed

2 tasks

lakesare mentioned this pull request Apr 4, 2024

Provider views rewrite (.files, .folders => .partialTree) #5050

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote file paths #4537

Remote file paths #4537

mifi commented Jun 29, 2023

dschmidt commented Jun 30, 2023

mifi commented Jun 30, 2023

dschmidt commented Jun 30, 2023 •

edited

This comment was marked as outdated.

arturi commented Jul 5, 2023

mifi commented Jul 6, 2023

arturi commented Jul 8, 2023

dschmidt commented Jul 9, 2023 •

edited

arturi commented Jul 10, 2023

dschmidt commented Jul 10, 2023

arturi commented Jul 10, 2023

Murderlon left a comment

Murderlon Jul 12, 2023

mifi Jul 12, 2023

Murderlon Jul 13, 2023

Murderlon Jul 12, 2023

mifi Jul 12, 2023

Murderlon Jul 12, 2023

dschmidt Jul 12, 2023

mifi Jul 12, 2023

Murderlon left a comment

mifi commented Jul 13, 2023

Murderlon commented Jul 13, 2023

mifi commented Jul 13, 2023

dschmidt commented Jul 13, 2023 •

edited

mifi commented Aug 6, 2023

Remote file paths #4537

Remote file paths #4537

Conversation

mifi commented Jun 29, 2023

dschmidt commented Jun 30, 2023

mifi commented Jun 30, 2023

dschmidt commented Jun 30, 2023 • edited

This comment was marked as outdated.

arturi commented Jul 5, 2023

mifi commented Jul 6, 2023

arturi commented Jul 8, 2023

dschmidt commented Jul 9, 2023 • edited

arturi commented Jul 10, 2023

dschmidt commented Jul 10, 2023

arturi commented Jul 10, 2023

Murderlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murderlon left a comment

Choose a reason for hiding this comment

mifi commented Jul 13, 2023

Murderlon commented Jul 13, 2023

mifi commented Jul 13, 2023

dschmidt commented Jul 13, 2023 • edited

mifi commented Aug 6, 2023

dschmidt commented Jun 30, 2023 •

edited

dschmidt commented Jul 9, 2023 •

edited

dschmidt commented Jul 13, 2023 •

edited