feat: deserialize form data objects

[alcidesqueiroz]

closes #429

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

lgtm

[climba03003]

Thanks for the PR.

1. Can you have any reference on the implementation syntax for array and json key structure?
   For example, I never see foo{} key as an indicator for JSON value.

2. How would you distinguish if foo[0] means to be array but not object property?
   For example, qs did a great job by providing option for different behavior. So, the user is opt-in to which fit them most.

3. The current implementation is exposed to prototype poisoning attack.

4. You would need to write the document for new feature before it get merged.

[alcidesqueiroz]

Hi, @climba03003.

I've just updated the PR with the requested changes. In order to keep backward compatibility, I disabled key special notation parsing by default. To enable it, I introduced an option called enableSpecialNotationKeys that might be set either to true or to objectsOnly (similarly to the existing attachFieldsToBody option, that can be either true or keyValues).

Answering your previous questions:

1. I borrowed this particular notation from Axios serializer
2. My first approach was to parse numeric keys as array indexes. However, after seeing how qs deals with this dilemma, I've adopted a similar strategy, allowing the developer to disable this behavior through an option. For this, the enableSpecialNotationKeys option can be set to objectsOnly.
3. Good point. I changed my implementation to prevent it.
4. I've just included complete documentation for this feature.

[simoneb]

Nice job, I must admit I can't follow entirely what's going on here but apart from a couple of inline comments I can't spot anything else that would need changing.

[simoneb]

hard to follow what's going on here but I guess this is something that we have to do. I have a feeling that this code may be coming from somewhere else, possibly slightly modified though. if that is the case, don't forget to cite the source please

[alcidesqueiroz]

Even though I originally wanted to save time by starting from some pre-existing battle-tested code, I couldn't find anything close to what I needed, so I ended up writing the entire feature from scratch. Zero third-party code. 😄

[climba03003]

@Uzlopak Do you have any advice on the implementation (performance wise)?

[climba03003]

prevToken is a single character.
It is necessary to use regexp for this check?

[alcidesqueiroz]

In this case, prevToken not necessarily will have a single character. This regex would also match a multi-digit number. But thanks to your comment, I remembered that I was not checking for leading zeroes, so I extracted this checking to a function and explicitly checked if the number has no leading zero.

I've also benchmarked this regex-based checking against two other alternatives, one iteration-based and other that relies on parseFloat. The regex version won by a slim margin.

[climba03003]

Unless I read the code wrongly const prevToken = tokens[i - 1].
prevToken is never reassigned and is always a single character.
That means matching multi-digit number is useless for prevToken.

[alcidesqueiroz]

tokens is not a string, it's an array of strings. So, prevToken might be either [, ], {}, a multi-digit numeric index or an alphanumeric object key.

[climba03003]

Then it doesn't make sense why doesn't use a single for-loop.

[alcidesqueiroz]

The logic I've proposed relies on more than a loop in order to separate concerns. Mixing all steps in a single loop would make it way harder to read, debug and maintain.

[Uzlopak]

Had some thouhts on the code.

[Uzlopak]

Had some thouhts on the code.

[Uzlopak]

attachToBody is called only in the preValidation hook in line 277. We could actually add above the addHook a line to normalize the value. Then we dont need to check here for multiple values.

[alcidesqueiroz]

Didn't get it. Could you explain it in more details?

[Uzlopak]

Your examples are form.append('foo{}', '{"bar":[1,2,3]}') and more complex keys. But what if the value is a primitive, like 'a' or worse 'null'?

secureParse would not fail in case of null.

[alcidesqueiroz]

I've included an explicit check for null, but secureJSON.parse would, in fact, allow primitives to be passed. Is there a good reason for explicitly preventing this?

[Uzlopak]

what if enableSpecialNotationKeys is true? Maybe we should normalize like described in a different comment.

[alcidesqueiroz]

At this point, enableSpecialNotationKeys can only be true or "objectsOnly", as we're explicitly checking it on line 77. So, if it isn't "objectsOnly", it necessarily must be true.

[Uzlopak]

options.enableSpecialNotationKeys !== 'objectOnly' would be true if enableSpecialNotationKeys is true. So parseArrays would be true. So you would generate Arrays and not Objects with numeric keys?!

[alcidesqueiroz]

Exactly. When enableSpecialNotationKeys is true, numeric keys generate arrays. When enableSpecialNotationKeys is objectsOnly, everything is parsed as an object, so no arrays at all (unless you use the {} special ending and declare an array using JSON).

[alcidesqueiroz]

@Uzlopak @climba03003 hey there! 👋🏼 I wanted to check in on the status of this PR. Could you please let me know if there's anything else I need to address or if there's any further feedback? Thanks.

[simoneb]

@alcidesqueiroz thanks for your contribution here, and apologies for the lack of feedback. I think this was a valuable contribution and it's a pity that we couldn't integrate it in the codebase.

[alcidesqueiroz]

@simoneb no worries, mate! 🙂 🙏