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.

Sign up for GitHub

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

fix: fix several go-runtime JSON encoding issues #1417

Merged
merged 2 commits into from
May 7, 2024
Merged

fix: fix several go-runtime JSON encoding issues #1417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47f45f7
init
deniseli May 2, 2024
020514c
add explicit UTC
deniseli May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 104 additions & 57 deletions go-runtime/encoding/encoding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,18 @@ package encoding

import (
"bytes"
"encoding"
"encoding/base64"
"encoding/json"
"fmt"
"io"
"reflect"
"strings"
"time"
"unicode"

"github.com/TBD54566975/ftl/backend/schema/strcase"
)

var (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

textMarshaler = reflect.TypeOf((*encoding.TextMarshaler)(nil)).Elem()
textUnmarshaler = reflect.TypeOf((*encoding.TextUnmarshaler)(nil)).Elem()
jsonMarshaler = reflect.TypeOf((*json.Marshaler)(nil)).Elem()
jsonUnmarshaler = reflect.TypeOf((*json.Unmarshaler)(nil)).Elem()
)

func Marshal(v any) ([]byte, error) {
w := &bytes.Buffer{}
err := encodeValue(reflect.ValueOf(v), w)
Expand All @@ -31,37 +27,29 @@ func encodeValue(v reflect.Value, w *bytes.Buffer) error {
w.WriteString("null")
return nil
}

t := v.Type()
switch {
case t.Kind() == reflect.Ptr && t.Elem().Implements(jsonMarshaler):
v = v.Elem()
fallthrough

case t.Implements(jsonMarshaler):
enc := v.Interface().(json.Marshaler) //nolint:forcetypeassert
data, err := enc.MarshalJSON()
// Special-cased types
switch {
case t == reflect.TypeFor[time.Time]():
data, err := json.Marshal(v.Interface().(time.Time))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, thanks for catching that! Fixed in #1424

if err != nil {
return err
}
w.Write(data)
return nil

case t.Kind() == reflect.Ptr && t.Elem().Implements(textMarshaler):
v = v.Elem()
fallthrough

case t.Implements(textMarshaler):
enc := v.Interface().(encoding.TextMarshaler) //nolint:forcetypeassert
data, err := enc.MarshalText()
if err != nil {
return err
}
data, err = json.Marshal(string(data))
case t == reflect.TypeFor[json.RawMessage]():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

data, err := json.Marshal(v.Interface().(json.RawMessage))
if err != nil {
return err
}
w.Write(data)
return nil

case isOption(v.Type()):
return encodeOption(v, w)
}

switch v.Kind() {
Expand Down Expand Up @@ -107,6 +95,24 @@ func encodeValue(v reflect.Value, w *bytes.Buffer) error {
}
}

var ftlOptionTypePath = "github.com/TBD54566975/ftl/go-runtime/ftl.Option"

func isOption(t reflect.Type) bool {
return strings.HasPrefix(t.PkgPath()+"."+t.Name(), ftlOptionTypePath)
}

func encodeOption(v reflect.Value, w *bytes.Buffer) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a reasonable approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@worstell worstell May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is implemented in #1431

if v.NumField() != 2 {
return fmt.Errorf("value cannot have type ftl.Option since it has %d fields rather than 2: %v", v.NumField(), v)
}
optionOk := v.Field(1).Bool()
if !optionOk {
w.WriteString("null")
return nil
}
return encodeValue(v.Field(0), w)
}

func encodeStruct(v reflect.Value, w *bytes.Buffer) error {
w.WriteRune('{')
afterFirst := false
Expand Down Expand Up @@ -213,50 +219,34 @@ func Unmarshal(data []byte, v any) error {

func decodeValue(d *json.Decoder, v reflect.Value) error {
if !v.CanSet() {
return fmt.Errorf("cannot set value")
allBytes, _ := io.ReadAll(d.Buffered())
return fmt.Errorf("cannot set value: %v", string(allBytes))
}

t := v.Type()
switch {
case v.Kind() != reflect.Ptr && v.CanAddr() && v.Addr().Type().Implements(jsonUnmarshaler):
v = v.Addr()
fallthrough

case t.Implements(jsonUnmarshaler):
if v.IsNil() {
v.Set(reflect.New(t.Elem()))
}
o := v.Interface()
return d.Decode(&o)

case v.Kind() != reflect.Ptr && v.CanAddr() && v.Addr().Type().Implements(textUnmarshaler):
v = v.Addr()
fallthrough

case t.Implements(textUnmarshaler):
if v.IsNil() {
v.Set(reflect.New(t.Elem()))
}
dec := v.Interface().(encoding.TextUnmarshaler) //nolint:forcetypeassert
var s string
if err := d.Decode(&s); err != nil {
return err
}
return dec.UnmarshalText([]byte(s))
// Special-case types
switch {
case t == reflect.TypeFor[time.Time]():
return d.Decode(v.Addr().Interface())
case isOption(v.Type()):
return decodeOption(d, v)
}

switch v.Kind() {
case reflect.Struct:
return decodeStruct(d, v)

case reflect.Ptr:
if token, err := d.Token(); err != nil {
return err
} else if token == nil {
return handleIfNextTokenIsNull(d, func(d *json.Decoder) error {
v.Set(reflect.Zero(v.Type()))
return nil
}
return decodeValue(d, v.Elem())
}, func(d *json.Decoder) error {
if v.IsNil() {
v.Set(reflect.New(v.Type().Elem()))
}
return decodeValue(d, v.Elem())
})

case reflect.Slice:
if v.Type().Elem().Kind() == reflect.Uint8 {
Expand All @@ -278,6 +268,63 @@ func decodeValue(d *json.Decoder, v reflect.Value) error {
}
}

func handleIfNextTokenIsNull(d *json.Decoder, ifNullFn func(*json.Decoder) error, elseFn func(*json.Decoder) error) error {
isNull, err := isNextTokenNull(d)
if err != nil {
return err
}
if isNull {
err = ifNullFn(d)
if err != nil {
return err
}
// Consume the null token
_, err := d.Token()
if err != nil {
return err
}
return nil
}
return elseFn(d)
}

// isNextTokenNull implements a cheap/dirty version of `Peek()`, which json.Decoder does
// not support.
func isNextTokenNull(d *json.Decoder) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, that makes sense.

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

s, err := io.ReadAll(d.Buffered())
if err != nil {
return false, err
}
if len(s) == 0 {
return false, fmt.Errorf("cannot check emptystring for token \"null\"")
}
if s[0] != ':' {
return false, fmt.Errorf("cannot check emptystring for token \"null\"")
}
i := 1
for len(s) > i && unicode.IsSpace(rune(s[i])) {
i++
}
if len(s) < i+4 {
return false, nil
}
return string(s[i:i+4]) == "null", nil
}

func decodeOption(d *json.Decoder, v reflect.Value) error {
return handleIfNextTokenIsNull(d, func(d *json.Decoder) error {
v.FieldByName("Okay").SetBool(false)
return nil
}, func(d *json.Decoder) error {
err := decodeValue(d, v.FieldByName("Val"))
if err != nil {
return err
}
v.FieldByName("Okay").SetBool(true)
return nil
})
}

func decodeStruct(d *json.Decoder, v reflect.Value) error {
if err := expectDelim(d, '{'); err != nil {
return err
Expand Down
14 changes: 14 additions & 0 deletions go-runtime/encoding/encoding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package encoding_test
import (
"reflect"
"testing"
"time"

"github.com/alecthomas/assert/v2"

Expand Down Expand Up @@ -31,6 +32,8 @@ func TestMarshal(t *testing.T) {
{name: "SliceOfStrings", input: struct{ Slice []string }{[]string{"hello", "world"}}, expected: `{"slice":["hello","world"]}`},
{name: "Map", input: struct{ Map map[string]int }{map[string]int{"foo": 42}}, expected: `{"map":{"foo":42}}`},
{name: "Option", input: struct{ Option ftl.Option[int] }{ftl.Some(42)}, expected: `{"option":42}`},
{name: "OptionNull", input: struct{ Option ftl.Option[int] }{ftl.None[int]()}, expected: `{"option":null}`},
{name: "OptionZero", input: struct{ Option ftl.Option[int] }{ftl.Some(0)}, expected: `{"option":0}`},
{name: "OptionPtr", input: struct{ Option *ftl.Option[int] }{&somePtr}, expected: `{"option":42}`},
{name: "OptionStruct", input: struct{ Option ftl.Option[inner] }{ftl.Some(inner{"foo"})}, expected: `{"option":{"fooBar":"foo"}}`},
{name: "Unit", input: ftl.Unit{}, expected: `{}`},
Expand Down Expand Up @@ -69,6 +72,9 @@ func TestUnmarshal(t *testing.T) {
{name: "Slice", input: `{"slice":[1,2,3]}`, expected: struct{ Slice []int }{[]int{1, 2, 3}}},
{name: "SliceOfStrings", input: `{"slice":["hello","world"]}`, expected: struct{ Slice []string }{[]string{"hello", "world"}}},
{name: "Map", input: `{"map":{"foo":42}}`, expected: struct{ Map map[string]int }{map[string]int{"foo": 42}}},
{name: "OptionNull", input: `{"option":null}`, expected: struct{ Option ftl.Option[int] }{ftl.None[int]()}},
{name: "OptionNullWhitespace", input: `{"option": null}`, expected: struct{ Option ftl.Option[int] }{ftl.None[int]()}},
{name: "OptionZero", input: `{"option":0}`, expected: struct{ Option ftl.Option[int] }{ftl.Some(0)}},
{name: "Option", input: `{"option":42}`, expected: struct{ Option ftl.Option[int] }{ftl.Some(42)}},
{name: "OptionPtr", input: `{"option":42}`, expected: struct{ Option *ftl.Option[int] }{&somePtr}},
{name: "OptionStruct", input: `{"option":{"fooBar":"foo"}}`, expected: struct{ Option ftl.Option[inner] }{ftl.Some(inner{"foo"})}},
Expand All @@ -77,6 +83,12 @@ func TestUnmarshal(t *testing.T) {
String string
Unit ftl.Unit
}{String: "something", Unit: ftl.Unit{}}},
// Whitespaces after each `:` and multiple fields to test handling of the
// two potential terminal delimiters: `}` and `,`
{name: "ComplexFormatting", input: `{"option": null, "bool": true}`, expected: struct {
Option ftl.Option[int]
Bool bool
}{ftl.None[int](), true}},
}

for _, tt := range tests {
Expand Down Expand Up @@ -111,7 +123,9 @@ func TestRoundTrip(t *testing.T) {
{name: "Slice", input: struct{ Slice []int }{[]int{1, 2, 3}}},
{name: "SliceOfStrings", input: struct{ Slice []string }{[]string{"hello", "world"}}},
{name: "Map", input: struct{ Map map[string]int }{map[string]int{"foo": 42}}},
{name: "Time", input: struct{ Time time.Time }{time.Date(2009, time.November, 29, 21, 33, 0, 0, time.UTC)}},
{name: "Option", input: struct{ Option ftl.Option[int] }{ftl.Some(42)}},
{name: "OptionNull", input: struct{ Option ftl.Option[int] }{ftl.None[int]()}},
{name: "OptionPtr", input: struct{ Option *ftl.Option[int] }{&somePtr}},
{name: "OptionStruct", input: struct{ Option ftl.Option[inner] }{ftl.Some(inner{"foo"})}},
{name: "Unit", input: ftl.Unit{}},
Expand Down
Loading