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

[JammingBen]

Adds the parent folder path to the OneDrive provider. This can be used to determine and distinguish different source locations of files.

Relates to #4034 (although it doesn't fix it completely because other providers are still missing).

[arturi]

Thank you! Note that we have relativePath meta data key in Uppy, I wonder if it should be used here instead of parentPath.

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

Lines 386 to 390 in 72f579e

	meta: {
	// path of the file relative to the ancestor directory the user selected.
	// e.g. 'docs/Old Prague/airbnb.pdf'
	relativePath: file.relativePath || file.webkitRelativePath || null,
	},

[JammingBen]

Thank you! Note that we have relativePath meta data key in Uppy, I wonder if it should be used here instead of parentPath.

That would be fine as well, although the property is a little bit different: relativePath is null for flat files while it would be /drive/root: here. Eventually you decide though, just let me know 🙂

[mifi]

what's a 'flat' file? and what does relativePath mean in the context of onedrive? I believe relativePath is only set for local files in Uppy? (not remote/provider files).

Also I'm not sure what's the difference between a file's relativePath and a file's path - what is a path relative to? should it instead be called path?

The way I understand it, parentPath in OneDrive is the whole absolute path to a file excluding the file name, kind of like dirname() in node.js, am i right?

[JammingBen]

what's a 'flat' file?

By that I meant files that are not located inside another directory but in the root directory.

The way I understand it, parentPath in OneDrive is the whole absolute path to a file excluding the file name, kind of like dirname() in node.js, am i right?

Yes, that's my understanding as well. Hence I figured parentPath would be a fitting name. Something like directoryPath would work as well I guess.

Also I'm not sure what's the difference between a file's relativePath and a file's path - what is a path relative to? should it instead be called path?

In that specific case there is no difference I believe. relativePath makes sense for local files as it's the path relative to the folder the file is located in on the OS. However, here, all paths are always relative to the OneDrive root = the file path.

[mifi]

Ok, I believe relativePath is something completely different than this then (it also includes the file's name), so we shouldn't mix the two together.

IMO something like directoryPath is a better name than parentPath, but we can let others weigh in too. @arturi ?

I find the root path /drive/root: a bit odd with the trailing colon. Am I right that if someone selects a file that has the (unix equivalent) path /mydir/myfile.pdf, then item.parentReference?.path would become /drive/root:/mydir ?

[mifi]

related? #4423

[arturi]

Also related: #4034?

[arturi]

Also thinking that /root: is a bit weird, especially if only GoogleDrive-specific, so maybe the path should be /googledrive/myfolder/file.txt or /googledrive/file.txt.

[Murderlon]

My two cents:

• Name it directoryPath
• Make the difference clear between requestPath and directoryPath. Comment could be enough.
• Align how we treat paths locally and remote. Is local absolute path? Perhaps not possible then. Otherwise we may want to remove /drive/root:?

[arturi]

Note: we add relativePath to file.id, so that two identical files from different folders are allowed in Uppy. Maybe we should account for that too with the new property.

[JammingBen]

Okay so I renamed the property to directoryPath, added comments and strip /drive/root: (which sounds reasonable to me).

It relates to #4423 and #4034 although it does not fix them because this is for OneDrive only.

[mifi]

I think the root path should be /. also there's a bug where it doesn't match from the start of the string

[dschmidt]

I've been playing around further with this with @JammingBen and we realized that this is not all we need.

Basically we want to achieve that recursive cloud uploads work - relative to the selected folder, just like local folders
The second change we would like to make is to add a relativePath property to the file via the ProviderView as the server has no knowledge about the selection:

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

Lines 277 to 292 in 2b9958c

	const onFiles = (files) => {
	for (const newFile of files) {
	const tagFile = this.getTagFile(newFile)
	const id = getSafeFileId(tagFile)
	// If the same folder is added again, we don't want to send
	// X amount of duplicate file notifications, we want to say
	// the folder was already added. This checks if all files are duplicate,
	// if that's the case, we don't add the files.
	if (!this.plugin.uppy.checkIfFileAlreadyExists(id)) {
	newFiles.push(newFile)
	numNewFiles++
	this.setLoading(this.plugin.uppy.i18n('addedNumFiles', { numFiles: numNewFiles }))
	}
	isEmpty = false
	}
	}

We might as well just build the full path including the filename in the server and name the property absolutePath instead of directoryPath.

To me that seems a tad more consistent and feels a little less awkward than directoryPath - what do you think?

Also it would make the code a little easier as we don't need to add the item.name in several places. We could do something along these lines:

              if(newFile.absolutePath?.startsWith(file.absolutePath)) {
                newFile.relativePath = newFile.absolutePath.slice(file.absolutePath.length - 1)
              }

This handles the relativePath completely analogue to the local file upload case:

• we set it only on files that are inside a selected folder and not for single files
• we have a leading /
• it contains the filename

The absolutePath would be analogue to the relativePath, it would just always be available and not only for files in selected folders.

Would you like a separate PR for the ProviderView change or would you prefer us to add it to this one?

[dschmidt]

When we agree on something, we should add it to the documentation here: https://github.com/transloadit/uppy.io/blob/4bc81c342bdf48fd9083c6b8975f1f3b078d4580/docs/companion.md?plain=1#L818

changed mind

[mifi]

I've been doing some more thinking around this, and I'm not sure if this is the way to do it, because not all providers support returning the path to Companion in their APIs, so it would't be consistent between providers. We probably want to implement a solution that work across all (most) providers.

Will continue the discussion here for now: #4034 (because it is not onedrive specific)

[dschmidt]

@JammingBen I guess you can close this one, as a different solution is being worked on and we use the same approach in our fork already

[Murderlon]

Superseded by #4537