From 1abd45d41a57fd505917d4d21dff850b1518884f Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Mon, 15 Jan 2024 11:19:59 -0300 Subject: [PATCH 1/4] handle duplicate types --- src/parse.ros1.test.ts | 67 ++++++++++++++++++++++++++++++++++++++++++ src/parse.ros2.test.ts | 67 ++++++++++++++++++++++++++++++++++++++++++ src/parse.ts | 12 ++++++-- 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/src/parse.ros1.test.ts b/src/parse.ros1.test.ts index 5229b0b..21d64df 100644 --- 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 index 23cef07..99b43e7 100644 --- 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 index 408c421..64b92e3 100644 --- a/src/parse.ts +++ b/src/parse.ts @@ -76,12 +76,20 @@ export function parse(messageDefinition: string, options: ParseOptions = {}): Me : buildType(definitionLines, ROS1_GRAMMAR), ); + // Filter out duplicate types. + // This will avoid that searching a type by name will return more than one result. + const seenTypes = new Set(); + const uniqueTypes = types.filter((definition) => { + const typeName = definition.name ?? ""; + return seenTypes.has(typeName) ? false : seenTypes.add(typeName); + }); + // Fix up complex type names if (options.skipTypeFixup !== true) { - fixupTypes(types); + fixupTypes(uniqueTypes); } - return types; + return uniqueTypes; } /** From b1ed03465854bf9ab921bc7113875225477dff85 Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Mon, 22 Jan 2024 10:47:53 -0300 Subject: [PATCH 2/4] bump @foxglove/message-definition to 0.3.1 --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 62bf543..5ec0f0c 100644 --- 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/yarn.lock b/yarn.lock index 01094df..6d5a491 100644 --- 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" From 28502b82b2aef364b05050a706fea103036fdaa7 Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Mon, 22 Jan 2024 11:01:27 -0300 Subject: [PATCH 3/4] remove duplicates with deep comparison --- src/parse.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/parse.ts b/src/parse.ts index 64b92e3..01fa9f9 100644 --- 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"; @@ -78,10 +82,11 @@ export function parse(messageDefinition: string, options: ParseOptions = {}): Me // Filter out duplicate types. // This will avoid that searching a type by name will return more than one result. - const seenTypes = new Set(); + const seenTypes: MessageDefinition[] = []; const uniqueTypes = types.filter((definition) => { - const typeName = definition.name ?? ""; - return seenTypes.has(typeName) ? false : seenTypes.add(typeName); + return seenTypes.find((otherDefinition) => isMsgDefEqual(definition, otherDefinition)) + ? false + : seenTypes.push(definition); // Always evaluates to true; }); // Fix up complex type names From 1f7569124640795e6cd41ed27f4d8030fb4bf365 Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Mon, 22 Jan 2024 17:17:05 -0300 Subject: [PATCH 4/4] add note about mcap convert bug --- src/parse.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/parse.ts b/src/parse.ts index 01fa9f9..cc5e284 100644 --- a/src/parse.ts +++ b/src/parse.ts @@ -80,8 +80,9 @@ export function parse(messageDefinition: string, options: ParseOptions = {}): Me : buildType(definitionLines, ROS1_GRAMMAR), ); - // Filter out duplicate types. - // This will avoid that searching a type by name will return more than one result. + // 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))