Merge pull request #23 from join-com/join-11557-stable-sort

[JOIN-11557] fix: make topological sort for classes stable
join-com · Apr 22, 2021 · 9567048 · 9567048
2 parents 5b8fd68 + c50bb6f
commit 9567048
Show file tree

Hide file tree

Showing 2 changed files with 123 additions and 106 deletions.
diff --git a/internal/generator/runner_generate_class.go b/internal/generator/runner_generate_class.go
@@ -1,6 +1,7 @@
 package generator
 
 import (
+	"sort"
 	"strings"
 
 	"github.com/iancoleman/strcase"
@@ -17,6 +18,10 @@ func (r *Runner) generateTypescriptMessageClasses(generatedFileStream *protogen.
 }
 
 func (r *Runner) getTopologicallySortedMessages(messageSpecs []*descriptorpb.DescriptorProto) []*descriptorpb.DescriptorProto {
+	if len(messageSpecs) <= 1 {
+		return messageSpecs // No need to go to sort in this case
+	}
+
 	//                   referrer class               ->   referred classes
 	refsMap := make(map[*descriptorpb.DescriptorProto]map[*descriptorpb.DescriptorProto]bool)
 
@@ -54,12 +59,24 @@ func (r *Runner) getTopologicallySortedMessages(messageSpecs []*descriptorpb.Des
 		refsMap[messageSpec] = messageMap
 	}
 
+	// We need to presort by name to obtain a stable order, as topological order is not absolute
+	orderedMessageSpecsByName := make([]*descriptorpb.DescriptorProto, len(messageSpecs))
+	copy(orderedMessageSpecsByName, messageSpecs)
+	sort.Slice(orderedMessageSpecsByName, func(i, j int) bool {
+		return orderedMessageSpecsByName[i].GetName() < orderedMessageSpecsByName[j].GetName()
+	})
+
 	processedMessages := make(map[*descriptorpb.DescriptorProto]bool)
 
 	// Second step, iteratively remove items from the graph and add them to our topologically sorted list
 	orderedMessageSpecs := make([]*descriptorpb.DescriptorProto, 0, len(messageSpecs))
 	for len(orderedMessageSpecs) < len(messageSpecs) {
-		for referrer, referredCollection := range refsMap {
+		for _, referrer := range orderedMessageSpecsByName {
+			referredCollection, ok := refsMap[referrer]
+			if !ok || referredCollection == nil {
+				utils.LogError("Unable to retrieve referred collection in topological sort for " + referrer.GetName())
+			}
+
 			if len(referredCollection) > 0 || processedMessages[referrer] {
 				continue
 			}

diff --git a/tests/__tests__/generated/Test.ts b/tests/__tests__/generated/Test.ts
@@ -92,34 +92,54 @@ export namespace Foo {
     optionalField?: number
   }
 
-  @protobufjs.Type.d('Request')
-  export class Request
-    extends protobufjs.Message<Request>
-    implements ConvertibleTo<IRequest>, IRequest {
-    @protobufjs.Field.d(1, 'int32')
-    public id?: number
+  @protobufjs.Type.d('CustomOptionsTest')
+  export class CustomOptionsTest
+    extends protobufjs.Message<CustomOptionsTest>
+    implements ConvertibleTo<ICustomOptionsTest> {
+    @protobufjs.Field.d(1, Common_Extra.ExtraPkgMessage)
+    public requiredField!: Common_Extra.ExtraPkgMessage
 
-    public asInterface(): IRequest {
-      return this
+    @protobufjs.Field.d(2, 'int32')
+    public typicalOptionalField?: number
+
+    @protobufjs.Field.d(3, 'int32')
+    public customOptionalField?: number
+
+    public asInterface(): ICustomOptionsTest {
+      return {
+        ...this,
+        requiredField: this.requiredField.asInterface(),
+      }
     }
 
-    public static fromInterface(this: void, value: IRequest): Request {
-      return Request.fromObject(value)
+    public static fromInterface(
+      this: void,
+      value: ICustomOptionsTest
+    ): CustomOptionsTest {
+      const patchedValue = {
+        ...value,
+        requiredField: Common_Extra.ExtraPkgMessage.fromInterface(
+          value.requiredField
+        ),
+      }
+
+      return CustomOptionsTest.fromObject(patchedValue)
     }
 
     public static decodePatched(
       this: void,
       reader: protobufjs.Reader | Uint8Array
-    ): IRequest {
-      return Request.decode(reader)
+    ): ICustomOptionsTest {
+      return CustomOptionsTest.decode(reader).asInterface()
     }
 
     public static encodePatched(
       this: void,
-      message: IRequest,
+      message: ICustomOptionsTest,
       writer?: protobufjs.Writer
     ): protobufjs.Writer {
-      return Request.encode(message, writer)
+      const transformedMessage = CustomOptionsTest.fromInterface(message)
+      return CustomOptionsTest.encode(transformedMessage, writer)
     }
   }
 
@@ -157,6 +177,77 @@ export namespace Foo {
     }
   }
 
+  @protobufjs.Type.d('Request')
+  export class Request
+    extends protobufjs.Message<Request>
+    implements ConvertibleTo<IRequest>, IRequest {
+    @protobufjs.Field.d(1, 'int32')
+    public id?: number
+
+    public asInterface(): IRequest {
+      return this
+    }
+
+    public static fromInterface(this: void, value: IRequest): Request {
+      return Request.fromObject(value)
+    }
+
+    public static decodePatched(
+      this: void,
+      reader: protobufjs.Reader | Uint8Array
+    ): IRequest {
+      return Request.decode(reader)
+    }
+
+    public static encodePatched(
+      this: void,
+      message: IRequest,
+      writer?: protobufjs.Writer
+    ): protobufjs.Writer {
+      return Request.encode(message, writer)
+    }
+  }
+
+  @protobufjs.Type.d('RequiredPropertiesTest')
+  export class RequiredPropertiesTest
+    extends protobufjs.Message<RequiredPropertiesTest>
+    implements ConvertibleTo<IRequiredPropertiesTest>, IRequiredPropertiesTest {
+    @protobufjs.Field.d(1, 'int32')
+    public requiredField!: number
+
+    @protobufjs.Field.d(2, 'int32')
+    public customRequiredField!: number
+
+    @protobufjs.Field.d(3, 'int32')
+    public optionalField?: number
+
+    public asInterface(): IRequiredPropertiesTest {
+      return this
+    }
+
+    public static fromInterface(
+      this: void,
+      value: IRequiredPropertiesTest
+    ): RequiredPropertiesTest {
+      return RequiredPropertiesTest.fromObject(value)
+    }
+
+    public static decodePatched(
+      this: void,
+      reader: protobufjs.Reader | Uint8Array
+    ): IRequiredPropertiesTest {
+      return RequiredPropertiesTest.decode(reader)
+    }
+
+    public static encodePatched(
+      this: void,
+      message: IRequiredPropertiesTest,
+      writer?: protobufjs.Writer
+    ): protobufjs.Writer {
+      return RequiredPropertiesTest.encode(message, writer)
+    }
+  }
+
   @protobufjs.Type.d('Test')
   export class Test
     extends protobufjs.Message<Test>
@@ -339,97 +430,6 @@ export namespace Foo {
     }
   }
 
-  @protobufjs.Type.d('CustomOptionsTest')
-  export class CustomOptionsTest
-    extends protobufjs.Message<CustomOptionsTest>
-    implements ConvertibleTo<ICustomOptionsTest> {
-    @protobufjs.Field.d(1, Common_Extra.ExtraPkgMessage)
-    public requiredField!: Common_Extra.ExtraPkgMessage
-
-    @protobufjs.Field.d(2, 'int32')
-    public typicalOptionalField?: number
-
-    @protobufjs.Field.d(3, 'int32')
-    public customOptionalField?: number
-
-    public asInterface(): ICustomOptionsTest {
-      return {
-        ...this,
-        requiredField: this.requiredField.asInterface(),
-      }
-    }
-
-    public static fromInterface(
-      this: void,
-      value: ICustomOptionsTest
-    ): CustomOptionsTest {
-      const patchedValue = {
-        ...value,
-        requiredField: Common_Extra.ExtraPkgMessage.fromInterface(
-          value.requiredField
-        ),
-      }
-
-      return CustomOptionsTest.fromObject(patchedValue)
-    }
-
-    public static decodePatched(
-      this: void,
-      reader: protobufjs.Reader | Uint8Array
-    ): ICustomOptionsTest {
-      return CustomOptionsTest.decode(reader).asInterface()
-    }
-
-    public static encodePatched(
-      this: void,
-      message: ICustomOptionsTest,
-      writer?: protobufjs.Writer
-    ): protobufjs.Writer {
-      const transformedMessage = CustomOptionsTest.fromInterface(message)
-      return CustomOptionsTest.encode(transformedMessage, writer)
-    }
-  }
-
-  @protobufjs.Type.d('RequiredPropertiesTest')
-  export class RequiredPropertiesTest
-    extends protobufjs.Message<RequiredPropertiesTest>
-    implements ConvertibleTo<IRequiredPropertiesTest>, IRequiredPropertiesTest {
-    @protobufjs.Field.d(1, 'int32')
-    public requiredField!: number
-
-    @protobufjs.Field.d(2, 'int32')
-    public customRequiredField!: number
-
-    @protobufjs.Field.d(3, 'int32')
-    public optionalField?: number
-
-    public asInterface(): IRequiredPropertiesTest {
-      return this
-    }
-
-    public static fromInterface(
-      this: void,
-      value: IRequiredPropertiesTest
-    ): RequiredPropertiesTest {
-      return RequiredPropertiesTest.fromObject(value)
-    }
-
-    public static decodePatched(
-      this: void,
-      reader: protobufjs.Reader | Uint8Array
-    ): IRequiredPropertiesTest {
-      return RequiredPropertiesTest.decode(reader)
-    }
-
-    public static encodePatched(
-      this: void,
-      message: IRequiredPropertiesTest,
-      writer?: protobufjs.Writer
-    ): protobufjs.Writer {
-      return RequiredPropertiesTest.encode(message, writer)
-    }
-  }
-
   export interface IUsersServiceImplementation {
     Find: grpc.handleUnaryCall<IRequest, Common_Common.IOtherPkgMessage>
     FindClientStream: grpc.handleClientStreamingCall<