Improve unserialization of VFS URL query parameters

[mahsashadi]

The implementation of createOptions method changes to support decoding of options passed by GET request.

Relevant:

• Fix unused options in readdir system VFS adapter osjs-client#164
• Improve serialization of VFS URL query parameters osjs-client#165

[andersevenrud]

Btw, this is what I came up with:

const assembleQueryData = (object) => {
  const assembled = {}
  const keys = Object.keys(object)

  for (let i = 0; i < keys.length; i++) {
    const key = keys[i]
    const dots = key.split('.')

    let last = assembled
    for (let j = 0; j < dots.length; j++) {
      const dot = dots[j]
      if (j >= dots.length - 1) {
        last[dot] = object[key]
      } else {
        last[dot] = last[dot] || {}
      }

      last = last[dot]
    }
  }

  return assembled
}

[andersevenrud]

Also, the parsing could just happen in the utility function:

osjs-server/src/utils/vfs.js

Line 177 in d250050

const parseGet = req => {

No need to touch the actual VFS code 🤔

[andersevenrud]

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

Improve unserialization  of VFS URL query parameters

[mahsashadi]

No need to touch the actual VFS code

Oh, yes, no need to change vfs!

so should I have a commit in src/utils/vfs.js with below changes?

/*
 * Parses URL Body
 */
const parseGet = req => {
  const {query} = url.parse(req.url, true);
  const assembledQuery = assembleQueryData(query);
  return Promise.resolve({fields: assembledQuery, files: {}});
};

/*
 * Assembles a given object query
 */
const assembleQueryData = (object) => {
  const assembled = {}
  const keys = Object.keys(object)

  for (let i = 0; i < keys.length; i++) {
    const key = keys[i]
    const dots = key.split('.')

    let last = assembled
    for (let j = 0; j < dots.length; j++) {
      const dot = dots[j]
      if (j >= dots.length - 1) {
        last[dot] = object[key]
      } else {
        last[dot] = last[dot] || {}
      }

      last = last[dot]
    }
  }
  return assembled;
}

[andersevenrud]

If you feel like my suggestion was a better example, then feel free to commit that.

We'll be adding tests here so we know that it works properly.

[andersevenrud]

Some minor stuff still to do here, including fixing the linting errors. I'll write a test for this and add it here.

[andersevenrud]

Here is a test that you can add to __tests__/utils/vfs.js. It can be placed in the bottom of the describe scope. Since I don't have write access to your fork, this is probably the easiest way to do this :)

  test('assembleQueryData', () => {
    const result = utils.assembleQueryData({
      'a': 'b',
      'b.a': 'foo',
      'b.b.a': 'foo',
      'b.b.b': 'foo',
      'b.b.c': 'foo',
      'b.b.d': 'foo',
      'b.b.e': 'foo',
      'c': 'null',
      'd': 'true',
      'e': '1'
    });

    expect(result).toEqual({
      a: 'b',
      b: {
        a: 'foo',
        b: {
          a: 'foo',
          b: 'foo',
          c: 'foo',
          d: 'foo',
          e: 'foo'
        }
      },
      c: 'null',
      d: 'true',
      e: '1'
    });
  });

[andersevenrud]

I realized that we had not taken arrays into account here.

?f.0=1&f.1=1&f.2=3 turns into:

    "f": {
        "2": "3"
    }

This is obviously wrong :D So this needs to be fixed as well.

[mahsashadi]

So this needs to be fixed as well.

I think we should encode differently in client-side.
For example f : ['a', 'b', 'c'] should be encoded like ?f=a&f=b&f=c, as suggested here.

In this way it will be decoded correctly. We don't have to touch unserialization.

[andersevenrud]

In this way it will be decoded correctly. We don't have to touch unserialization.

I believe that it's the underialization that needs to change. The client already does this correctly by having a sequence as keys, ex a.0, a.1 and so on.

[mahsashadi]

In this way, how we can distinguish between index and key 🤔
a.0=1 Can be both a:{0:1} and a:[1]

[andersevenrud]

We can pretty much assume that it's an array if all the suffixes for the current key are numbers.

a.0=1 Can be both a:{0:1} and a:[1]

I don't understand what you mean by this.

[mahsashadi]

I think we can replace this line with :
last[dot] = last[dot] || (/^\d+$/.test(dots[j + 1]) ? [] : {})

[andersevenrud]

Not sure that is 100% reliable. An object can have numbers as keys as well, and this does not seem to handle cases where it is for example mixed. You can check this by updating the test with the suggested change. I would recommend using a test-driven workflow: write the test with the outcome you want, then change the implementation. That way there's no need for guessing :)

…

On Sun, Sep 5, 2021, 19:54 mahsashadi ***@***.***> wrote: I think we can replace this line <https://github.com/mahsashadi/osjs-server/blob/146efc9335b9c68603765698d15479374ca8afa6/src/utils/vfs.js#L191> with : last[dot] = last[dot] || (/^\d+$/.test(dots[j + 1]) ? [] : {}) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#56 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABHODBHYESO62I23YQGBU3UAOVEXANCNFSM5C74ZRNQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[andersevenrud]

We're starting to get somewhere now 😄

Could you please rebase this branch to the latest master so that the tests runs properly ? I discovered a race condition in the Core that prevents the tests from completing without doing a timeout which makes it impossible to check this PR 🤓

[andersevenrud]

Just a quick instruction on how to rebase quickly:

git fetch origin master:master
git rebase master
git push --force

Edit I just realized that this won't work with forks... sorry. This should work 🤔

git remote add upstream https://github.com/os-js/osjs-server.git
git fetch upstream master:master
git rebase master
git push --force

[andersevenrud]

I ran the latest commit of this locally and checked the test, and it looks good to me 👍

So after you've done a rebase then the only thing that is left is to add the type casting!

os-js/osjs-client#165 (comment)

Nice work :)

