Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid results with values larger than MAX_SAFE_INTEGER #8

Closed
harriha opened this issue Sep 28, 2023 · 7 comments
Closed

Invalid results with values larger than MAX_SAFE_INTEGER #8

harriha opened this issue Sep 28, 2023 · 7 comments

Comments

@harriha
Copy link

harriha commented Sep 28, 2023

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
Copy link
Owner

sergiss commented Sep 28, 2023

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
Copy link
Owner

sergiss commented Sep 28, 2023

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
Copy link
Author

harriha commented Sep 29, 2023

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
Copy link
Owner

sergiss commented Sep 29, 2023

😅 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
Copy link
Author

harriha commented Sep 29, 2023

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
Copy link
Owner

sergiss commented Sep 29, 2023

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!

@sergiss sergiss closed this as completed Sep 29, 2023
@harriha
Copy link
Author

harriha commented Oct 2, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants