Dave/support string to int float from json

[soolaimon]

We're incrementally converting our model system to sqlboiler and had some trouble because one of our API consumers was passing int values as strings in JSON, which the volatiletech/nullpackage didn't allow for.

The original guregu/null package has made updates to allow some flexibility when unmarshalling JSON, namely that number's can be passed as strings:
guregu@dcb5120

I've borrowed heavily from them and then adapted the changes to all of the number types you have added.

We're currently overriding the import in our sqlboiler.toml to use our fork and it's working just fine now (we're only using null.Int64).

[aarondl]

I wonder why this is desirable. Although guregu/null has adopted this it doesn't necessarily mean we should, it's pretty unclear why they added it from the commit message and no other documentation was referred to. It's also true that typically we haven't encouraged people to marshal their models directly although it is possible. Quite often it's the case that a marshaling layer needs to be added. Given that this in the norm, how does this use case (which requires a marshaling layer because there's an API consumer that does something bad) stand out above all others?

To me it's a feature that it's strongly typed and that if you try to marshal a string into an int it fails even if the string is "35" it fails with "this is not an int". This degrades that property in favor of compatibility with clients that have little notion of type safety which only seems like a workaround for allowing odd/bad/faulty software to exist elsewhere. This also penalizes people for coding correctly as there's extra reflect calls in here now adding overhead (albeit small).

This change also does more than advertised at least in the case of float32, though I can't imagine how this code can actually work:

	case map[string]interface{}:
		err = json.Unmarshal(data, &f.Float64)

Currently I'm not in favor of this change and I think it needs more discussion particularly around the use case. What does this support that we can't do in other ways? Why is it desirable to confuse this library with all this extra code and overhead to allow bad clients to exist, instead of adding code to solve the problem in the null package consumer's code directly?

Looking forward to hearing your thoughts.

[soolaimon]

I understand and actually share your concerns. We discussed them as well, and in our particular case, it made the most sense to make these changes, at least for now.

I PRed here in case you wanted them, and if you did, so I could stop overriding the package import on sqlboiler and use yours again once it was merged. That's my only skin in the game and I certainly sympathize with your reasons to not merge.

In the case of

case map[string]interface{}:
		err = json.Unmarshal(data, &f.Float64)

as you can probably imagine, this involved a lot of copy -> paste -> adapt from guregu/null. I removed this case from the other types but missed it here. I've removed it now.

Feel free to close this, unless others would like this change and want to make a stronger case for it.

Thanks!

[aarondl]

Let's leave it opened for others to weigh in on for now.

[ccakes]

Late to the party on this one - but one argument for this is if you're converting directly between database records and protobuf messages.

protojson encodes large ints and floats using strings. Adding ,string to the field tags in the boiler templates works ok for int64 but null.Int64 breaks without this change.