feat(collapse): add parseInteger option

[hekike]

Make automatic string to integer parsing configurable (it's enabled by default, no change for existing customers)

Use-case: ['videosById', '123', 'title'] become ['videosById', 123, 'title'] after collapse.
In some places like in https://github.com/NetflixUI/nq-falcor-router we would like to preserve types to help Groovy comparison and keep consistency between user-input and output.

[lrowe]

As a dependency of falcor client I don't believe this code can use es6 language features.
I also question whether this feature is worthwhile since keys which roundtrip to integers may also be collapsed to ranges, so it's not possible for device code to rely on keys always being strings.

[hekike]

@lrowe thanks for the review. I turned my ES6 pieces to ES5.

We already got some feedback from iOS while they are working on their new client that the auto-casting makes Groovy comparison harder. Even if it shouldn't matter on the device by protocol definition I'd prefer if the user input and router output would match for consistency. I think auto-casting puts an unnecessary mental overhead on the device developer and makes testing more complex.

[DonutEspresso]

@lrowe I'm not sure about the use case here:

keys which roundtrip to integers may also be collapsed to ranges

Could you expand on that a little bit?

As @pmarton describes, our use case is of course router centric (which doesn't cross any serialization boundaries). It can be quite confusing if the input is not equivalent to the output. i.e., if I ask the router for ['videos', '123', 'title] and it cannot fulfill that route, it would be weird for the router to return an unhandled paths value of ['videos', 123, 'title']. We can of course implement our own collapser if necessary, but would prefer to reuse what we can if it makes sense.

[lrowe]

@DonutEspresso:

@lrowe I'm not sure about the use case here:

keys which roundtrip to integers may also be collapsed to ranges

Could you expand on that a little bit?

Because ['videos', '123', 'title'] and ['videos', 123, 'title'] are considered equivalent in Falcor, collapse will turn the paths [['videos', '123', 'title'], ['videos', '124', 'title']] into [['videos', {from: 123, to: 124}, 'title']] (in both Groovy and JavaScript.)

As @pmarton describes, our use case is of course router centric (which doesn't cross any serialization boundaries). It can be quite confusing if the input is not equivalent to the output. i.e., if I ask the router for ['videos', '123', 'title] and it cannot fulfill that route, it would be weird for the router to return an unhandled paths value of ['videos', 123, 'title'].

That weirdness is unavoidable if you run this collapse implementation since it sets keys onto JS objects in an intermediate step (coercing to string.) They may also ask for ['lolomo', 0, 0, 'title'] and with this option set they would get back ['lolomo', '0', '0', 'title'], which would be just as weird.

There are subtle inconsistencies between the Groovy and JS collapse implementations around sorting, so naively comparing the the pojos will inevitably lead to false positives. The solution is to expand to paths and normalize before comparing.

We can of course implement our own collapser if necessary, but would prefer to reuse what we can if it makes sense.

It may well make sense to diverge from this implementation targeted at devices at some point in the future.

[hekike]

@lrowe are you saying basically that Falcor shouldn't have integer keys? Because we cannot know for sure that it's not a range? Should we remove integerKey?

[lrowe]

@hekike:

JS objects and thus JSON and JSON Graph coerce all keys to strings. So JSON Graph Keys must be coerced to strings when being compared (since ["videos", 123] and ["videos", "123"] resolve to the same node in the graph.)

The goal of collapse is to provide a compact representation of a set of paths. Flexibility of representation of Keys in a Path or PathSet provides greater scope for compaction. Keys which safely roundtrip to integers are more compactly represented as integers and a set of consecutive integers are more compactly represented as a range.

Preservation of Key representation is a non-goal for either the JS or Groovy collapse implementations.

When evaluating PathSets in a router it's necessary to account for the variety of ways in which Keys may be represented and treat them equivalently. The initial representation of Keys used in the PathSets requested by a developer will likely have been lost along the way in the intermediate stages of cache, batch, and collapse before a request is made across the network.

The integerKey predicate is a convenience for developers defining routes that map onto services which use a 32 bit integer, so I would not argue for its removal. (It's also useful when the key maps logically onto an index in a list, though it may well be worth providing a higher level abstraction for nodes which are logically lists to automatically handle negative indices and length for the developer.)

[DonutEspresso]

Hm, interesting! That's good to know. FWIW we cast everything to a string in the router when we expand the incoming path sets into single paths (needed for route matching anyway).

I think to do this properly, we'd probably need to retain the original type information and use that when doing collapsing, instead of doing optimistic casting? I'd like to retain the intent of the caller - so if they pass a string we treat it as a string, if they pass a number we treat it as such.

As is, we'd likely to run into some inadvertent casting. We ran into a similar problem with fast properties, where values like '1e2' would be cast into an integer. Two paths with '1e2' and '2e2' could thus be collapsed into a range ({ from: 100, to: 200 }).

[lrowe]

Use isIntegerKey (or maybeIntegerKey which returns a number or undefined) to check if a key may be considered an integer.

[jcranendonk]

After some investigation, it appears that adding this option would not get us closer to the behavior of our legacy system.

Closing for now, but feel free to re-open if there are other arguments to add this option.

[DonutEspresso]

EDIT: attaching correct JIRA this time

I'm reopening this PR due to this issue:
https://jira.netflix.com/browse/IOSUI-26124

Because of this bug, NQ is returning a payload that seems to cause the client to be unhappy. If this PR is not the way to do it, then is there a proposed path forward?

cc @raymondk-nf