Add support for "ambiguous" message names (#106)

The algorithm for resolving ROS type names inside message definitions that don't have a package specified is, - If it's a primitive, use the primitive value. - If it's the (reserved) type name `Header`, it's `std_msgs/Header`. - Otherwise, use the package of the containing message. (Note _containing_ message, not top-level message.) This unfortunately necessitates a breaking change in API. Message definitions can't be unambiguously parsed without knowing the package of the top-level message. (Definition MD5s are similarly ambiguous.) This felt like a good time to merge the named/unnamed definition types. Test plan: - Unit tests have been updated to use the new API. - Tested against a development version of webviz. Works on a bag I checked, and fixes the "ambiguous datatype" issue reported by a user. Co-authored-by: Matthew STEEL <matt.STEEL@getcruise.com>
cruise-automation · Nov 10, 2021 · 66e06ae · 66e06ae
1 parent 788856d
commit 66e06ae
Show file tree

Hide file tree

Showing 12 changed files with 245 additions and 227 deletions.
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "rosbag",
-  "version": "2.6.4",
+  "version": "3.0.0",
   "license": "Apache-2.0",
   "repository": "cruise-automation/rosbag.js",
   "dependencies": {

diff --git a/src/BagReader.js b/src/BagReader.js
@@ -293,13 +293,13 @@ export default class BagReader {
   // read an individual record from a buffer
   readRecordFromBuffer<T: Record>(buffer: Buffer, fileOffset: number, cls: Class<T> & { opcode: number }): T {
     const headerLength = buffer.readInt32LE(0);
-    const record = parseHeader(buffer.slice(4, 4 + headerLength), cls);
+    const headerFields = parseHeader(buffer.slice(4, 4 + headerLength), cls);
 
     const dataOffset = 4 + headerLength + 4;
     const dataLength = buffer.readInt32LE(4 + headerLength);
     const data = buffer.slice(dataOffset, dataOffset + dataLength);
 
-    record.parseData(data);
+    const record = new cls(headerFields, data);
 
     record.offset = fileOffset;
     record.dataOffset = record.offset + 4 + headerLength + 4;

diff --git a/src/MessageReader.js b/src/MessageReader.js
@@ -8,7 +8,7 @@
 
 import int53 from "int53";
 import { extractTime } from "./fields";
-import type { RosMsgDefinition, NamedRosMsgDefinition } from "./types";
+import type { RosMsgDefinition } from "./types";
 import { parseMessageDefinition } from "./parseMessageDefinition";
 
 type TypedArrayConstructor = (
@@ -179,42 +179,26 @@ class StandardTypeReader {
   }
 }
 
-const findTypeByName = (types: RosMsgDefinition[], name = ""): NamedRosMsgDefinition => {
-  let foundName = ""; // track name separately in a non-null variable to appease Flow
-  const matches = types.filter((type) => {
-    const typeName = type.name || "";
-    // if the search is empty, return unnamed types
-    if (!name) {
-      return !typeName;
-    }
-    // return if the search is in the type name
-    // or matches exactly if a fully-qualified name match is passed to us
-    const nameEnd = name.indexOf("/") > -1 ? name : `/${name}`;
-    if (typeName.endsWith(nameEnd)) {
-      foundName = typeName;
-      return true;
-    }
-    return false;
-  });
+const findTypeByName = (types: RosMsgDefinition[], name: string): RosMsgDefinition => {
+  const matches = types.filter((type) => type.name === name);
   if (matches.length !== 1) {
     throw new Error(`Expected 1 top level type definition for '${name}' but found ${matches.length}.`);
   }
-  return { ...matches[0], name: foundName };
+  return matches[0];
 };
 
 const friendlyName = (name: string) => name.replace(/\//g, "_");
 
-const createParser = (types: RosMsgDefinition[], freeze: boolean) => {
-  const unnamedTypes = types.filter((type) => !type.name);
-  if (unnamedTypes.length !== 1) {
-    throw new Error("multiple unnamed types");
+const createParser = (types: RosMsgDefinition[], typeName: string, freeze: boolean) => {
+  const topLevelTypes = types.filter((type) => type.name === typeName);
+  if (topLevelTypes.length !== 1) {
+    throw new Error("multiple top-level types");
   }
+  const [topLevelType] = topLevelTypes;
 
-  const [unnamedType] = unnamedTypes;
-
-  const namedTypes: NamedRosMsgDefinition[] = (types.filter((type) => !!type.name): any[]);
+  const nestedTypes: RosMsgDefinition[] = types.filter((type) => type.name !== typeName);
 
-  const constructorBody = (type: RosMsgDefinition | NamedRosMsgDefinition) => {
+  const constructorBody = (type: RosMsgDefinition) => {
     const readerLines: string[] = [];
     type.definitions.forEach((def) => {
       if (def.isConstant) {
@@ -264,10 +248,10 @@ const createParser = (types: RosMsgDefinition[], freeze: boolean) => {
 
   let js = `
   var Record = function (reader) {
-    ${constructorBody(unnamedType)}
+    ${constructorBody(topLevelType)}
   };\n`;
 
-  namedTypes.forEach((t) => {
+  nestedTypes.forEach((t) => {
     js += `
   Record.${friendlyName(t.name)} = function(reader) {
     ${constructorBody(t)}
@@ -299,16 +283,16 @@ export class MessageReader {
   // takes an object message definition and returns
   // a message reader which can be used to read messages based
   // on the message definition
-  constructor(definitions: RosMsgDefinition[], options: { freeze?: ?boolean } = {}) {
+  constructor(definitions: RosMsgDefinition[], typeName: string, options: { freeze?: ?boolean } = {}) {
     let parsedDefinitions = definitions;
     if (typeof parsedDefinitions === "string") {
       // eslint-disable-next-line no-console
       console.warn(
         "Passing string message defintions to MessageReader is deprecated. Instead call `parseMessageDefinition` on it and pass in the resulting parsed message definition object."
       );
-      parsedDefinitions = parseMessageDefinition(parsedDefinitions);
+      parsedDefinitions = parseMessageDefinition(parsedDefinitions, typeName);
     }
-    this.reader = createParser(parsedDefinitions, !!options.freeze);
+    this.reader = createParser(parsedDefinitions, typeName, !!options.freeze);
   }
 
   readMessage(buffer: Buffer) {

diff --git a/src/MessageReader.test.js b/src/MessageReader.test.js
@@ -19,13 +19,19 @@ const getStringBuffer = (str: string) => {
   return Buffer.concat([len, data]);
 };
 
+const getMessageReader = (messageDefinitions, options = {}) => {
+  const typeName = options.typeName || "custom_type/CustomMessage";
+  const parsedMessageDefinitions = parseMessageDefinition(messageDefinitions, typeName);
+  return new MessageReader(parsedMessageDefinitions, typeName, options.readerOptions);
+};
+
 describe("MessageReader", () => {
   describe("simple type", () => {
     const testNum = (type: string, size: number, expected: any, cb: (buffer: Buffer) => any) => {
       const buffer = new Buffer(size);
       cb(buffer);
       it(`parses buffer ${JSON.stringify(buffer)} containing ${type}`, () => {
-        const reader = new MessageReader(parseMessageDefinition(`${type} foo`));
+        const reader = getMessageReader(`${type} foo`);
         expect(reader.readMessage(buffer)).toEqual({
           foo: expected,
         });
@@ -42,7 +48,7 @@ describe("MessageReader", () => {
     testNum("float64", 8, 0xdeadbeefcafebabe, (buffer) => buffer.writeDoubleLE(0xdeadbeefcafebabe, 0));
 
     it("parses string", () => {
-      const reader = new MessageReader(parseMessageDefinition("string name"));
+      const reader = getMessageReader("string name");
       const buff = getStringBuffer("test");
       expect(reader.readMessage(buff)).toEqual({
         name: "test",
@@ -61,7 +67,7 @@ describe("MessageReader", () => {
       // $FlowFixMe flow doesn't like util.TextDecoder
       global.TextDecoder = util.TextDecoder;
 
-      const reader = new MessageReader(parseMessageDefinition("string name"));
+      const reader = getMessageReader("string name");
       const string = range(0, 5000)
         .map(() => String.fromCharCode(Math.floor(Math.random() * 255)))
         .join("");
@@ -73,36 +79,30 @@ describe("MessageReader", () => {
     });
 
     it("parses JSON", () => {
-      const reader = new MessageReader(parseMessageDefinition("#pragma rosbag_parse_json\nstring dummy"));
+      const reader = getMessageReader("#pragma rosbag_parse_json\nstring dummy");
       const buff = getStringBuffer('{"foo":123,"bar":{"nestedFoo":456}}');
       expect(reader.readMessage(buff)).toEqual({
         dummy: { foo: 123, bar: { nestedFoo: 456 } },
       });
 
-      const readerWithSpaces = new MessageReader(
-        parseMessageDefinition(" #pragma rosbag_parse_json  \n  string dummy")
-      );
+      const readerWithSpaces = getMessageReader(" #pragma rosbag_parse_json  \n  string dummy");
       expect(readerWithSpaces.readMessage(buff)).toEqual({
         dummy: { foo: 123, bar: { nestedFoo: 456 } },
       });
 
-      const readerWithNewlines = new MessageReader(
-        parseMessageDefinition("#pragma rosbag_parse_json\n\n\nstring dummy")
-      );
+      const readerWithNewlines = getMessageReader("#pragma rosbag_parse_json\n\n\nstring dummy");
       expect(readerWithNewlines.readMessage(buff)).toEqual({
         dummy: { foo: 123, bar: { nestedFoo: 456 } },
       });
 
-      const readerWithNestedComplexType = new MessageReader(
-        parseMessageDefinition(`#pragma rosbag_parse_json
+      const readerWithNestedComplexType = getMessageReader(`#pragma rosbag_parse_json
       string dummy
       Account account
       ============
       MSG: custom_type/Account
       string name
       uint16 id
-      `)
-      );
+      `);
       expect(
         readerWithNestedComplexType.readMessage(
           Buffer.concat([buff, getStringBuffer('{"first":"First","last":"Last"}}'), new Buffer([100, 0x00])])
@@ -112,17 +112,15 @@ describe("MessageReader", () => {
         account: { name: '{"first":"First","last":"Last"}}', id: 100 },
       });
 
-      const readerWithTrailingPragmaComment = new MessageReader(
-        parseMessageDefinition(`#pragma rosbag_parse_json
+      const readerWithTrailingPragmaComment = getMessageReader(`#pragma rosbag_parse_json
       string dummy
       Account account
       #pragma rosbag_parse_json
       ============
       MSG: custom_type/Account
       string name
       uint16 id
-      `)
-      );
+      `);
       expect(
         readerWithTrailingPragmaComment.readMessage(
           Buffer.concat([buff, getStringBuffer('{"first":"First","last":"Last"}}'), new Buffer([100, 0x00])])
@@ -134,7 +132,7 @@ describe("MessageReader", () => {
     });
 
     it("parses time", () => {
-      const reader = new MessageReader(parseMessageDefinition("time right_now"));
+      const reader = getMessageReader("time right_now");
       const buff = new Buffer(8);
       const now = new Date();
       now.setSeconds(31);
@@ -161,7 +159,7 @@ describe("MessageReader", () => {
     ### foo bar baz?
     string lastName
     `;
-    const reader = new MessageReader(parseMessageDefinition(messageDefinition));
+    const reader = getMessageReader(messageDefinition);
     const buffer = Buffer.concat([getStringBuffer("foo"), getStringBuffer("bar")]);
     expect(reader.readMessage(buffer)).toEqual({
       firstName: "foo",
@@ -171,14 +169,14 @@ describe("MessageReader", () => {
 
   it("still works given string message definitions", () => {
     const messageDefinition = "string value";
-    const reader = new MessageReader(parseMessageDefinition(messageDefinition));
+    const reader = getMessageReader(messageDefinition);
     const buffer = getStringBuffer("foo");
     expect(reader.readMessage(buffer)).toEqual({ value: "foo" });
   });
 
   describe("array", () => {
     it("parses variable length string array", () => {
-      const reader = new MessageReader(parseMessageDefinition("string[] names"));
+      const reader = getMessageReader("string[] names");
       const buffer = Buffer.concat([
         // variable length array has int32 as first entry
         new Buffer([0x03, 0x00, 0x00, 0x00]),
@@ -192,9 +190,9 @@ describe("MessageReader", () => {
     });
 
     it("parses fixed length arrays", () => {
-      const parser1 = new MessageReader(parseMessageDefinition("string[1] names"));
-      const parser2 = new MessageReader(parseMessageDefinition("string[2] names"));
-      const parser3 = new MessageReader(parseMessageDefinition("string[3] names"));
+      const parser1 = getMessageReader("string[1] names");
+      const parser2 = getMessageReader("string[2] names");
+      const parser3 = getMessageReader("string[3] names");
       const buffer = Buffer.concat([getStringBuffer("foo"), getStringBuffer("bar"), getStringBuffer("baz")]);
       expect(parser1.readMessage(buffer)).toEqual({
         names: ["foo"],
@@ -208,7 +206,7 @@ describe("MessageReader", () => {
     });
 
     it("uses an empty array for a 0 length array", () => {
-      const reader = new MessageReader(parseMessageDefinition("string[] names"));
+      const reader = getMessageReader("string[] names");
       const buffer = Buffer.concat([
         // variable length array has int32 as first entry
         new Buffer([0x00, 0x00, 0x00, 0x00]),
@@ -220,7 +218,7 @@ describe("MessageReader", () => {
 
     describe("typed arrays", () => {
       it("uint8[] uses the same backing buffer", () => {
-        const reader = new MessageReader(parseMessageDefinition("uint8[] values\nuint8 after"));
+        const reader = getMessageReader("uint8[] values\nuint8 after");
         const buffer = Buffer.from([0x03, 0x00, 0x00, 0x00, 0x01, 0x02, 0x03, 0x04]);
         const result = reader.readMessage(buffer);
         const { values, after } = result;
@@ -237,7 +235,7 @@ describe("MessageReader", () => {
       });
 
       it("parses uint8[] with a fixed length", () => {
-        const reader = new MessageReader(parseMessageDefinition("uint8[3] values\nuint8 after"));
+        const reader = getMessageReader("uint8[3] values\nuint8 after");
         const buffer = Buffer.from([0x01, 0x02, 0x03, 0x04]);
         const result = reader.readMessage(buffer);
         const { values, after } = result;
@@ -254,7 +252,7 @@ describe("MessageReader", () => {
       });
 
       it("int8[] uses the same backing buffer", () => {
-        const reader = new MessageReader(parseMessageDefinition("int8[] values\nint8 after"));
+        const reader = getMessageReader("int8[] values\nint8 after");
         const buffer = new Buffer([0x03, 0x00, 0x00, 0x00, 0x01, 0x02, 0x03, 0x04]);
         const result = reader.readMessage(buffer);
         const { values, after } = result;
@@ -271,7 +269,7 @@ describe("MessageReader", () => {
       });
 
       it("parses int8[] with a fixed length", () => {
-        const reader = new MessageReader(parseMessageDefinition("int8[3] values\nint8 after"));
+        const reader = getMessageReader("int8[3] values\nint8 after");
         const buffer = new Buffer([0x01, 0x02, 0x03, 0x04]);
         const result = reader.readMessage(buffer);
         const { values, after } = result;
@@ -288,7 +286,7 @@ describe("MessageReader", () => {
       });
 
       it("parses combinations of typed arrays", () => {
-        const reader = new MessageReader(parseMessageDefinition("int8[] first\nuint8[2] second"));
+        const reader = getMessageReader("int8[] first\nuint8[2] second");
         const buffer = new Buffer([0x02, 0x00, 0x00, 0x00, 0x01, 0x02, 0x03, 0x04]);
         const result = reader.readMessage(buffer);
         const { first, second } = result;
@@ -310,7 +308,8 @@ describe("MessageReader", () => {
 
   describe("complex types", () => {
     it("parses single complex type", () => {
-      const reader = new MessageReader(parseMessageDefinition("string firstName \n string lastName\nuint16 age"));
+      const messageDefinition = "string firstName \n string lastName\nuint16 age";
+      const reader = getMessageReader(messageDefinition);
       const buffer = Buffer.concat([getStringBuffer("foo"), getStringBuffer("bar"), new Buffer([0x05, 0x00])]);
       expect(reader.readMessage(buffer)).toEqual({
         firstName: "foo",
@@ -328,7 +327,7 @@ describe("MessageReader", () => {
       string name
       uint16 id
       `;
-      const reader = new MessageReader(parseMessageDefinition(messageDefinition));
+      const reader = getMessageReader(messageDefinition);
       const buffer = Buffer.concat([getStringBuffer("foo"), getStringBuffer("bar"), new Buffer([100, 0x00])]);
       expect(reader.readMessage(buffer)).toEqual({
         username: "foo",
@@ -348,7 +347,7 @@ describe("MessageReader", () => {
       string name
       uint16 id
       `;
-      const reader = new MessageReader(parseMessageDefinition(messageDefinition));
+      const reader = getMessageReader(messageDefinition);
       const buffer = Buffer.concat([
         getStringBuffer("foo"),
         // uint32LE length of array (2)
@@ -388,8 +387,7 @@ describe("MessageReader", () => {
       string url
       uint8 id
       `;
-
-      const reader = new MessageReader(parseMessageDefinition(messageDefinition));
+      const reader = getMessageReader(messageDefinition);
       const buffer = Buffer.concat([
         getStringBuffer("foo"),
         // uint32LE length of Account array (2)
@@ -479,7 +477,10 @@ describe("MessageReader", () => {
       string name # a description of the test/component reporting`;
 
     it("parses bytes and constants", () => {
-      const reader = new MessageReader(parseMessageDefinition(withBytesAndBools));
+      const reader = getMessageReader(withBytesAndBools, {
+        typeName: "diagnostic_msgs/DiagnosticArray",
+        readerOptions: {},
+      });
       const buffer = Buffer.concat([Buffer.from([0x01]), Buffer.from([0x00]), getStringBuffer("foo")]);
 
       const message = reader.readMessage(buffer);
@@ -493,8 +494,8 @@ describe("MessageReader", () => {
     });
 
     it("freezes the resulting message if requested", () => {
-      const reader = new MessageReader(parseMessageDefinition("string firstName \n string lastName\nuint16 age"), {
-        freeze: true,
+      const reader = getMessageReader("string firstName \n string lastName\nuint16 age", {
+        readerOptions: { freeze: true },
       });
       const buffer = Buffer.concat([getStringBuffer("foo"), getStringBuffer("bar"), new Buffer([0x05, 0x00])]);
       const output = reader.readMessage(buffer);