From cf302dbc6d9932afbc72567209df044cbcf3557d Mon Sep 17 00:00:00 2001 From: Tim Deeb-Swihart Date: Tue, 28 Nov 2023 13:06:47 -0800 Subject: [PATCH] Switch to explicitly enabling shorthand and improve tests I never tested longform JSON compatibility. This fixes that --- common/v1/payload_json.go | 21 ++++--- internal/temporalcommonv1/payload_json.go | 21 ++++--- .../temporalcommonv1/payload_json_test.go | 62 +++++++++++++++---- 3 files changed, 72 insertions(+), 32 deletions(-) diff --git a/common/v1/payload_json.go b/common/v1/payload_json.go index b72b91ba..637faf2b 100644 --- a/common/v1/payload_json.go +++ b/common/v1/payload_json.go @@ -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]. @@ -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 } @@ -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() @@ -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 @@ -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 diff --git a/internal/temporalcommonv1/payload_json.go b/internal/temporalcommonv1/payload_json.go index b72b91ba..637faf2b 100644 --- a/internal/temporalcommonv1/payload_json.go +++ b/internal/temporalcommonv1/payload_json.go @@ -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]. @@ -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 } @@ -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() @@ -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 @@ -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 diff --git a/internal/temporalcommonv1/payload_json_test.go b/internal/temporalcommonv1/payload_json_test.go index 2b200efa..3e6d5c74 100644 --- a/internal/temporalcommonv1/payload_json_test.go +++ b/internal/temporalcommonv1/payload_json_test.go @@ -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"), @@ -47,8 +49,9 @@ 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"), @@ -56,7 +59,10 @@ var tests = []struct { }, }, { 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{ @@ -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)) })