Fix unused options in readdir system VFS adapter

[mahsashadi]

This change sends options correctly in readdir request.

Relevant:

• Improve serialization of VFS URL query parameters #165
• Improve unserialization of VFS URL query parameters osjs-server#56

[andersevenrud]

Could you please amend the commit message so that it says:

Fix unused options in readdir system VFS adapter

[andersevenrud]

I was just about to merge all your changes, and then it suddenly dawned on me:

The internal client options are now sent over to the server... which I feel like should be filtered.

A good example of a scenario like this is the following:

osjs-client/src/vfs.js

Lines 70 to 75 in 385b38f

	const handleDirectoryList = (path, options) => result =>
	Promise.resolve(result.map(stat => createFileIter(stat)))
	.then(result => transformReaddir(pathToObject(path), result, {
	showHiddenFiles: options.showHiddenFiles !== false,
	filter: options.filter
	}));

So before we can finally publish all the changes this needs to be addressed 🤓

[andersevenrud]

The internal client options are now sent over to the server... which I feel like should be filtered.

Actually, this brings up another interesting point! Should the client or the server be responsible for handling these options when you're using a "pagination" system like this.

---

Because the client sorting algorithm works like this:

1. Take all of the files in a readdir request
2. Filter out the ones that does not pass showHiddenFiles
3. Pass additional filter option
4. Go over the list, then:
   1. Put all directories into a group with an internal filter
   2. Put all the "special" into a group with an internal filter
   3. Put all the rest of the "regular" files into a group
5. Sort all groups alphabetically and merge them together into a flat array

Which would mean that in some cases you might see this:

👎 Hm... with client sorting and paging, this looks strange

-- PAGE 1 --
[dir] a
[dir] b
[dir] c
[file] bar

-- PAGE 2 --
[dir] d
[file] foo
[file] foo 1
[file] foo 2

-- PAGE 3 --
[dir] g
[dir] h
[file] i
[file] j

👌 This is it usually looks when you only get "one page" with client sorting

[dir] a
[dir] b
[dir] c
[dir] d

[dir] g
[dir] h
[file] bar
[file] foo

[file] foo 1
[file] foo 2
[file] i
[file] j

👍 No sorting on the client side

-- PAGE 1 --
[dir] a
[dir] b
[file] bar
[dir] c

-- PAGE 2 --
[dir] d
[file] foo
[file] foo 1
[file] foo 2

-- PAGE 3 --
[dir] g
[dir] h
[file] i
[file] j

---

The original design of readdir was to get "everything" at once, so we need to settle on something that does not "look wrong".

I feel like the option to disable client sorting is a good choice, especially since most filesystems will sort alphabetically no mater the type (dir / file), which is a lot more "natural".

As far as I see it, the best way to solve this is to add a new method to the adapter that can tell the client what it's supposed to do 🤔.

const adapter = (core) => {
  return {
    capabilities: async (file /*optional*/) => ({ // As async and with file as arument to future-proof implementation
      sort: true // Sorting is done on the adater side (aka "server")
    }),
    readdir: async ({path}, options) => {}
  };
};

---

I hope that makes sense... it's very late for me here, and I've been busy with other projects all day 😅

Also, I had to edit this like a million times, so what you see in the email is not what I originally had in mind....

---

Maybe we can just merge everything as-is then tackle this afterwards?

[mahsashadi]

he internal client options are now sent over to the server... which I feel like should be filtered.

I see.

As an another example, by opening a Open Dialog , since its options.filter has function typeof (which is not considered), it is sent to the server as options.filter.undefined. so it will not work!

So do you believe that filtering should be done in encodeQueryData method? Or some deeper solution is needed to completely separate server and client options 🤔

[mahsashadi]

So as I understand, you mean since most filesystems retuen the whole list in an alphabetic order, we can also make sure that every single page is already sorted (even beside other pages as a whole) if vfs.capabilities().sort returns true. If not, we should sort client-sidely, but just in alphabetic order (dir or files does not matter).

BTW, I did not understand what and why is the file argument in capabilities.

[mahsashadi]

In my case, the list returned by my storage API is ordered alphabetically, but files with uppercase letters precede the ones with lowercase letters.

