Write a completely custom JSON decoder/encoder #1262
Comments
alecthomas
added
intermediate
This PR: * Rips out the existing textMarshaler and jsonMarshaler usage from encoding.go. We may want to add those back for #1296 down the road, but we will need to be thoughtful about how we do that. Removing it for now keeps the logic much more predictable. * Moves the (un)marshaling logic for `ftl.Option` out of `option.go` and into `encoding.go`. * Special-cases both `time.Time` (the only stdlib type we currently support) and `ftl.Option`. Also `json.RawMessage` for _just_ encoding to preserve the existing `omitempty` behavior. * Fixes some existing issues where the Pointer unmarshaling wasn't actually working correctly * [eww] Adds a rather grotesque alternative to `Peek()` in `isNextTokenNull()` because json Decoder does not support Peek. * [eww] Makes the ftl.Option struct fields public so that they are settable by `encoding.go`. Suggestions welcome for both counts of [eww] above :) Fixes #1247. Addresses most of #1262, except `omitempty` is only working for json.RawMessage for now.
|
The work left to do on this ticket is the remaining |
|
I can't recall if this was specifically referring to ingress, or to the backend encoder, or both, but it feels like we could/should use it for both. |
|
Now that we've taken out the json marshaler logic in the custom encoder, we are nearly doing that already. That said, referring back to the title, the fact that we're still using |
|
I think I got a bit mixed up before - we should separate the two concerns. The ingress JSON encoding/decoding shouldn't be tightly coupled to the backend |
|
Ahhh got it, thanks for clarifying! I'll make a note to touch base with Lizzy on this when she's back. In the meantime, I did a quick review of #1432 and that doesn't talk about where the encoder is used, except that it's defined in go-runtime, so I think we're ok. Please let me know if I missed something, though - there were a lot of loose ends on this one |
Currently we delegate to the
encoding/jsonpackage under certain circumstances, which can do the wrong thing. We should also add support for,omitemptyincluding for any zero value (not just nil, as with the stdlib).The text was updated successfully, but these errors were encountered: