fix(mongodb-schema): follow up migration with small bug fixes COMPASS-10911

packages/mongodb-schema/src/schema-analyzer.ts

@@ -256,7 +256,10 @@ function fieldComparator(
     return 1;
   }
   // Otherwise sort case-insensitively.
-  return aName.toLowerCase() < bName.toLowerCase() ? -1 : 1;
+  const aLower = aName.toLowerCase();
+  const bLower = bName.toLowerCase();
+
+  return aLower < bLower ? -1 : aLower > bLower ? 1 : 0;
 }
 
 /**
@@ -603,7 +606,7 @@ export class SchemaAnalyzer {
   allowAbortDuringAnalysis(): Promise<void> {
     // Allow aborting the analysis.
     if (this.fieldAndTypeAnalysisCounter++ % ALLOW_ABORT_INTERVAL_COUNT === 0) {
-      void allowAbort();
+      return allowAbort(this.options.signal);
     }
     return Promise.resolve();
   }

packages/mongodb-schema/src/schema-converters/internal-to-mongodb.ts

@@ -59,7 +59,7 @@ async function parseType(
   };
 
   if (type.bsonType === 'Array') {
-    const items = await parseTypes((type as ArraySchemaType).types);
+    const items = await parseTypes((type as ArraySchemaType).types, signal);
     // Don't include empty bson type arrays (it's invalid).
     if (!items.bsonType || items.bsonType?.length > 0) {
       schema.items = items;

packages/mongodb-schema/src/to-typescript.spec.ts

@@ -22,6 +22,7 @@ import {
 } from 'bson';
 
 import { inspect } from 'util';
+import * as ts from 'typescript';
 
 function convertAndCompare(schema: MongoDBJSONSchema, expected: string) {
   const code = toTypescriptTypeDefinition(schema);
@@ -42,6 +43,21 @@ function convertAndCompare(schema: MongoDBJSONSchema, expected: string) {
 
     throw err;
   }
+
+  // Comparing strings cannot tell us whether the generated code is actually
+  // valid TypeScript, so parse it as well. This is a syntax-only check, which
+  // means unresolved names like `bson.Double` are not reported.
+  const { diagnostics } = ts.transpileModule(`type Generated = ${code};`, {
+    reportDiagnostics: true,
+    compilerOptions: {},
+  });
+  assert.deepEqual(
+    (diagnostics ?? []).map((diagnostic) =>
+      ts.flattenDiagnosticMessageText(diagnostic.messageText, ' '),
+    ),
+    [],
+    'generated type definition is not valid TypeScript',
+  );
 }
 
 describe('toTypescriptTypeDefinition', function () {
@@ -104,7 +120,7 @@ describe('toTypescriptTypeDefinition', function () {
       schema,
       `{
   _id?: bson.ObjectId;
-  array?: bson.Double | number)[];
+  array?: (bson.Double | number)[];
   binaries?: {
     binaryOld?: bson.Binary;
     compressedTimeSeries?: bson.Binary;
@@ -275,6 +291,42 @@ describe('toTypescriptTypeDefinition', function () {
     };
   };
   b?: any[];
+}`,
+    );
+  });
+
+  it('quotes field names that are not valid identifiers', async function () {
+    const docs = [
+      {
+        // Field names the analyzer already supports elsewhere, see
+        // test/nested-document-path.test.ts and test/basic.test.ts.
+        'bar.with.dot': 'foo',
+        'name[]': 'Annabeth Frankie',
+        'has space': 1,
+        '2leading': 'foo',
+        'quote"inside': 'foo',
+        valid_one: 'foo',
+        nested: {
+          'a.b': [1, 2, 3],
+        },
+      },
+    ];
+
+    const analyzedDocuments = await analyzeDocuments(docs);
+    const schema = await analyzedDocuments.getMongoDBJsonSchema();
+
+    convertAndCompare(
+      schema,
+      `{
+  "2leading"?: string;
+  "bar.with.dot"?: string;
+  "has space"?: bson.Double | number;
+  "name[]"?: string;
+  nested?: {
+    "a.b"?: (bson.Double | number)[];
+  };
+  "quote\\"inside"?: string;
+  valid_one?: string;
 }`,
     );
   });

packages/mongodb-schema/src/to-typescript.ts

@@ -105,7 +105,11 @@ function arrayType(types: string[]) {
   if (types.length === 1) {
     return `${types[0]}[]`;
   }
-  return `${types.join(' | ')})[]`;
+  return `(${types.join(' | ')})[]`;
+}
+
+function propertyKey(name: string): string {
+  return /^[A-Za-z_$][A-Za-z0-9_$]*$/.test(name) ? name : JSON.stringify(name);
 }
 
 function toTypescriptType(
@@ -117,20 +121,22 @@ function toTypescriptType(
       switch (getBSONType(schema)) {
         case 'array':
           assertIsDefined(schema.items, 'schema.items must be defined');
-          return `${indentSpaces(indent)}${propertyName}?: ${arrayType([
-            ...uniqueTypes(schema.items),
-          ])}`;
+          return `${indentSpaces(indent)}${propertyKey(
+            propertyName,
+          )}?: ${arrayType([...uniqueTypes(schema.items)])}`;
         case 'object':
           assertIsDefined(
             schema.properties,
             'schema.properties must be defined',
           );
-          return `${indentSpaces(indent)}${propertyName}?: ${toTypescriptType(
+          return `${indentSpaces(indent)}${propertyKey(
+            propertyName,
+          )}?: ${toTypescriptType(
             schema.properties as Record<string, JSONSchema>,
             indent + 1,
           )}`;
         default:
-          return `${indentSpaces(indent)}${propertyName}?: ${[
+          return `${indentSpaces(indent)}${propertyKey(propertyName)}?: ${[
             ...uniqueTypes(schema),
           ].join(' | ')}`;
       }

packages/mongodb-schema/test/bloated.test.ts

@@ -2,6 +2,7 @@ import assert from 'assert';
 import { Binary, Code } from 'bson';
 
 import type { PrimitiveSchemaType } from '../src/schema-analyzer';
+import { SchemaAnalyzer } from '../src/schema-analyzer';
 import getSchema from '../src';
 
 function generateRandomString(length: number) {
@@ -139,4 +140,45 @@ describe('bloated documents', function () {
       }
     });
   });
+
+  describe('abort signal', function () {
+    // `analyzeDoc` is exercised directly here: `getCompletedSchemaAnalyzer`
+    // checks the signal in between documents, which would hide whether the
+    // signal is honoured *during* the analysis of a single document.
+    const document = {
+      field1: {
+        field2: 'abc',
+      },
+      field3: 'bca',
+    };
+
+    it('aborts during the analysis of a document with the signal reason', async function () {
+      const controller = new AbortController();
+      const reason = new Error('Analysis no longer needed');
+      controller.abort(reason);
+
+      const analyzer = new SchemaAnalyzer({ signal: controller.signal });
+      await assert.rejects(analyzer.analyzeDoc(document), reason);
+    });
+
+    it("aborts during the analysis of a document with the signal's default reason", async function () {
+      const controller = new AbortController();
+      controller.abort();
+
+      const analyzer = new SchemaAnalyzer({ signal: controller.signal });
+      await assert.rejects(analyzer.analyzeDoc(document), {
+        name: 'AbortError',
+      });
+    });
+
+    it('does not abort while the signal is not aborted', async function () {
+      const controller = new AbortController();
+
+      const analyzer = new SchemaAnalyzer({ signal: controller.signal });
+      await analyzer.analyzeDoc(document);
+
+      const fieldNames = analyzer.getResult().fields.map((v) => v.name);
+      assert.deepStrictEqual(fieldNames, ['field1', 'field3']);
+    });
+  });
 });

packages/mongodb-schema/test/field-order.test.ts

@@ -34,4 +34,19 @@ describe('order of fields', function () {
       ['a', 'b', 'Ca', 'cb', 'cC'],
     );
   });
+  it('should keep keys that only differ in case in the order they appear in', async function () {
+    // The comparator treats these as equal, so the sort - being stable - has to
+    // leave them in the order the documents introduced them in.
+    const upperFirst = await getSchema([{ cb: 1, Ca: 1, ca: 1, a: 1 }]);
+    assert.deepEqual(
+      upperFirst.fields.map((v) => v.name),
+      ['a', 'Ca', 'ca', 'cb'],
+    );
+
+    const lowerFirst = await getSchema([{ cb: 1, ca: 1, Ca: 1, a: 1 }]);
+    assert.deepEqual(
+      lowerFirst.fields.map((v) => v.name),
+      ['a', 'ca', 'Ca', 'cb'],
+    );
+  });
 });

packages/mongodb-schema/test/integration/generate-and-validate.test.ts

@@ -54,6 +54,8 @@ describe('Documents -> Generate schema -> Validate Documents against the schema'
 });
 
 describe('With a MongoDB Cluster', function () {
+  this.timeout(30_000);
+
   let client: MongoClient;
   let db: Db;
   const cluster = mochaTestServer();