[mahsashadi]

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

• system adapter implements capabilities method (suggested here)
• totalCount and totalSize is added to stat (for filemanager statusBar, as suggested here)

[ajmeese7]

Whenever I try to check the codeclimate report to see why the check is failing, I'm just getting the message "We do not currently have an analysis of these two commits" and whenever I click "Analyze" nothing seems to happen. Could you let me know if this PR needs some work that I could help out with?

[andersevenrud]

Whenever I try to check the codeclimate report to see why the check is failing

Probably because they have a retention policy on logs and this one is expired :/

[mahsashadi]

Hi again, long time past since this pull request.
Do you have any time to check them out?

[andersevenrud]

@mahsashadi yeah, it's been quite a while. This is such a large integration that I haven't found much time to review it. However, I have been thinking about it every now and then.

I'm not sure if having serialization like this is the best approach 🤔 Maybe this could be better solved by allowing JSON in the GET query ?

[ajmeese7]

@andersevenrud could you give an example of what you mean? If @mahsashadi doesn't have time to look into it I would be happy to give it a shot. Also, any updates on os-js/osjs-client#165, since that was mentioned in this PR as well?

[andersevenrud]

@ajmeese7 Instead of serializing like suggested here (and in linked PRs), I suggest just using JSON instead.

Suggested solution here for the following options = {a: {b: 1}} is:

?options.a.b.i=1

But I feel like this would be better approach:

?options=<JSON serialized data>.

---

In this context the relevant URL query parameters revolves around the options argument given to VFS methods.

---

The server by default does not allow this, but it's possible to override this to only be enabled on the VFS express router using middleware.

[mahsashadi]

So in client-side, we need to change this method as below:
export const encodeQueryData = (data) => (JSON.stringify(data));

In server-side, we can change this method as below:
const assembleQueryData = (object) => (JSON.parse(Object.keys(object)[0]))

But I think you mean something else in server-side, can you explain more? @andersevenrud

[andersevenrud]

So in client-side, we need to change this method as below:

Almost. It would be something like:

  const pairs = Object.entries(data).map(([key, val]) => {
    const isNull = val === null;
    if (typeof val === 'object' && !isNull) {
      return `${encodeURIComponent(key)}=${encodeURIComponent(JSON.stringify(val))}`
    } else {
      return `${encodeURIComponent(key)}=${encodeURIComponent(val)}`
    }
  });
  return pairs.join('&');

So instead of a "deep" serialization, any object or array is just made into JSON.

But I think you mean something else in server-side, can you explain more?

I believe there's a way to just use body-parser as a middleware for the VFS so a query string with JSON is parsed automatically.

If not, it's super easy to just do this manually (as you can see from the code example above).

[mahsashadi]

I believe there's a way to just use body-parser as a middleware for the VFS so a query string with JSON is parsed automatically.

Sorry but I did not get the whole thing yet:

• As I know, body-parser is used for POST requests, while here we are sending options as a query string in GET request. So how can It help?
• Doing the json-serialized in client-side as you said, then everything works well in server-side. Just left parseGet method as it was, then this method will create expected options for us.

[andersevenrud]

As I know, body-parser is used for POST requests

Sorry, yes that's correct. Not sure what I was thinking here :)

Doing the json-serialized in client-side as you said, ...

I don't understand this.

[mahsashadi]

I don't understand this.

With the code example you wrote above for serialization, seems nothing need to be change in sever… options are correctly unserialized and passed to vfs.

[andersevenrud]

With the code example you wrote above for serialization, seems nothing need to be change in sever… options are correctly unserialized and passed to vfs.

Since I was an idiot with the body-parser stuff, we have to implement some kind of deserialize on the server. But it would be as simple as just doing the opposite of the code example I gave :)

[mahsashadi]

Is this method OK for unserialization?

const assembleQueryData = (data) => {
  let assembled = {};
  for (const key in data) {
    try{
      const currentVal = JSON.parse(data[key]);
      assembled[key] = currentVal;
    }catch(e) {
      assembled[key] = data[key];
    }
  }
  return assembled;
};

[mahsashadi]

Almost. It would be something like:

I think here we are loosing some types for non-objects values. For example options: undefined

[andersevenrud]

I think here we are loosing some types for non-objects values. For example options: undefined

That won't really matter :)

[andersevenrud]

Is this method OK for unserialization?

Using native methods is a little bit nicer :)

const entries = Object
  .entries(data)
  .map(([k, v]) => {
    try {
      return [k, JSON.parse(v)]
    } catch (e) {
      return [k, v]
    }
  })

return Object.fromEntries(entries)

[mahsashadi]

Thanks @andersevenrud
I have committed mentioned changes both server and client side.
mahsashadi/osjs-client@12d5b65
mahsashadi@23b3c57

Just since undefined values are not supported by JSON.stringify(), I add a replacer function here.

[andersevenrud]

As mentioned in os-js/osjs-client#165 (comment) I would like to have the PR split up.

The same goes for this one.

[andersevenrud]

Thanks for the split 👌

since this branch now has so many commits and merges, and in addition to this now a conflict it might just be easier to open a new PR.

A small tip: Look into git rebase instead of doing merges. This will save you a little bit of pain when working with PRs when the master branch is updated.

[mahsashadi]

it might just be easier to open a new PR.

Great. Thanks. Since unserialization PR is done, I think I should create two new PRs (spliting the rest):

1. Add VFS capabilities method, suggested here.
2. Add totalCount and totalSize to VFS stats, suggested here.

If you agree, I will create these two PRs.

[andersevenrud]

Agreed @mahsashadi

[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