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

SystemTextJson: Add option to reject null string values #87

Merged
merged 11 commits into from
Nov 30, 2022

Conversation

nordfjord
Copy link
Contributor

@nordfjord nordfjord commented Nov 28, 2022

Adds a StringConverter to the converter list by default. This converter will throw when deserializing a string that's set to null

Before this change this would return null:

serde.Deserialize<string>("null")

After this change the above throws. Also affects properties on records.


I'm also updating FsCheck to the 3.0.0-* branch because it doesn't emit null as a legal string

@bartelink
Copy link
Collaborator

bartelink commented Nov 28, 2022

TL;DR if this was off by default, I'd probably just merge it without much discussion. If I truly hated it as a default, I'd insist on it simply being changed to that. However, the truth is I'm torn.


In general I agree that apps are better off with null-valued Strings not getting into serialized data

I don't believe anything in github.com/jet will break based on it (but I had to check - e.g. the correlationId and causationId values from IEventData may be null, but https://github.com/jet/equinox/blob/master/src/Equinox.CosmosStore/CosmosStore.fs#L1200 will permit that

The elephant in the room is the fact that this is a breaking change for FsCodec an'd we're not in version 0.x. As we're in a phase where FsCodec and Equinox are in RC, that's less significant than it ordinarily would be (and it's not a binary breaking change). However, I can definitely picture people being surprised and ending up with broken production systems.

The simplest way to address this is to add an optional argument to Options.Create which lets one opt into this.

The next level is to make it a non-optional argument in Options.Create to force a decision. That's less desirable though

As it stands, this is a thus a pretty big change to force on people.

Other aspects:

  • The intention is for FsCodec to provide an equivalent experience OOTB in JSON.net and STJ to the max degree possible (e.g. Options are handled, DUs are not). Atm off the top of my head the notable diffs in the experience are:
    1. JSON.NET has the ugly DU support by default, STJ throws (ideally we'd probably throw - plugging that hole on the Newtonsoft side would be a bugfix to my mind)
    2. JSON.NET has broken list support, STJ just works (not sure about Map, but its advised against in the README)
    3. FsCodec.NewtonsoftJson has an opt in to exclude empty fields, it's not implemented on the STJ side
    4. On the STJ side, there's opt in support for default TypeSafeEnum encoding and the safe DU encoding (but that could be ported)
  • While the main principle is to give no surprises to the average dev that has used STJ and/or Newtonsoft but is not the unfortunately that has spent days writing the ugly serialization helpers in a medium to large size prod system and has hence been forced to know details on this sort of thing, there is decent docs coverage and someone scanning the README should come away understanding the key diffs between FsCodec and vanilla STJ/JSON.NET.

In my world, I tend to have two places in my app where I would be able to configure things (and I personally would immediately switch it on):

Thoughts, @deviousasti @enricosada @wantastic84 - do you have a preference for default off, default on, or force choice via having Options.Create adding a mandatory argument?


Conclusions:

  1. I do want this converter in the lib
  2. Whatever it is that's concluded, it needs to be well-documented in the xmldoc and in the readme

These are the doc sections this should be covered in (perhaps a dedicated section near the top with an anchor that can be referenced by the sections might help?)

@bartelink
Copy link
Collaborator

OK now that the default is flipped, the main remaining thing other than the minor review comment things is to mention the converter in the README in the cited places (perhaps with a small section illustrating how string option can be used in a messy case where the input is conditional in some input?)

Am still slightly torn on what the default should be, but I guess the simplification of the PR shows its probably for the best...

@bartelink bartelink changed the title disallow null strings by default SystemTextJson: Add option to reject null string values Nov 30, 2022
@bartelink
Copy link
Collaborator

Looks great, thanks!
@enricosada @deviousasti @wantastic84 thoughts on whether this should be a default?

@bartelink bartelink merged commit b66b91e into jet:master Nov 30, 2022
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

Successfully merging this pull request may close these issues.

2 participants