Refactor and write tests for JsonHelper in DataController

[ivarne]

In testing our new app, we discovered that the app would crash if we used really large numbers in a numeric field. While investigating I found JsonHelper which kind of begs for a rewrite, so I thought the good first step would be to write some tests for it.

The cause for our (testers) issue is that JsonHelper returns System.Numeric.BigInteger, for numbers larger than long.MaxValue, and the json serialization library does not support serialzing BigInteger, so the app crashes.

Related Issue(s)

• Not sure if this has been reported

Verification

• Your code builds clean without any errors or warnings
• Manual testing done (required)
• Relevant automated test added (if you find this hard, leave it and we'll help out)
• All tests run green

[ivarne]

After seeing that the tests work. I added commit 4eda33a for a suggested change that solves the problem of crashes caused by calculationResult.ChangedFields containing BigInt serialized content {"IsPowerOfTwo":true,"IsZero":false,"IsOne":false,"IsEven":true,"Sign":1} instead of a sensible number.

[ivarne]

A better approach would probably be to ensure that the diff is delivered as a Dictionary<string, string?> instead of Dictionary<string, object?> so that we get better control over serialization of different types.

[tjololo]

I agree that changing to Dictinoary<string, string?> would make sense, then again that is a change of the rest-api contract so not sure how the frontend consuming this would react, any insights on that @olemartinorg ?
The change as it stands now looks good to me we should probably create an issue for v8 of the nugets to change to Dictionary<string, string?>. Its not a hard breaking change as string? is a sub-type of object? but a change in the api nonetheless

[olemartinorg]

We're actively working on supporting other data-types than strings in app-frontend, and JsonHelper is very much relied upon in app-frontend (and the diff is in fact pretty much equivalent to the internal representation of the data model in app-frontend).

How about we simplify all of this in the future by simply returning the entire updated data model. App-frontend can bear the burden of diffing what it sendt and what it received back, merging those changes into its own internal representation of the data model.

I hope we can find a solution to the bigint problem in isolation without rewriting JsonHelper (can it just be returned as string?).

[tjololo]

this change should resolve the bigint problem. With this change JsonHelper checks if the value is larger than max value of decimal, if the number "fits" in a decimal it returns a decimal if not it returns a string.
Your suggestion seems like a huge simplification on the backend 🥳 😉

[olemartinorg]

Yup, I haven't looked into the code in detail - but if it doesn't change anything regarding the contract with app-frontend, I'm all for it. JavaScript has a bigint type, but not sure it's possible to represent that in JSON.

Forgot to link the relevant work in app-frontend in my last comment:

• Simplifying flattenObject(), adding support for non-string types app-frontend-react#738

[tjololo]

Yup, I haven't looked into the code in detail - but if it doesn't change anything regarding the contract with app-frontend, I'm all for it. JavaScript has a bigint type, but not sure it's possible to represent that in JSON.

Forgot to link the relevant work in app-frontend in my last comment:

• Simplifying flattenObject(), adding support for non-string types app-frontend-react#738

Contracts the same, but backend will return bigint as either decimal or string depending on the size of the number. If you don't see any issues with that I think we should just merge this

[ivarne]

Just one further clarification. It is not the model class that uses BigInt (in our case). The JsonHelper.FindChangedFields gets serialized json and uses Newtonsoft JToken parser that represents large numbers as BigInt, that is creating the issue.

[tjololo]

Just one further clarification. It is not the model class that uses BigInt (in our case). The JsonHelper.FindChangedFields gets serialized json and uses Newtonsoft JToken parser that represents large numbers as BigInt, that is creating the issue.

👍 thanks! Hope my comments about breaking/non-breaking sounds reasonable to you?

[ivarne]

Yes, it definitely makes sense. The question is if this is really an api that should offer that kind of stability, or if we can safely assume that no one has implemented a tool for delivering schemas that depends on the diff returned from this api. I don't know the answer, but it would be good to have some sort of documentation that explains what apis are specific for app frontend (ie. subject to change), and which one we expect third parties to be able to leverage.

[olemartinorg]

Good point. I happen to know that @DanRJ used the tool in OED (directly in backend code) to compare some values. Assuming the external interfaces hasn't changed, and if they did, their code would fail to compile (and thus this would be picked up during testing).

[tjololo]

Agreed. As we currently haven't identified or communicated what APIs are internal and subject to change, and have no good way of telling if someone has written custom implementations against them. I think the clean way of marking them as internal will be when we release a new major, we don't change the api, but we are rebranding it as internal to allow us to change them across minor/patch versions.
If you agree I think we should merge this as is and create an issue for identifying and marking api methods as internal, and how we communicate this to the consumer