From 4731aeb6ad85c04160b30232e85e3f8a43c712a6 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Wed, 17 Feb 2021 14:08:13 +0200 Subject: [PATCH] fix(go-runtime): enums are not encoded/decoded correctly (#2585) Enum values are represented in the jsii kernel as `{ "$jsii.enum": "fqn/member" }`, so we need to marshal/unmarshall them based on this encoding. Change the type registry to capture a mapping between FQN to enum member consts and type to enum FQNs. Fixes #2534 Co-authored-by: Romain Marcadier --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --- .../content/specification/2-type-system.md | 4 +- .../go-runtime/jsii-calc-test/main_test.go | 40 ++++++++++ .../@jsii/go-runtime/jsii-runtime-go/api.go | 16 ++-- .../go-runtime/jsii-runtime-go/runtime.go | 74 ++++++++++++++---- .../jsii-runtime-go/type-registry.go | 76 ++++++++++++++----- 5 files changed, 170 insertions(+), 40 deletions(-) diff --git a/gh-pages/content/specification/2-type-system.md b/gh-pages/content/specification/2-type-system.md index 5bad34a67a..1d774ce0b7 100644 --- a/gh-pages/content/specification/2-type-system.md +++ b/gh-pages/content/specification/2-type-system.md @@ -230,10 +230,10 @@ value: In **JavaScript**, `enum` entries are represented by their value equivalent. In order to support statically typed representations in other languages, these are serialized using a dedicated wrapper object, using a single key -(`$jsii.enum`) with the fully qualified name of the `enum` entry: +(`$jsii.enum`) with the fully qualified name of the `enum` entry (formatted as `/`): ```json -{ "$jsii.enum": "@scope/module.EnumType.ENTRY_NAME" } +{ "$jsii.enum": "@scope/module.EnumType/ENTRY_NAME" } ``` ### Identity Serialization diff --git a/packages/@jsii/go-runtime/jsii-calc-test/main_test.go b/packages/@jsii/go-runtime/jsii-calc-test/main_test.go index d19d5e48ac..26a0efe5c3 100644 --- a/packages/@jsii/go-runtime/jsii-calc-test/main_test.go +++ b/packages/@jsii/go-runtime/jsii-calc-test/main_test.go @@ -157,6 +157,46 @@ func TestAllTypes(t *testing.T) { }) } +func TestEnumUnmarshal(t *testing.T) { + actual := calc.EnumDispenser_RandomStringLikeEnum() + if actual != calc.StringEnumB { + t.Errorf("Expected StringEnum.B. Actual: %s", actual) + } +} + +func TestEnumRoundtrip(t *testing.T) { + allTypes := calc.NewAllTypes() + actual := allTypes.EnumMethod(calc.StringEnumA) + if actual != calc.StringEnumA { + t.Errorf("Expected StringEnum.A. Actual: %s", actual) + } + + actual = allTypes.EnumMethod(calc.StringEnumC) + if actual != calc.StringEnumC { + t.Errorf("Expected StringEnum.C. Actual: %s", actual) + } +} + +func TestOptionalEnums(t *testing.T) { + allTypes := calc.NewAllTypes() + actual := allTypes.GetOptionalEnumValue() + if actual != "" { + t.Error("Expected value to be nil") + } + + allTypes.SetOptionalEnumValue(calc.StringEnumB) + actual = allTypes.GetOptionalEnumValue() + if actual != calc.StringEnumB { + t.Errorf("Expected StringEnum.B. Actual: %s", actual) + } + + allTypes.SetOptionalEnumValue("") + actual = allTypes.GetOptionalEnumValue() + if actual != "" { + t.Error("Expected value to be nil") + } +} + func TestReturnsSpecialParam(t *testing.T) { retSpecialParam := returnsParam.NewReturnsSpecialParameter() val := retSpecialParam.ReturnsSpecialParam() diff --git a/packages/@jsii/go-runtime/jsii-runtime-go/api.go b/packages/@jsii/go-runtime/jsii-runtime-go/api.go index ca8dafa357..8d47f68432 100644 --- a/packages/@jsii/go-runtime/jsii-runtime-go/api.go +++ b/packages/@jsii/go-runtime/jsii-runtime-go/api.go @@ -85,10 +85,14 @@ func (r kernelResponder) isResponse() { return } -type objref struct { +type objectRef struct { InstanceID string `json:"$jsii.byref"` } +type enumRef struct { + MemberFQN string `json:"$jsii.enum"` +} + type loadRequest struct { kernelRequester @@ -126,7 +130,7 @@ type delRequest struct { kernelRequester API string `json:"api"` - ObjRef objref `json:"objref"` + ObjRef objectRef `json:"objref"` } type delResponse struct { @@ -138,7 +142,7 @@ type getRequest struct { API string `json:"api"` Property string `json:"property"` - ObjRef objref `json:"objref"` + ObjRef objectRef `json:"objref"` } type staticGetRequest struct { @@ -161,7 +165,7 @@ type setRequest struct { API string `json:"api"` Property string `json:"property"` Value interface{} `json:"value"` - ObjRef objref `json:"objref"` + ObjRef objectRef `json:"objref"` } type staticSetRequest struct { @@ -192,7 +196,7 @@ type invokeRequest struct { API string `json:"api"` Method string `json:"method"` Arguments []interface{} `json:"args"` - ObjRef objref `json:"objref"` + ObjRef objectRef `json:"objref"` } type invokeResponse struct { @@ -207,7 +211,7 @@ type beginRequest struct { API string `json:"api"` Method *string `json:"method"` Arguments []interface{} `json:"args"` - ObjRef objref `json:"objref"` + ObjRef objectRef `json:"objref"` } type beginResponse struct { diff --git a/packages/@jsii/go-runtime/jsii-runtime-go/runtime.go b/packages/@jsii/go-runtime/jsii-runtime-go/runtime.go index cfa9142ba9..4b1ec80f29 100644 --- a/packages/@jsii/go-runtime/jsii-runtime-go/runtime.go +++ b/packages/@jsii/go-runtime/jsii-runtime-go/runtime.go @@ -117,7 +117,7 @@ func Invoke(obj interface{}, method string, args []interface{}, hasReturn bool, API: "invoke", Method: method, Arguments: castPtrsToRef(args), - ObjRef: objref{ + ObjRef: objectRef{ InstanceID: refid, }, }) @@ -167,7 +167,7 @@ func Get(obj interface{}, property string, ret interface{}) { res, err := client.get(getRequest{ API: "get", Property: property, - ObjRef: objref{ + ObjRef: objectRef{ InstanceID: refid, }, }) @@ -213,7 +213,7 @@ func Set(obj interface{}, property string, value interface{}) { API: "set", Property: property, Value: castPtrToRef(value), - ObjRef: objref{ + ObjRef: objectRef{ InstanceID: refid, }, }) @@ -240,8 +240,8 @@ func StaticSet(fqn FQN, property string, value interface{}) { } } -func castValToRef(data interface{}) (objref, bool) { - ref := objref{} +func castValToRef(data interface{}) (objectRef, bool) { + ref := objectRef{} ok := false dataVal := reflect.ValueOf(data) @@ -262,6 +262,27 @@ func castValToRef(data interface{}) (objref, bool) { return ref, ok } +func castValToEnumRef(data interface{}) (enum enumRef, ok bool) { + dataVal := reflect.ValueOf(data) + ok = false + + if dataVal.Kind() == reflect.Map { + for _, k := range dataVal.MapKeys() { + // Finding values type requires extracting from reflect.Value + // otherwise .Kind() returns `interface{}` + v := reflect.ValueOf(dataVal.MapIndex(k).Interface()) + + if k.Kind() == reflect.String && k.String() == "$jsii.enum" && v.Kind() == reflect.String { + enum.MemberFQN = v.String() + ok = true + return + } + } + } + + return +} + // Accepts pointers to structs that implement interfaces and searches for an // existing object reference in the client. If it exists, it casts it to an // objref for the runtime. Recursively casts types that may contain nested @@ -273,7 +294,11 @@ func castPtrToRef(data interface{}) interface{} { if dataVal.Kind() == reflect.Ptr { valref, valHasRef := client.findObjectRef(data) if valHasRef { - return objref{InstanceID: valref} + return objectRef{InstanceID: valref} + } + } else if dataVal.Kind() == reflect.String { + if enumRef, isEnumRef := client.types.tryRenderEnumRef(dataVal); isEnumRef { + return enumRef } } else if dataVal.Kind() == reflect.Slice { refs := make([]interface{}, dataVal.Len()) @@ -308,7 +333,28 @@ func (c *client) castAndSetToPtr(ptr interface{}, data interface{}) { dataVal := reflect.ValueOf(data) ref, isRef := castValToRef(data) + if isRef { + // If return data is JSII object references, add to objects table. + if concreteType, err := c.types.concreteTypeFor(ptrVal.Type()); err == nil { + ptrVal.Set(reflect.New(concreteType)) + c.objects[ptrVal.Interface()] = ref.InstanceID + } else { + panic(err) + } + return + } + + if enumref, isEnum := castValToEnumRef(data); isEnum { + member, err := c.types.enumMemberForEnumRef(enumref) + if err != nil { + panic(err) + } + + ptrVal.Set(reflect.ValueOf(member)) + return + } + // arrays if ptrVal.Kind() == reflect.Slice && dataVal.Kind() == reflect.Slice { // If return type is a slice, recursively cast elements for i := 0; i < dataVal.Len(); i++ { @@ -318,15 +364,13 @@ func (c *client) castAndSetToPtr(ptr interface{}, data interface{}) { c.castAndSetToPtr(inner.Interface(), dataVal.Index(i).Interface()) ptrVal.Set(reflect.Append(ptrVal, inner.Elem())) } - } else if isRef { - // If return data is JSII object references, add to objects table. - concreteType, err := c.types.concreteTypeFor(ptrVal.Type()) - if err != nil { - panic(err) - } - ptrVal.Set(reflect.New(concreteType)) - c.objects[ptrVal.Interface()] = ref.InstanceID - } else { + + return + } + + // TODO: maps + + if data != nil { val := reflect.ValueOf(data) ptrVal.Set(val) } diff --git a/packages/@jsii/go-runtime/jsii-runtime-go/type-registry.go b/packages/@jsii/go-runtime/jsii-runtime-go/type-registry.go index 4fd673dc09..1379cd9d8c 100644 --- a/packages/@jsii/go-runtime/jsii-runtime-go/type-registry.go +++ b/packages/@jsii/go-runtime/jsii-runtime-go/type-registry.go @@ -13,6 +13,7 @@ type typeRegistry struct { // qualified type name. The kind of type being returned depends on what the // FQN represents... This will be the second argument of provided to a // register* function. + // enums are not included fqnToType map[FQN]reflect.Type // ifaceToStruct provides the go struct that should be used to build a proxy @@ -20,17 +21,22 @@ type typeRegistry struct { // conversion. ifaceToStruct map[reflect.Type]reflect.Type - // enumMembers has enum constants for each registered enum type, supporting - // correct conversion of enum values received from JavaScript. - enumMembers map[reflect.Type]map[string]interface{} + // map enum member FQNs (e.g. "jsii-calc.StringEnum/A") to the corresponding + // go const for this member. + fqnToEnumMember map[string]interface{} + + // maps Go enum type ("StringEnum") to the corresponding jsii enum FQN (e.g. + // "jsii-calc.StringEnum") + typeToEnumFQN map[reflect.Type]FQN } // newTypeRegistry creates a new type registry. func newTypeRegistry() *typeRegistry { return &typeRegistry{ - fqnToType: make(map[FQN]reflect.Type), - ifaceToStruct: make(map[reflect.Type]reflect.Type), - enumMembers: make(map[reflect.Type]map[string]interface{}), + fqnToType: make(map[FQN]reflect.Type), + ifaceToStruct: make(map[reflect.Type]reflect.Type), + fqnToEnumMember: make(map[string]interface{}), + typeToEnumFQN: make(map[reflect.Type]FQN), } } @@ -66,21 +72,24 @@ func (t *typeRegistry) registerEnum(fqn FQN, enm reflect.Type, members map[strin if enm.Kind() != reflect.String { return fmt.Errorf("the provided enum is not a string derivative: %v", enm) } - - for k, v := range members { - vt := reflect.ValueOf(v).Type() + if existing, exists := t.fqnToType[fqn]; exists && existing != enm { + return fmt.Errorf("another type was already registered with %s: %v", fqn, existing) + } + for memberName, memberVal := range members { + vt := reflect.ValueOf(memberVal).Type() if vt != enm { - return fmt.Errorf("the enum entry for key %s has incorrect type %v", k, vt) + return fmt.Errorf("the enum entry for key %s has incorrect type %v", memberName, vt) } + // Not setting in t.fqnToEnumMember here so we don't cause any side-effects + // if the pre-condition fails at any point. This is done in a second loop. } - if existing, exists := t.fqnToType[fqn]; exists && existing != enm { - return fmt.Errorf("another type was already registered with %s: %v", fqn, existing) + t.typeToEnumFQN[enm] = fqn + for memberName, memberVal := range members { + memberFQN := fmt.Sprintf("%s/%s", fqn, memberName) + t.fqnToEnumMember[memberFQN] = memberVal } - t.fqnToType[fqn] = enm - t.enumMembers[enm] = members - return nil } @@ -143,8 +152,7 @@ func (t *typeRegistry) concreteTypeFor(typ reflect.Type) (structType reflect.Typ err = nil } else if typ.Kind() == reflect.Interface { var ok bool - structType, ok = t.ifaceToStruct[typ] - if !ok { + if structType, ok = t.ifaceToStruct[typ]; !ok { err = fmt.Errorf("no concrete type was registered for interface: %v", typ) } } else { @@ -152,3 +160,37 @@ func (t *typeRegistry) concreteTypeFor(typ reflect.Type) (structType reflect.Typ } return } + +// enumMemberForEnumRef returns the go enum member corresponding to a jsii fully +// qualified enum member name (e.g: "jsii-calc.StringEnum/A"). If no enum member +// was registered (via registerEnum) for the provided enumref, an error is +// returned. +func (t *typeRegistry) enumMemberForEnumRef(ref enumRef) (interface{}, error) { + if member, ok := t.fqnToEnumMember[ref.MemberFQN]; ok { + return member, nil + } + return nil, fmt.Errorf("no enum member registered for %s", ref.MemberFQN) +} + +// tryRenderEnumRef returns an enumref if the provided value corresponds to a +// registered enum type. The returned enumref is nil if the provided enum value +// is a zero-value (i.e: ""). +func (t *typeRegistry) tryRenderEnumRef(value reflect.Value) (ref *enumRef, isEnumRef bool) { + if value.Kind() != reflect.String { + isEnumRef = false + return + } + + if enumFQN, ok := t.typeToEnumFQN[value.Type()]; ok { + isEnumRef = true + if memberName := value.String(); memberName != "" { + ref = &enumRef{MemberFQN: fmt.Sprintf("%s/%s", enumFQN, memberName)} + } else { + ref = nil + } + } else { + isEnumRef = false + } + + return +}