Skip to content

Commit

Permalink
Switch to explicitly enabling shorthand and improve tests
Browse files Browse the repository at this point in the history
I never tested longform JSON compatibility. This fixes that
  • Loading branch information
tdeebswihart committed Nov 28, 2023
1 parent 7885765 commit cf302db
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 32 deletions.
21 changes: 11 additions & 10 deletions common/v1/payload_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,10 @@ func marshal(enc *json.Encoder, value interface{}) error {
return marshalValue(enc, reflect.ValueOf(value))
}

// Key on the marshaler metadata specifying whether shorthand is disabled.
// Key on the marshaler metadata specifying whether shorthand is enabled.
//
// WARNING: This is internal API and should not be called externally.
const DisablePayloadShorthandMetadataKey = "__temporal_disable_payload_shorthand"
const EnablePayloadShorthandMetadataKey = "__temporal_enable_payload_shorthand"

// MaybeMarshalProtoJSON implements
// [go.temporal.io/api/internal/temporaljsonpb.ProtoJSONMaybeMarshaler.MaybeMarshalProtoJSON].
Expand All @@ -255,8 +255,9 @@ func (p *Payloads) MaybeMarshalProtoJSON(meta map[string]interface{}, enc *json.
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {

// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}

Expand Down Expand Up @@ -289,8 +290,8 @@ func (p *Payloads) MaybeUnmarshalProtoJSON(meta map[string]interface{}, dec *jso
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {
// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}
tok, err := dec.Peek()
Expand Down Expand Up @@ -331,8 +332,8 @@ func (p *Payload) MaybeMarshalProtoJSON(meta map[string]interface{}, enc *json.E
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {
// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}
// If any are not handled or there is an error, return
Expand All @@ -352,8 +353,8 @@ func (p *Payload) MaybeUnmarshalProtoJSON(meta map[string]interface{}, dec *json
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {
// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}
// Always considered handled, unmarshaler ignored (unknown fields always
Expand Down
21 changes: 11 additions & 10 deletions internal/temporalcommonv1/payload_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,10 @@ func marshal(enc *json.Encoder, value interface{}) error {
return marshalValue(enc, reflect.ValueOf(value))
}

// Key on the marshaler metadata specifying whether shorthand is disabled.
// Key on the marshaler metadata specifying whether shorthand is enabled.
//
// WARNING: This is internal API and should not be called externally.
const DisablePayloadShorthandMetadataKey = "__temporal_disable_payload_shorthand"
const EnablePayloadShorthandMetadataKey = "__temporal_enable_payload_shorthand"

// MaybeMarshalProtoJSON implements
// [go.temporal.io/api/internal/temporaljsonpb.ProtoJSONMaybeMarshaler.MaybeMarshalProtoJSON].
Expand All @@ -255,8 +255,9 @@ func (p *Payloads) MaybeMarshalProtoJSON(meta map[string]interface{}, enc *json.
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {

// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}

Expand Down Expand Up @@ -289,8 +290,8 @@ func (p *Payloads) MaybeUnmarshalProtoJSON(meta map[string]interface{}, dec *jso
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {
// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}
tok, err := dec.Peek()
Expand Down Expand Up @@ -331,8 +332,8 @@ func (p *Payload) MaybeMarshalProtoJSON(meta map[string]interface{}, enc *json.E
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {
// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}
// If any are not handled or there is an error, return
Expand All @@ -352,8 +353,8 @@ func (p *Payload) MaybeUnmarshalProtoJSON(meta map[string]interface{}, dec *json
if p == nil {
return false, nil
}
// If shorthand is disabled, ignore
if disabled, _ := meta[DisablePayloadShorthandMetadataKey].(bool); disabled {
// Skip unless explicitly enabled
if _, enabled := meta[EnablePayloadShorthandMetadataKey].(bool); !enabled {
return false, nil
}
// Always considered handled, unmarshaler ignored (unknown fields always
Expand Down
62 changes: 50 additions & 12 deletions internal/temporalcommonv1/payload_json_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@ import (
)

var tests = []struct {
name string
pb proto.Message
json string
name string
pb proto.Message
longformJSON string
shorthandJSON string
}{{
name: "json/plain",
json: `{"_protoMessageType": "temporal.api.common.v1.WorkflowType", "name": "workflow-name"}`,
name: "json/plain",
longformJSON: `{"metadata":{"encoding":"anNvbi9wcm90b2J1Zg==","messageType":"dGVtcG9yYWwuYXBpLmNvbW1vbi52MS5Xb3JrZmxvd1R5cGU="},"data":"eyJuYW1lIjoid29ya2Zsb3ctbmFtZSJ9"}`,
shorthandJSON: `{"_protoMessageType": "temporal.api.common.v1.WorkflowType", "name": "workflow-name"}`,
pb: &common.Payload{
Metadata: map[string][]byte{
"encoding": []byte("json/protobuf"),
Expand All @@ -47,16 +49,20 @@ var tests = []struct {
Data: []byte(`{"name":"workflow-name"}`),
},
}, {
name: "binary/null",
json: `{"metadata":{"encoding":"YmluYXJ5L251bGw="}}`,
name: "binary/null",
longformJSON: `{"metadata":{"encoding":"YmluYXJ5L251bGw="}}`,
shorthandJSON: `{"metadata":{"encoding":"YmluYXJ5L251bGw="}}`,
pb: &common.Payload{
Metadata: map[string][]byte{
"encoding": []byte("binary/null"),
},
},
}, {
name: "memo with varying fields",
json: `{"fields": {
longformJSON: `{"fields":{
"some-string":{"metadata":{"encoding":"anNvbi9wbGFpbg=="},"data":"InN0cmluZyI="},
"some-array":{"metadata":{"encoding":"anNvbi9wbGFpbg=="},"data":"WyJmb28iLDEyMyxmYWxzZV0="}}}`,
shorthandJSON: `{"fields": {
"some-string": "string",
"some-array": ["foo", 123, false]}}`,
pb: &common.Memo{
Expand All @@ -78,24 +84,56 @@ var tests = []struct {
},
}}

func TestMaybeMarshal(t *testing.T) {
func TestMaybeMarshal_ShorthandEnabled(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
opts := temporalproto.CustomJSONMarshalOptions{
Metadata: map[string]interface{}{
common.EnablePayloadShorthandMetadataKey: true,
},
}
got, err := opts.Marshal(tt.pb)
require.NoError(t, err)
t.Logf("%s", string(got))
require.JSONEq(t, tt.shorthandJSON, string(got))
})
}
}

func TestMaybeMarshal_ShorthandDisabled(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var opts temporalproto.CustomJSONMarshalOptions
got, err := opts.Marshal(tt.pb)
require.NoError(t, err)
t.Logf("%s", string(got))
require.JSONEq(t, tt.json, string(got), tt.pb)
require.JSONEq(t, tt.longformJSON, string(got))
})
}
}

func TestMaybeUnmarshal_Shorthand(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var out = tt.pb.ProtoReflect().New().Interface()
opts := temporalproto.CustomJSONUnmarshalOptions{
Metadata: map[string]interface{}{
common.EnablePayloadShorthandMetadataKey: true,
},
}
err := opts.Unmarshal([]byte(tt.shorthandJSON), out)
require.NoError(t, err)
require.True(t, proto.Equal(tt.pb, out))
})
}
}

func TestMaybeUnmarshal(t *testing.T) {
func TestMaybeUnmarshal_Longform(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var out = tt.pb.ProtoReflect().New().Interface()
var opts temporalproto.CustomJSONUnmarshalOptions
err := opts.Unmarshal([]byte(tt.json), out)
err := opts.Unmarshal([]byte(tt.longformJSON), out)
require.NoError(t, err)
require.True(t, proto.Equal(tt.pb, out))
})
Expand Down

0 comments on commit cf302db

Please sign in to comment.