Improve serialization of VFS URL query parameters

[mahsashadi]

The implementation of encodeQueryData method changes to support encoding of nested objects.

Relevant:

• Fix unused options in readdir system VFS adapter #164
• Improve unserialization of VFS URL query parameters osjs-server#56

[andersevenrud]

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

Improve serialization of VFS URL query parameters

[andersevenrud]

Here is a test that you can place in a new file called __tests__/utils/fetch.js:

import * as fetch from '../../src/utils/fetch';

describe('utils.fetch#encodeQueryData', () => {
  test('should create valid query string', () => {
    const result1 = fetch.encodeQueryData({
      a: 1,
      b: true,
      c: null,
      d: 'foo'
    });

    const result2 = fetch.encodeQueryData({
      a: {
        a: 1,
        b: true,
        c: null,
        d: 'foo'
      },
      b: {
        c: {
          d: 'foo'
        }
      }
    });

    expect(result1).toEqual('a=1&b=true&c=null&d=foo');
    expect(result2).toEqual('a.a=1&a.b=true&a.c=null&a.d=foo&b.c.d=foo');
  });
});

As you can see from this test and the one in osjs-server, it demonstrates that issue with all values being strings 🤓

[mahsashadi]

As you can see from this test and the one in osjs-server, it demonstrates that issue with all values being strings

Yes, You are totally right. We should preserve value type somehow.

[mahsashadi]

To support encoding arrays, we can change method as below 🤔

function encodeQueryData(data, nesting = '') {
  const pairs = Object.entries(data).map(([key, val]) => {
    if (typeof val === 'object' && val !== null) {
      return Array.isArray(val)
        ? encodeQueryData(val, nesting + `${key}`)
        : encodeQueryData(val, nesting + `${key}.`);
    } else {
      return Array.isArray(data)
        ? encodeURIComponent(nesting) + '=' + encodeURIComponent(val)
        : encodeURIComponent(nesting + key) + '=' + encodeURIComponent(val);
    }
  });
  return pairs.join('&');
}

Do you have a better idea?

[mahsashadi]

We should preserve value type somehow.

I have not yet reached a solution to this problem 🤔

[andersevenrud]

Technically the serialization is correct, so I don't really think this needs to be changed to support arrays.

[andersevenrud]

Good stuff!

Now that we're on the same wavelength on how this all should work, I thought I'd open a new review thread on the final issue which is to solve the preservation of types.

I have added a possible solution to this here: #165 (comment)

[andersevenrud]

Sweet. I think this is done now.

I'm at work now, but I will test this myself etc. and merge when I get home.

[mahsashadi]

Thanks a lot for your help :)
Sorry if it takes your time.
I learned a lot during this PR 👍

[andersevenrud]

Thanks a lot for your help :)

And thank you for making these changes.

Sorry if it takes your time.

Don't worry about that. I haven't spent any time I don't have to look at this.

I learned a lot during this PR

Glad to hear that :)

[mahsashadi]

Hi again :)
To continue work on filemanager pagination, I have commited some changes:

• capabilities method is added to vfs (suggested here)
• I have merged changes in this PR here.

[mahsashadi]

Hi again, did you find anytime to take a look at my new commited codes?

[andersevenrud]

Sorry. Personal stuff has prevented me from doing much lately. Sorry about that, but I will get to this ASAP.

[andersevenrud]

Could you split up this PR so that it only contains the URL changes ?

Right now this technically contains two different kinds of work, and I don't want to merge everything at once.

[mahsashadi]

@andersevenrud Here is the subset of changes for URL serialization:

#182

Here is the subset of changes for URL unserialization:

os-js/osjs-server#63

[mahsashadi]

Do you agree to split like this:

1. URL serialization (done)
2. Client option filtering (maybe should be considered in url serialization)
3. Add VFS capabilities method
4. Improve client sorting algorithm

[andersevenrud]

I'm closing this since we've agreed on a way to move forward.

I've also opened an issue where we can concentrate the discussion. Things got a bit confusing with the client/server stuff :D

os-js/OS.js#804