Fix exception when message definition contains duplicate types (#45)

Fix exception when message definition contains duplicate types This PR removes duplicate types (with the same name) to ensure that at max one result is returned when searching a type by its name. This fixes an exception that occurred when the message definition contained duplicate types due to a bug in mcap convert.
foxglove · Jan 22, 2024 · a1b8678 · a1b8678
1 parent 95a9e29
commit a1b8678
Show file tree

Hide file tree

Showing 5 changed files with 156 additions and 8 deletions.
diff --git a/package.json b/package.json
@@ -47,7 +47,7 @@
     "node": ">= 14"
   },
   "dependencies": {
-    "@foxglove/message-definition": "^0.2.0",
+    "@foxglove/message-definition": "^0.3.1",
     "md5-typescript": "^1.0.5"
   },
   "devDependencies": {

diff --git a/src/parse.ros1.test.ts b/src/parse.ros1.test.ts
@@ -623,4 +623,71 @@ describe("fixupTypes", () => {
       },
     ]);
   });
+
+  it("handles duplicate types", () => {
+    const messageDefinition = `
+    foo_msgs/TypeA a
+    foo_msgs/TypeB b
+    ================================================================================
+    MSG: foo_msgs/TypeA
+
+    uint64 u
+    ================================================================================
+    MSG: foo_msgs/TypeB
+
+    foo_msgs/TypeA a
+    int32 i
+    ================================================================================
+    MSG: foo_msgs/TypeA
+
+    uint64 u
+    `;
+    const types = parse(messageDefinition, { ros2: true });
+    expect(types).toEqual([
+      {
+        definitions: [
+          {
+            type: "foo_msgs/TypeA",
+            isArray: false,
+            name: "a",
+            isComplex: true,
+          },
+          {
+            type: "foo_msgs/TypeB",
+            isArray: false,
+            name: "b",
+            isComplex: true,
+          },
+        ],
+      },
+      {
+        name: "foo_msgs/TypeA",
+        definitions: [
+          {
+            type: "uint64",
+            isArray: false,
+            name: "u",
+            isComplex: false,
+          },
+        ],
+      },
+      {
+        name: "foo_msgs/TypeB",
+        definitions: [
+          {
+            type: "foo_msgs/TypeA",
+            isArray: false,
+            name: "a",
+            isComplex: true,
+          },
+          {
+            type: "int32",
+            isArray: false,
+            name: "i",
+            isComplex: false,
+          },
+        ],
+      },
+    ]);
+  });
 });
diff --git a/src/parse.ros2.test.ts b/src/parse.ros2.test.ts
@@ -1109,4 +1109,71 @@ string<=10[<=5] up_to_five_strings_up_to_ten_characters_each
       },
     ]);
   });
+
+  it("handles duplicate types", () => {
+    const messageDefinition = `
+    foo_msgs/msg/TypeA a
+    foo_msgs/msg/TypeB b
+    ================================================================================
+    MSG: foo_msgs/msg/TypeA
+
+    uint64 u
+    ================================================================================
+    MSG: foo_msgs/msg/TypeB
+
+    foo_msgs/msg/TypeA a
+    int32 i
+    ================================================================================
+    MSG: foo_msgs/msg/TypeA
+
+    uint64 u
+    `;
+    const types = parse(messageDefinition, { ros2: true });
+    expect(types).toEqual([
+      {
+        definitions: [
+          {
+            type: "foo_msgs/msg/TypeA",
+            isArray: false,
+            name: "a",
+            isComplex: true,
+          },
+          {
+            type: "foo_msgs/msg/TypeB",
+            isArray: false,
+            name: "b",
+            isComplex: true,
+          },
+        ],
+      },
+      {
+        name: "foo_msgs/msg/TypeA",
+        definitions: [
+          {
+            type: "uint64",
+            isArray: false,
+            name: "u",
+            isComplex: false,
+          },
+        ],
+      },
+      {
+        name: "foo_msgs/msg/TypeB",
+        definitions: [
+          {
+            type: "foo_msgs/msg/TypeA",
+            isArray: false,
+            name: "a",
+            isComplex: true,
+          },
+          {
+            type: "int32",
+            isArray: false,
+            name: "i",
+            isComplex: false,
+          },
+        ],
+      },
+    ]);
+  });
 });
diff --git a/src/parse.ts b/src/parse.ts
@@ -7,7 +7,11 @@
 //   found at http://www.apache.org/licenses/LICENSE-2.0
 //   You may not use this file except in compliance with the License.
 
-import { MessageDefinition, MessageDefinitionField } from "@foxglove/message-definition";
+import {
+  MessageDefinition,
+  MessageDefinitionField,
+  isMsgDefEqual,
+} from "@foxglove/message-definition";
 import { Grammar, Parser } from "nearley";
 
 import { buildRos2Type } from "./buildRos2Type";
@@ -76,12 +80,22 @@ export function parse(messageDefinition: string, options: ParseOptions = {}): Me
       : buildType(definitionLines, ROS1_GRAMMAR),
   );
 
+  // Filter out duplicate types to handle the case where schemas are erroneously duplicated
+  // e.g. caused by a bug in `mcap convert`. Removing duplicates here will avoid that searching
+  // a type by name will return more than one result.
+  const seenTypes: MessageDefinition[] = [];
+  const uniqueTypes = types.filter((definition) => {
+    return seenTypes.find((otherDefinition) => isMsgDefEqual(definition, otherDefinition))
+      ? false
+      : seenTypes.push(definition); // Always evaluates to true;
+  });
+
   // Fix up complex type names
   if (options.skipTypeFixup !== true) {
-    fixupTypes(types);
+    fixupTypes(uniqueTypes);
   }
 
-  return types;
+  return uniqueTypes;
 }
 
 /**

diff --git a/yarn.lock b/yarn.lock
@@ -416,10 +416,10 @@
     tsutils "^3"
     typescript "^4"
 
-"@foxglove/message-definition@^0.2.0":
-  version "0.2.0"
-  resolved "https://registry.yarnpkg.com/@foxglove/message-definition/-/message-definition-0.2.0.tgz#e31922e58e57d224717bcfbd20d3cbaad709dc6b"
-  integrity sha512-IQHIGCvBZR8GIua9nEpS+hsMF3gm1bfbrrnjG0rgtcFBWiNuKbzx4vIP8OIwDC+8wtwcFdfJhf4Vp5TPFiUUcQ==
+"@foxglove/message-definition@^0.3.1":
+  version "0.3.1"
+  resolved "https://registry.yarnpkg.com/@foxglove/message-definition/-/message-definition-0.3.1.tgz#63f48e8f3de47bba8943d8bfa8021af635254f1c"
+  integrity sha512-nkPowiED67LjcKEC77CprkUG3XvSsFHHR9HEwWCuhnIC2wm0W57T1J+WWvteoArZ7SdGGlKzSYSRFyjQkgmITw==
 
 "@humanwhocodes/config-array@^0.10.4":
   version "0.10.4"