record.set() fails to set some keys

[jonerer]

Hello!

We ran into an issue recently when building a record. We wanted to have a record containing a bunch of mac addresses. We wanted to use the mac addresses as keys. But we ran into this problem:

var macsRecord = ds.record.getRecord('some/path').whenReady(record => {
    macsRecord.set('0x000347971', 'macrecord')
    console.log(macsRecord.get())

    // will output { '0': 'macrecord' }

    macsRecord.set({
      '0x000347972': 'macrecord2'
    })
    console.log(macsRecord.get())

   // will output { '0x000347972': 'macrecord2' }
})

The expected result is that both methods produce the same output (like the latter method). I.e. it shouldn't turn a valid key string into a string containing just a zero.

The problem seems to be in the "tokenize" method in json-path.js:

	return cache[ path ] = parts.map( function( part ) {
		return !isNaN( part ) ? parseInt( part, 10 ) : part;
	} );

For some reason it's running "parseInt(part, 10)" on the key, which will turn it into 0.

We're running "deepstream.io-client-js": "^2.1.1"

[jonerer]

I guess it's meant to do parseInt() for things that are base-10-numbers. Unfortunately javascript accepts things as !isNaN than decimal numbers.
FYI:

> isNaN('0x23')
false
> isNaN('0f23')
true
> parseInt('0x234', 16)
564
> parseInt('0x234', 10)
0
> parseInt('0x234')
564

I guess the solution could either be to remove the "10" part from parseInt, or to just remove this whole thing altogether. I can't find any indication in the tests or the code why this tokenization exists.

[ralphtheninja]

So maybe we could replace parseInt(part, 10) with just parseInt(part)? It should work out of the box for base 10 numbers and also fix the hex problem.

[ralphtheninja]

@yasserf @WolframHempel

What is the reason behind this parseInt-conversion in the first place? I suspect some kind of optimization of key sizes. Would it be a terrible solution to remove it altogether? :) Alternatively, a way to opt-out of the optimization and store the keys as-is.

[jonerer]

I tried how the code works when changing parseInt(part, 10) to parseInt(part):

    macsRecord.set('0x000347971', 'macrecord')
    console.log(macsRecord.get())
// { '3438961': 'macrecord }
    console.log(macsRecord.get('0x000347971'))
// macrecord

    macsRecord.set({
      '0x000347972': 'macrecord2'
    })
    console.log(macsRecord.get())
// { '0x000347972': 'macrecord2' }
    console.log(macsRecord.get('0x000347972'))
// undefined

I would say that's rather counter-intuitive, and causes a change between how .get()/.set() and .get(path)/.set(path) works. I suggest removing the parseInt thing altogether for now

[yasserf]

To clarify, are you taking about:

deepstream.io-client-js/src/record/json-path.js

Line 127 in 1456176

return !isNaN( part ) ? parseInt( part, 10 ) : part;

Not sure if there is a specific reason we turn it into a number, other than because its an array index. This seems like a legit fix, can't see when anyone would use something that isn't a number to point to an element in an array.

[yasserf]

Just saw PR, thanks!

[ralphtheninja]

Ping! Can we get this merged? :)

[yasserf]

Closing since this has been merged on Feb 16