-
Notifications
You must be signed in to change notification settings - Fork 75
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
Support decimal type from typespec #2157
Comments
How is this being represented over the wire? Presumably they are not sending it as a JSON number since that would potentially lose precision. If it's a string my preference is to keep it as a string and let the consumer use whatever userland library they prefer to handle the number manipulation. |
@xirzec @bterlson Let me explain this with more details. I'm not sure whether we should adopt any third party libs or we have better choices to support decimal. BackgroundThe JSON standard does not specify any length and precision limitation for
When supporting No native support in JS
No native support in JSON.parse and JSON.stringifyEven for const res = BigInt("9223372036854775807");
console.log(JSON.stringify(res));
// ============= Here is the output =============
/**
[ERR]: "Executed JavaScript Failed:"
[ERR]: Do not know how to serialize a BigInt
*/ And for general cases we may lose the precision or lead to wrong numbers. const json = '{ "foo" : 0.1111111111111111222222222222222111111111111, "bar": 9223372036854775807, "baz": 123 }';
const r = JSON.parse(json);
console.log('JSON.parse(input).foo : ', r.foo.toString());
console.log('JSON.parse(input).bar : ', r.bar.toString());
console.log('JSON.parse(input).baz : ', r.baz.toString());
// ============= Here is the output =============
/**
[LOG]: "JSON.parse(input).foo : ", "0.11111111111111112", "number"
[LOG]: "JSON.parse(input).bar : ", "9223372036854776000", "number"
[LOG]: "JSON.parse(input).baz : ", "123", "number"
*/ |
Regarding how it's sent on the wire, we decided to use actual numbers by default. Looking at the general use cases, most often we're talking about currency amounts that fit well inside a double's range and precision. The string just added confusion. This does mean that if a service that cares strongly about preserving decimal's precision, it should use My opinion is that, for RLC, it's just whatever is on the wire, so number by default, but maybe string if For modular, I don't think it's worth using a library like decimal.js or similar. Again, going back to common use cases, we're usually talking about numbers within the safe range. Problems due to rounding may crop up when users need to use those numbers in calculations, but I think this is far less common, and anyway it's fairly trivial for a user to pick their favorite decimal lib and convert a number or string to the decimal prior to doing any calculations. So I think my recommendation there is also to just use numbers or strings depending on @encode. |
Using @encode as string could be our recommandation. But not sure it could be a general one considering below reasons:
I agree that for calculations, users could pick their own lib then converting to a number or string for us(aka client SDK) and within the safe range it should be safe also. However I am worrying about that the support has potential risks but it is a silent process. Not sure we need to involve more communications with service team to clarify this because eventually they will take the cost of the risk. On the other hand how to educate end users - the contact with server side is based on |
pls ignore the wrong linked to lro epic :) |
To be explicit, there are two risks related to precision loss:
For 1, the damage is limited to JavaScript as I believe most other languages have the ability to either dynamically choose an appropriate data type or specify the data type when parsing number literals from JSON. For JS in particular, the risk of 1 seems very low considering I have not seen a use case in Azure yet where numbers would fall outside the range of a double, which isn't particularly surprising given that the main usecase is currency amounts, and currency amounts above 2^53-1 or outside the precision of floats for typical currency values (~15 decimal places) seems very unlikely. However, services should be aware of these limitations and use @encode properly, documenting this somewhere for service teams seems good. For 2, again the damage is mostly limited to JavaScript, since other languages can presumably vend a decimal appropriately regardless of how the decimal is placed on the wire. For JavaScript in particular, the risk seems low considering that the use cases I examined didn't involve round trip of currency values that are calculated by the client. However, it would probably be a good idea to document fields with the decimal type with some standard verbiage explaining that it's a decimal value and to take care when doing math on the value to avoid precision loss. |
https://github.com/Azure/azure-rest-api-specs-pr/pull/13119#discussion_r1376998182
In typespec, we have
decimal
anddecimal128
. And in health insight we have decimal type used.https://microsoft.github.io/typespec/language-basics/built-in-types#numeric-types
TypeSpec definition for decimal:
We need to decide on how to support this type in both RLC and Modular.
The text was updated successfully, but these errors were encountered: