fix: fix several go-runtime JSON encoding issues

[deniseli]

This PR:

• Rips out the existing textMarshaler and jsonMarshaler usage from encoding.go. We may want to add those back for Allow FTL to support external types by utilising type widening #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.

[worstell]

maybe move this into isNextTokenNull?

[deniseli]

nice catch - fixed!

[worstell]

nit: i wonder if we should also handle consuming this token in isNextTokenNull (and maybe rename/change signature as needed) so callers don't need to manage this

i don't feel strongly!

[deniseli]

ooooo callbacks! Done! Looks way better now :D

[alecthomas]

Nice!

[alecthomas]

Are we using json.RawMessage somewhere internally? I ask because I'm assuming we should only be supporting types explicitly supported by FTL.

[deniseli]

We use it in ingress: https://github.com/TBD54566975/ftl/blob/main/backend/controller/ingress/response.go#L19

Without this, the omitempty behavior doesn't happen, and this Ingress test fails because "error": "" gets included in the response body, which also breaks the status code.

That's a really good point... should I update encoding to handle the omitempty next, so that this special case for json.RawMessage can be removed?

[alecthomas]

Mmmm... this doesn't seem like an improvement over keeping UnmarshalJSON() on ftl.Option does it? What's the rationale for doing that?

[deniseli]

Thanks for catching that... we originally did this to break a circular dependency between the encoding and ftl packages, but since call's dependency prevents that anyway, this is now pointless. cc @worstell so we can discuss

[deniseli]

One option is to move the logic back to option.go, undo the broken encapsulation, and then special case Option explicitly in the switch in encodeValue.

[alecthomas]

That sounds like a reasonable approach.

[worstell]

ah actually we needed to move the implementation into encoding.go to prepare for a following change that will pass context through to the custom encoder. the json marshaling overrides for option must call into the custom encoder to marshal/unmarshal the optional’s underlying value, and we don’t have a context to pass from these callers

[worstell]

a potential solution with callbacks—adding an interface like this in encoding.go:

type OptionMarshaler interface {
	Marshal(marshalSome func ... ) ([]byte, error)
	Unmarshal(unmarshalSome func ... ) error
}

implemented in optional.go (this just subs the ftlencoding calls from our current JSON overrides in option.go with the callbacks), e.g.:

func (o Option[T]) Marshal(marshalSome func(v any) ([]byte, error)) ([]byte, error) {
	if o.ok {
		return marshalSome(o.value) // was `ftlencoding.Marshal(o.value)`
	}
	return []byte("null"), nil
}

func (o *Option[T]) Unmarshal(data []byte, unmarshalSome func(data []byte, v any) error) error {
	if string(data) == "null" {
		o.ok = false
		return nil
	}
	if err := unmarshalSome(data, &o.value); err != nil { // was `ftlencoding.Unmarshal(data, &o.value)`
		return err
	}
	o.ok = true
	return nil
}

then encoding.go uses OptionMarshaler.Marshal/Unmarshal if the value implements OptionMarshaler. the callbacks it would pass would be the original Marshal/Unmarshal calls

[worstell]

this is implemented in #1431

[alecthomas]

The additional .(time.Time) is redundant here (and below), it's just getting cast back to an any anyway.

[deniseli]

Whoops, thanks for catching that! Fixed in #1424

[alecthomas]

This feels pretty hacky. I'd be tempted to write a thin layer over json.Decoder that supports peeking, rather than doing this.

[deniseli]

It is very very gross. We spent a bit of time working on wrapping json.Decoder, but the issue is that we can't just wrap Token() (returning the last Peek value when defined instead of what's at the top of the buffer) and add Peek because d.Decode will pop tokens off the underlying buffer, and if we've popped a token into the wrapper object, the underlying Decoder's Decode() function won't use that. So unless there's an alternative I haven't thought of, we'd effectively have to reimplement the whole json.Decoder. Do you have any ideas?

[alecthomas]

Ah right, that makes sense.

I think we want to ditch json.Decoder altogether, but that's not appropriate for this PR.

[alecthomas]

We really shouldn't be doing this, it violates encapsulation completely and it's a public type exposed to end users.

[deniseli]

I added a proposal for fixing this here: #1417 (comment)

What do you think? Pending agreement, I'll fix forward using that strategy.