[dir] 0
[file] 1
[dir] A
[dir] B
[file] B
[dir] C
[dir] a
[file] c
[dir] d

[andersevenrud]

As an another example, by opening a Open Dialog

This fits under the scenario I described. This will create a client-side option.

So do you believe that filtering should be done in encodeQueryData method?

Probably in the vfs abstraction per-function:

const filterOptions = (ignore, options) => {} // Returns a new "options" object without properties from "ignore"

export const readdir = (adapter, mount) => (path, options = {}) =>
  adapter.readdir(pathToObject(path), filterOptions(['a', 'b', 'c'], options), mount)
    .then(handleDirectoryList(path, options));

BTW, I did not understand what and why is the file argument in capabilities.

Because I wanted to make sure that this function can return capabilities based on files, not just global settings. Think for example if in the future there needs to be a new check for readfile.

In my case, the list returned by my storage API is ordered alphabetically, but files with uppercase letters precede the ones with lowercase letters.

This is what I suspected. This is what most filesystems do, which is fine.

[mahsashadi]

I commited some changes,
I think we only need to filter options in getters (readdir, readfile, exists, stat), right?
Except readdir, I didn't find options needed to be filtered in other getters, Is there any?

[mahsashadi]

This is what I suspected. This is what most filesystems do, which is fine.

So to start working on this point, I think we should have some agreement:

1. Is this format ok for detecting server capabilities?

capabilities: async (file /*optional*/) => ({ // As async and with file as arument to future-proof implementation
      sort: true, // Sorting is done on the adater side (aka "server")
      pagination: true // pagination is supported by this adapter
    }),

2. For servers without sorting capabilities (as system adapter I think), should we use this kind of sorting in client-side, and per page:?

[dir] 0
[file] 1
[dir] A
[dir] B
[file] B
[dir] C
[dir] a
[file] c
[dir] d

3. Any other agreement if needed ...

Maybe we can just merge everything as-is then tackle this afterwards?

I also think it's better to merge all these changes (after completing option filtering), and then work on this new point in two new PR (server and client)

[andersevenrud]

I also think it's better to merge all these changes (after completing option filtering), and then work on this new point in two new PR (server and client)

I think we'll take this in these steps:

1. I'll merge this PR now
2. You'll add another PR with the filtering of client options
3. Then rebase your other PR
4. I will merge the new option serialization stuff
5. Then open a new PR for the capabilities

Does this sound good ? I know it's been a long process so far, but I really wanna make sure we do this right 😊

Note to self: Since this is a semi-breaking change (both client and server** needs update together) I have to write some notices about this in the migration article in the online manual.

Edit: I just realized that you did 2) here 🤦‍♂️ I'll review that.

[mahsashadi]

Does this sound good ? I know it's been a long process so far, but I really wanna make sure we do this right

You are completely right, and I do wanna the same 👍

I just realized that you did 2) here I'll review that.

Sorry, since you said it is needed to be addressed before publishing, I did it here. So as you said after finishing these PR, maybe we can do sorting stuff in new one.

[mahsashadi]

Hi.
I again find some time to work on paging project :)
So is there anything else to be done on this branch?

[andersevenrud]

Hm. I don't think there's anything to do in this branch. This branch actually contains more changes than I really feel comfortable with, hehe.

I need to set off a day where I look over all this work again and then finally do a merge. Thankfully you don't need me to do that in order for you to continue the work on the pagination in the file manager :D

[andersevenrud]

in order for you to continue the work on the pagination in the file manager

You probably already know this -- and might even be using this right now: you can just use your forks with your changes in your installation: https://manual.os-js.org/guide/modules/

[mahsashadi]

you can just use your forks with your changes in your installation

Yes, but as I need to make pagination changes in the same fork package (osjs-client), I think it causes problem.

[andersevenrud]

Yes, but as I need to make pagination changes in the same fork package (osjs-client), I think it causes problem.

I don't understand what you mean by this. If you wanna test/use all these changes, simply make a new branch where you merge #164 and #164 :)

[andersevenrud]

Just a small reminder. I would appreciate if you'd have another look at this when #182 is merged :)