sergiss / epc-tds

:factory: EPC Tag Data Standard encoding and decoding library, written in typescript/javascript for Node.js
MIT License
29 stars 17 forks source link

Invalid results with values larger than MAX_SAFE_INTEGER #8

Closed harriha closed 11 months ago

harriha commented 12 months ago

Hi,

Thanks for the library, very useful.

After starting to experiment with this library, I ran into problems which seem to originate from how BigInts and Numbers are being handled. I played with GIAI-96 values, but I suspect this issue might apply to many more formats as well. Case example:

// Testing with 34144B5A1C6A94D74F834DC7 / urn:epc:tag:giai-96:0.1234567.30000000004214215

const t = tds.fromTagURI("urn:epc:tag:giai-96:0.1234567.30000000004214215")
console.log(t.toTagURI())  // 'urn:epc:tag:giai-96:0.1234567.30000000004214216'   <-- incorrect assetReference
console.log(t.getAssetReference()) // 30000000004214216    <-- incorrect assetReference
console.log(t.toHexString())  // '34144B5A1C6A94D74F834DC8'    <-- incorrect hexString

As far as I could investigate this, this seems to relate to the fact that 30000000004214215 > Number.MAX_SAFE_INTEGER. In https://github.com/sergiss/epc-tds/blob/master/epc/utils/bit-array.js#L78 a BigInt is casted to a Number, where things fail:

> var big = BigInt(30000000004214215)
undefined
> Number(big)
30000000004214216

I'm afraid I don't understand all the corners of the library and EPC values well-enough to provide a proper fix, but I believe BigInts should be used everywhere in the library when dealing with the values. Then, when outputting the values, perhaps a bigger question from dev experience point of view is whether e.g. getAssetReference() should output BigInts or strings - I guess BigInt would be more correct.

Here's a simple snippet which could be added to your test.js for validating this case with GIAI-96 values, might be useful:

function withLargeValues() {
    // 30000000004214215 > Number.MAX_SAFE_INTEGER
    const tagUri = "urn:epc:tag:giai-96:0.1234567.30000000004214215"
    const epc = tds.fromTagURI(tagUri)
    const resultTagUri = epc.toTagURI()

    if (resultTagUri !== tagUri) {
        throw Error(`Giai96, expected: ${tagUri}, got: ${resultTagUri}`)
    }
}
withLargeValues()
sergiss commented 12 months ago

Hello, thank you very much for your contribution, you are correct, in that case, the decoding should be done using BigInt. I have prepared a solution for the problem that I will post between today and tomorrow. Thanks again, regards!

sergiss commented 12 months ago

I have updated the library (v.1.3.2) , and now your code should work correctly. If you detect any malfunctions, I would appreciate it if you could notify me. Regards.

harriha commented 11 months ago

Hi, thanks for the superfast fix, appreciated!

However, I'm afraid there's still some issues here with GIAI-96:

> const t = tds.fromTagURI("urn:epc:tag:giai-96:0.1234567.30000000004214215")
undefined
> t.toTagURI()
'urn:epc:tag:giai-96:0.1234567.30000000004214215'
> t.toHexString()
'34144B5A1C6A94D74F834DC7'
> t.getAssetReference()
30000000004214216
> t.getGiai()
'123456730000000004214216'

The toTagURI() and toHexString() produce correct output now, but the getAssetReference() and getGiai() don't. If I'm seeing correctly, the fix was fully applied to GIAI-202 but in GIAI-96 these functions are still using the .getSegment() which produces incorrect results?

sergiss commented 11 months ago

😅 I apologize, it was overlooked. I have created a new version (v.1.3.3) with the problem solved, thank you for your contribution.

harriha commented 11 months ago

Thanks! Those getAssetReference() and getGiai() seem to work now with GIAI-96, great.

One observation though: the GIAI-96.getAssetReference() returns a BigInt but GIAI-202.getAssetReference() returns a string now. Not sure which one was your target, but would probably be good to have identical return values in these.

On a related note, I guess these changes to the return types are likely to lead to issues if someone is using these functions and they blindly update the lib version to latest. Just stating this out loud as a heads-up, hopefully no major issues arise from these changes as a side effect.

sergiss commented 11 months ago

In the case of GIAI-96.getAssetReference(), a BigInt is returned because it is the only way to represent the values stored with that length. In the case of GIAI-202.getAssetReference(), a string is returned because in this case, alpha-numeric values are allowed. In this case, the user will need to be knowledgeable about the standards and handle the values correctly. Regards!

harriha commented 11 months ago

In the case of GIAI-96.getAssetReference(), a BigInt is returned because it is the only way to represent the values stored with that length. In the case of GIAI-202.getAssetReference(), a string is returned because in this case, alpha-numeric values are allowed. In this case, the user will need to be knowledgeable about the standards and handle the values correctly. Regards!

Ah, of course, thanks for the clarification, the limits of my knowledge about the different formats were already met it seems. But thanks a lot for the quick support with this issue, cheers!