From 5da79520537176b6418f55d767cddc681b020f8d Mon Sep 17 00:00:00 2001 From: Andres Correa Casablanca Date: Wed, 14 Jul 2021 23:30:58 +0200 Subject: [PATCH 1/2] fix: apply .asInterface() always --- internal/generator/runner_generate_class.go | 31 ++---- tests/__tests__/generated/Flavors.ts | 8 +- tests/__tests__/generated/Regressions.ts | 102 ++++++++++++++++++ tests/__tests__/generated/Test.ts | 18 +++- tests/__tests__/generated/common/Common.ts | 4 +- .../generated/google/protobuf/Empty.ts | 4 +- .../generated/google/protobuf/Timestamp.ts | 4 +- tests/__tests__/proto/regressions.proto | 17 +++ tests/__tests__/regressions.test.ts | 19 ++++ 9 files changed, 179 insertions(+), 28 deletions(-) create mode 100644 tests/__tests__/generated/Regressions.ts create mode 100644 tests/__tests__/proto/regressions.proto create mode 100644 tests/__tests__/regressions.test.ts diff --git a/internal/generator/runner_generate_class.go b/internal/generator/runner_generate_class.go index 7d50616..51fa027 100644 --- a/internal/generator/runner_generate_class.go +++ b/internal/generator/runner_generate_class.go @@ -181,14 +181,14 @@ func (r *Runner) generateTypescriptClassField( } func (r *Runner) generateTypescriptClassPatchedMethods(generatedFileStream *protogen.GeneratedFile, messageSpec *descriptorpb.DescriptorProto, requiredFields bool, hasEnums bool) { - r.generateAsInterfaceMethod(generatedFileStream, messageSpec, requiredFields, hasEnums) + r.generateAsInterfaceMethod(generatedFileStream, messageSpec, requiredFields) r.generateFromInterfaceMethod(generatedFileStream, messageSpec, requiredFields, hasEnums) r.generateDecodePatchedMethod(generatedFileStream, messageSpec, requiredFields) r.generateEncodePatchedMethod(generatedFileStream, messageSpec, requiredFields, hasEnums) } -func (r *Runner) generateAsInterfaceMethod(generatedFileStream *protogen.GeneratedFile, messageSpec *descriptorpb.DescriptorProto, requiredFields bool, hasEnums bool) { +func (r *Runner) generateAsInterfaceMethod(generatedFileStream *protogen.GeneratedFile, messageSpec *descriptorpb.DescriptorProto, requiredFields bool) { className := strcase.ToCamel(messageSpec.GetName()) r.P(generatedFileStream, "public asInterface(): I"+className+" {") r.indentLevel += 2 @@ -198,29 +198,23 @@ func (r *Runner) generateAsInterfaceMethod(generatedFileStream *protogen.Generat hasRepeated = hasRepeated || (fieldSpec.GetLabel() == descriptorpb.FieldDescriptorProto_LABEL_REPEATED) } - if hasEnums { - r.P(generatedFileStream, "const message = {") - r.indentLevel += 2 - - r.P(generatedFileStream, "...this,") - for _, fieldSpec := range messageSpec.GetField() { - r.generatePatchedInterfaceField(generatedFileStream, fieldSpec, messageSpec, requiredFields) - } - - r.indentLevel -= 2 - r.P(generatedFileStream, "}") - } else { - r.P(generatedFileStream, "const message = { ...this }") + r.P(generatedFileStream, "const message = {") + r.indentLevel += 2 + r.P(generatedFileStream, "...this,") + for _, fieldSpec := range messageSpec.GetField() { + r.generatePatchedInterfaceField(generatedFileStream, fieldSpec, messageSpec, requiredFields) } + r.indentLevel -= 2 + r.P(generatedFileStream, "}") var fieldAssignment string var loopInnerIfClause string if hasRepeated { - fieldAssignment = " const field = message[fieldName as keyof I"+className+"]" + fieldAssignment = " const field = message[fieldName as keyof I" + className + "]" loopInnerIfClause = "field == null || Array.isArray(field) && field.length === 0" } else { fieldAssignment = "" - loopInnerIfClause = "message[fieldName as keyof I"+className+"] == null" + loopInnerIfClause = "message[fieldName as keyof I" + className + "] == null" } r.P( @@ -410,9 +404,6 @@ func (r *Runner) generatePatchedInterfaceField( if !ok || nestedMessageSpec == nil { utils.LogError("Unable to retrieve message spec for " + fieldTypeName) } - if fieldTypeName != ".google.protobuf.Timestamp" && !r.messageHasEnumsOrDates(nestedMessageSpec) { - return - } if confirmRequired { if fieldTypeName == ".google.protobuf.Timestamp" { diff --git a/tests/__tests__/generated/Flavors.ts b/tests/__tests__/generated/Flavors.ts index a06ed73..b85b862 100644 --- a/tests/__tests__/generated/Flavors.ts +++ b/tests/__tests__/generated/Flavors.ts @@ -42,7 +42,9 @@ export namespace Flavors { public emails!: Email[] public asInterface(): IUserProfile { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { const field = message[fieldName as keyof IUserProfile] if (field == null || (Array.isArray(field) && field.length === 0)) { @@ -107,7 +109,9 @@ export namespace Flavors { public userId?: UserId public asInterface(): IUserRequest { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof IUserRequest] == null) { // We remove the key to avoid problems with code making too many assumptions diff --git a/tests/__tests__/generated/Regressions.ts b/tests/__tests__/generated/Regressions.ts new file mode 100644 index 0000000..a8ca3e0 --- /dev/null +++ b/tests/__tests__/generated/Regressions.ts @@ -0,0 +1,102 @@ +// GENERATED CODE -- DO NOT EDIT! +/* eslint-disable @typescript-eslint/no-non-null-assertion */ + +import * as protobufjs from 'protobufjs/light' + +// eslint-disable-next-line @typescript-eslint/no-namespace +export namespace Regressions { + interface ConvertibleTo { + asInterface(): T + } + + export interface IReg01Inner { + value?: string + } + + export interface IReg01Outer { + inner?: IReg01Inner + } + + @protobufjs.Type.d('regressions_Reg01Inner') + export class Reg01Inner + extends protobufjs.Message + implements ConvertibleTo, IReg01Inner + { + @protobufjs.Field.d(1, 'string', 'optional') + public value?: string + + public asInterface(): IReg01Inner { + const message = { + ...this, + } + for (const fieldName of Object.keys(message)) { + if (message[fieldName as keyof IReg01Inner] == null) { + // We remove the key to avoid problems with code making too many assumptions + delete message[fieldName as keyof IReg01Inner] + } + } + return message + } + + public static fromInterface(this: void, value: IReg01Inner): Reg01Inner { + return Reg01Inner.fromObject(value) + } + + public static decodePatched( + this: void, + reader: protobufjs.Reader | Uint8Array + ): IReg01Inner { + return Reg01Inner.decode(reader).asInterface() + } + + public static encodePatched( + this: void, + message: IReg01Inner, + writer?: protobufjs.Writer + ): protobufjs.Writer { + return Reg01Inner.encode(message, writer) + } + } + + @protobufjs.Type.d('regressions_Reg01Outer') + export class Reg01Outer + extends protobufjs.Message + implements ConvertibleTo, IReg01Outer + { + @protobufjs.Field.d(1, Reg01Inner, 'optional') + public inner?: Reg01Inner + + public asInterface(): IReg01Outer { + const message = { + ...this, + inner: this.inner?.asInterface(), + } + for (const fieldName of Object.keys(message)) { + if (message[fieldName as keyof IReg01Outer] == null) { + // We remove the key to avoid problems with code making too many assumptions + delete message[fieldName as keyof IReg01Outer] + } + } + return message + } + + public static fromInterface(this: void, value: IReg01Outer): Reg01Outer { + return Reg01Outer.fromObject(value) + } + + public static decodePatched( + this: void, + reader: protobufjs.Reader | Uint8Array + ): IReg01Outer { + return Reg01Outer.decode(reader).asInterface() + } + + public static encodePatched( + this: void, + message: IReg01Outer, + writer?: protobufjs.Writer + ): protobufjs.Writer { + return Reg01Outer.encode(message, writer) + } + } +} diff --git a/tests/__tests__/generated/Test.ts b/tests/__tests__/generated/Test.ts index 744c782..6ae56bb 100644 --- a/tests/__tests__/generated/Test.ts +++ b/tests/__tests__/generated/Test.ts @@ -188,7 +188,9 @@ export namespace Foo { public title?: string public asInterface(): INested { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof INested] == null) { // We remove the key to avoid problems with code making too many assumptions @@ -227,7 +229,9 @@ export namespace Foo { public id?: number public asInterface(): IRequest { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof IRequest] == null) { // We remove the key to avoid problems with code making too many assumptions @@ -272,7 +276,9 @@ export namespace Foo { public optionalField?: number public asInterface(): IRequiredPropertiesTest { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof IRequiredPropertiesTest] == null) { // We remove the key to avoid problems with code making too many assumptions @@ -454,6 +460,8 @@ export namespace Foo { fieldEnumRepeated: this.fieldEnumRepeated?.map( (e) => Role_Enum[e]! as Role ), + message: this.message?.asInterface(), + messageRepeated: this.messageRepeated?.map((o) => o.asInterface()), timestamp: this.timestamp != null ? new Date( @@ -464,6 +472,10 @@ export namespace Foo { timestampRepeated: this.timestampRepeated?.map( (ts) => new Date((ts.seconds ?? 0) * 1000 + (ts.nanos ?? 0) / 1000000) ), + otherPkgMessage: this.otherPkgMessage?.asInterface(), + otherPkgMessageRepeated: this.otherPkgMessageRepeated?.map((o) => + o.asInterface() + ), } for (const fieldName of Object.keys(message)) { const field = message[fieldName as keyof ITest] diff --git a/tests/__tests__/generated/common/Common.ts b/tests/__tests__/generated/common/Common.ts index a3db7f4..0fd512c 100644 --- a/tests/__tests__/generated/common/Common.ts +++ b/tests/__tests__/generated/common/Common.ts @@ -26,7 +26,9 @@ export namespace Common { public latsName?: string public asInterface(): IOtherPkgMessage { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof IOtherPkgMessage] == null) { // We remove the key to avoid problems with code making too many assumptions diff --git a/tests/__tests__/generated/google/protobuf/Empty.ts b/tests/__tests__/generated/google/protobuf/Empty.ts index 11e74ea..31c9b7f 100644 --- a/tests/__tests__/generated/google/protobuf/Empty.ts +++ b/tests/__tests__/generated/google/protobuf/Empty.ts @@ -17,7 +17,9 @@ export namespace GoogleProtobuf { implements ConvertibleTo, IEmpty { public asInterface(): IEmpty { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof IEmpty] == null) { // We remove the key to avoid problems with code making too many assumptions diff --git a/tests/__tests__/generated/google/protobuf/Timestamp.ts b/tests/__tests__/generated/google/protobuf/Timestamp.ts index 9f0e633..49fff51 100644 --- a/tests/__tests__/generated/google/protobuf/Timestamp.ts +++ b/tests/__tests__/generated/google/protobuf/Timestamp.ts @@ -26,7 +26,9 @@ export namespace GoogleProtobuf { public nanos?: number public asInterface(): ITimestamp { - const message = { ...this } + const message = { + ...this, + } for (const fieldName of Object.keys(message)) { if (message[fieldName as keyof ITimestamp] == null) { // We remove the key to avoid problems with code making too many assumptions diff --git a/tests/__tests__/proto/regressions.proto b/tests/__tests__/proto/regressions.proto new file mode 100644 index 0000000..bd3ef3c --- /dev/null +++ b/tests/__tests__/proto/regressions.proto @@ -0,0 +1,17 @@ +syntax = "proto3"; + +package regressions; + +// Makes protoc happy (useful for Golang code generation, not for TS) +option go_package = "github.com/join-com/protoc-gen-ts/regressions"; + + +// Regression 01 + +message Reg01Inner { + string value = 1; +} + +message Reg01Outer { + Reg01Inner inner = 1; +} diff --git a/tests/__tests__/regressions.test.ts b/tests/__tests__/regressions.test.ts new file mode 100644 index 0000000..8d4bcd7 --- /dev/null +++ b/tests/__tests__/regressions.test.ts @@ -0,0 +1,19 @@ +import { Regressions } from './generated/Regressions' + +describe('regressions', () => { + it('decodes missing nested fields as undefined when there are no enums', () => { + // Background: before this test was written, calling .asInterface() for nested objects was + // done only when there were enum fields at some level of the object. + + const originalA: Regressions.IReg01Outer = {} + const bufferA = Regressions.Reg01Outer.encodePatched(originalA).finish() + const reconstructedA = Regressions.Reg01Outer.decodePatched(bufferA) + + const originalB: Regressions.IReg01Outer = { inner: {} } + const bufferB = Regressions.Reg01Outer.encodePatched(originalB).finish() + const reconstructedB = Regressions.Reg01Outer.decodePatched(bufferB) + + expect(reconstructedA.inner?.value).toBeUndefined() + expect(reconstructedB.inner?.value).toBeUndefined() + }) +}) From aae3e81a25113cf52c256e64bb789518b10f5640 Mon Sep 17 00:00:00 2001 From: Andres Correa Casablanca Date: Wed, 14 Jul 2021 23:32:28 +0200 Subject: [PATCH 2/2] test: rename test --- tests/__tests__/regressions.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/__tests__/regressions.test.ts b/tests/__tests__/regressions.test.ts index 8d4bcd7..b764a93 100644 --- a/tests/__tests__/regressions.test.ts +++ b/tests/__tests__/regressions.test.ts @@ -1,7 +1,7 @@ import { Regressions } from './generated/Regressions' describe('regressions', () => { - it('decodes missing nested fields as undefined when there are no enums', () => { + it('01. decodes missing nested fields as undefined when there are no enums', () => { // Background: before this test was written, calling .asInterface() for nested objects was // done only when there were enum fields at some level of the object.