fsprojects / FSharp.Data

F# Data: Library for Data Access
https://fsprojects.github.io/FSharp.Data
Other
814 stars 288 forks source link

Always use the original json primitive type when serializing values #1477

Closed mlaily closed 1 year ago

mlaily commented 1 year ago

Before this PR, serializing a value inferred by the json type provider would always write it using its inferred type, regardless of its original primitive json type in the json sample.

Example:

type PT = JsonProvider<""" { "a": "42" } """> // int value in a string primitive type
PT.Root(42).JsonValue.ToString() // correctly inferred as an int

// incorrectly serialized to a json number
val it: string = "{
  "a": 42
}"

After this PR:

val it: string = "{
  "a": "42"
}"

This partially fixes #1271


Note: This PR is currently on top of the fix-json-dictionaries branch from #1476, itself on top of the rearrange-project-files branch from #1475 — I will rebase it on master when the other ones are merged.

EDIT: rebased and ready!

cartermp commented 1 year ago

I need to take a closer look but so far this seems great

mlaily commented 1 year ago

Thanks for taking a first look!

One thing to note: I struggled a little with the code quotations, and didn't manage to pass a DU case value directly to the runtime code, so I decided to pass it as an int...

I'm still unsure whether it's possible or not, but I started having doubts after I realized that the existing implementation of the InferedTypeTag does a similar thing, though it serializes its values as strings.

Feel free to ask questions if something is unclear.

mlaily commented 1 year ago

Hello,

Since I started working on a JsonProvider2 (https://github.com/fsprojects/FSharp.Data/issues/1478) and it seems to be going somewhere, would you prefer it if I closed this PR and only did the changes it contains in the new provider, keeping the original JsonProvider's behaviour untouched?

I think the changes are still valuable, but they are not enough on their own to make the original JsonProvider trustworthy.
The missing part is the serialization of null values (as null or as missing properties), but I'll only be doing that in JsonProvider2 because it's going to change the generated code somewhat more...

Let me know what you think.

cartermp commented 1 year ago

Hmm, yeah, let's close this. AFter second thought, this is a breaking change (even if it's the right one), and I'd rather be a little more whole-hog in doing a breaking change than just have a minor behavior difference.