Serialization of Infinity is null

[accordproject]

Serialization of Infinity is null

[rajatrawataku1]

Can i work on this, if its not assigned to anyone else?

[mttrbrts]

@rajatrawataku1 by all means.

What I expect to happen is that the outputs of execution (contract state, response and emits) should all be validated, and a runtime error thrown if validation fails. Perhaps @jeromesimeon has some pointers?

[jeromesimeon]

Thanks @rajatrawataku1 and @mttrbrts I would actually first look at this exact same issue in Concerto (https://github.com/accordproject/concerto). I wouldn't be surprised that serialising from a Concerto object with +/-Infinity or NaN yields a JSON with a null. If so that would be the right place to fix this IMO.

[jeromesimeon]

A bit of a review of concerto about this. The internals seem sound, using parseFloat and should round trip properly. It's possible that the best approach might be to delay the check all the way to JSON.stringify.

This could be done along the lines of: https://stackoverflow.com/questions/21896792/force-json-stringify-to-emit-nan-infinity-or-js-json-lib-that-does-so

I would simply raise an error for NaN Infinity and -Infinity, something like:

bash-3.2$ cat test.js 
const json1 = {
    'foo': 'hello!',
    'bar': [ 1, 2, 3 ]
};
const json2 = {
    'foo': 'hello!',
    'bar': [ 1, 2, NaN ]
};

function safeStringify(input) {
    return JSON.stringify(input, function (key, value) {
        if (value !== value) {
            throw new Error('Cannot export NaN to JSON')
        }

        if (value === Infinity) {
            throw new Error('Cannot export Infinity to JSON');
        }

        if (value === -Infinity) {
            throw new Error('Cannot export -Infinity to JSON');
        }
        
        return value;
    });
}

console.log(JSON.stringify(json1));
console.log(safeStringify(json1));
console.log(JSON.stringify(json2));
console.log(safeStringify(json2));
bash-3.2$ node test.js
{"foo":"hello!","bar":[1,2,3]}
{"foo":"hello!","bar":[1,2,3]}
{"foo":"hello!","bar":[1,2,null]}
/Users/jeromesimeon/git/concerto/test.js:13
            throw new Error('Cannot export NaN to JSON')
            ^

Error: Cannot export NaN to JSON
    at Array.<anonymous> (/Users/jeromesimeon/git/concerto/test.js:13:19)
    at JSON.stringify (<anonymous>)
    at safeStringify (/Users/jeromesimeon/git/concerto/test.js:11:17)
    at Object.<anonymous> (/Users/jeromesimeon/git/concerto/test.js:31:13)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)

The benefit of that approach (and delaying the check to JSON.stringify) is that applications using the API would be able to handle results or inputs that include the full range of IEEE 754 values.

[jeromesimeon]

@rajatrawataku1 would you still feel up to the task?

This should include a review of everywhere we use JSON.stringify and maybe to put the variant above in a common utility file e.g., https://github.com/accordproject/ergo/blob/master/packages/ergo-compiler/lib/util.js

[irmerk]

Moving this back to open after 7 days.

[jeromesimeon]

@mttrbrts Any thoughts on my alternative suggestion? Would you see any benefit in doing this at validation time rather than serialization time?

[coderkalyan]

Could I take a shot at this? I'm new to this project, and would appreciate some guidance!

[jeromesimeon]

Could I take a shot at this? I'm new to this project, and would appreciate some guidance!

Sure @coderkalyan ! There is already quite a lot of details in this thread. Does it make sense to you, or which part is unclear?

[mttrbrts]

@coderkalyan yes, please feel free to pick this up. @jeromesimeon's approach is correct.

IMO, the correct place to fix this is in the ResourceValidator in Concerto

concerto/packages/concerto-core/lib/serializer/resourcevalidator.js

Line 293 in bf155ae

if(field.isPrimitive()) {

If we raise validation errors for NaN, +Infinity, and -Infinity, we can detect these elsewhere in the stack.

[jeromesimeon]

No activity on this for a while, I'm releasing the ownership for now.

[jeromesimeon]

A fix for this issue, by changing toJSON call in the serializer to raise a validation error when the Double is NaN, Infinity or -Infinity is available for review in PR #161.

[jeromesimeon]

A fix for this issue, by changing toJSON call in the serializer to raise a validation error when the Double is NaN, Infinity or -Infinity is available for review in PR #161.

That fix has been refactored to be done in the resourcevalidator rather than the jsongenerator class, as suggested by @mttrbrts. This provides better and more consistent error messages.

[jeromesimeon]

Fixed based on proposal in #161 in the 1.0 branch