Merge pull request #7641 from googleapis/feat-implicit-orderby

feat(firestore): global option to turn on implicit orderby

Author: wu-hui

.DS_Store

@@ -783,6 +783,13 @@ export class Firestore implements firestore.Firestore {
       validateBoolean('settings.ssl', settings.ssl);
     }
 
+    if (settings.alwaysUseImplicitOrderBy !== undefined) {
+      validateBoolean(
+        'settings.alwaysUseImplicitOrderBy',
+        settings.alwaysUseImplicitOrderBy,
+      );
+    }
+
     if (settings.maxIdleChannels !== undefined) {
       validateInteger('settings.maxIdleChannels', settings.maxIdleChannels, {
         minValue: 0,
@@ -845,6 +852,14 @@ export class Firestore implements firestore.Firestore {
     return this._databaseId || DEFAULT_DATABASE_ID;
   }
 
+  /**
+   * Whether to always use implicit order by clauses.
+   * @internal
+   */
+  get alwaysUseImplicitOrderBy(): boolean {
+    return this._settings.alwaysUseImplicitOrderBy ?? false;
+  }
+
   /**
    * Returns the root path of the database. Validates that
    * `initializeIfNeeded()` was called before.

handwritten/firestore/dev/src/index.ts

@@ -1475,6 +1475,7 @@ export class Query<
   toProto(
     transactionOrReadTime?: Uint8Array | Timestamp | api.ITransactionOptions,
     explainOptions?: firestore.ExplainOptions,
+    forceImplicitOrderBy?: boolean,
   ): api.IRunQueryRequest {
     const projectId = this.firestore.projectId;
     const databaseId = this.firestore.databaseId;
@@ -1483,18 +1484,18 @@ export class Query<
       databaseId,
     );
 
-    const structuredQuery = this.toStructuredQuery();
+    const structuredQuery = this.toStructuredQuery(forceImplicitOrderBy);
 
     // For limitToLast queries, the structured query has to be translated to a version with
     // reversed ordered, and flipped startAt/endAt to work properly.
     if (this._queryOptions.limitType === LimitType.Last) {
-      if (!this._queryOptions.hasFieldOrders()) {
-        throw new Error(
-          'limitToLast() queries require specifying at least one orderBy() clause.',
-        );
-      }
+      const forceImplicit =
+        forceImplicitOrderBy || this._firestore.alwaysUseImplicitOrderBy;
+      const fieldOrders = forceImplicit
+        ? this.createImplicitOrderBy()
+        : this._queryOptions.fieldOrders;
 
-      structuredQuery.orderBy = this._queryOptions.fieldOrders!.map(order => {
+      structuredQuery.orderBy = fieldOrders.map(order => {
         // Flip the orderBy directions since we want the last results
         const dir =
           order.direction === 'DESCENDING' ? 'ASCENDING' : 'DESCENDING';
@@ -1564,7 +1565,9 @@ export class Query<
     return bundledQuery;
   }
 
-  private toStructuredQuery(): api.IStructuredQuery {
+  private toStructuredQuery(
+    forceImplicitOrderBy?: boolean,
+  ): api.IStructuredQuery {
     const structuredQuery: api.IStructuredQuery = {
       from: [{}],
     };
@@ -1586,9 +1589,19 @@ export class Query<
       ).toProto();
     }
 
-    if (this._queryOptions.hasFieldOrders()) {
-      structuredQuery.orderBy = this._queryOptions.fieldOrders.map(o =>
-        o.toProto(),
+    // orders
+    const forceImplicit =
+      forceImplicitOrderBy || this._firestore.alwaysUseImplicitOrderBy;
+    let fieldOrders = this._queryOptions.fieldOrders;
+    if (forceImplicit) {
+      fieldOrders = this.createImplicitOrderBy();
+    }
+
+    if (fieldOrders.length > 0) {
+      structuredQuery.orderBy = fieldOrders.map(o => o.toProto());
+    } else if (this._queryOptions.limitType === LimitType.Last) {
+      throw new Error(
+        'limitToLast() queries require specifying at least one orderBy() clause.',
       );
     }
 

handwritten/firestore/dev/src/reference/query.ts

@@ -941,7 +941,7 @@ export class QueryWatch<
   }
 
   getTarget(resumeToken?: Uint8Array): google.firestore.v1.ITarget {
-    const query = this.query.toProto();
+    const query = this.query.toProto(undefined, undefined, true);
     return {query, targetId: WATCH_TARGET_ID, resumeToken};
   }
 }

handwritten/firestore/dev/src/watch.ts

@@ -2291,5 +2291,55 @@ describe.skipClassic('Query and Pipeline Compare - Enterprise DB', () => {
       expectDocs(await compareQueryAndPipeline(query), 'doc2', 'doc3', 'doc4');
       expectDocs(await queryWithCursor.get(), 'doc2', 'doc3', 'doc4');
     });
+
+    it('alwaysUseImplicitOrderBy returns same results', async () => {
+      const collection = getTestRoot();
+      const docs = {
+        doc01: {sort: 1},
+        doc02: {sort: 2},
+        doc03: {sort: 3},
+        doc04: {sort: 4},
+        doc05: {sort: 5},
+        doc06: {sort: 6},
+        doc07: {sort: 7},
+        doc08: {sort: 8},
+        doc09: {sort: 9},
+        doc10: {sort: 10},
+      };
+
+      for (const [id, data] of Object.entries(docs)) {
+        await collection.doc(id).set(data);
+      }
+
+      const expectedOrder = [
+        'doc02',
+        'doc03',
+        'doc04',
+        'doc05',
+        'doc06',
+        'doc07',
+        'doc08',
+        'doc09',
+        'doc10',
+      ];
+
+      // TODO: This test should run against both standard and enterprise
+      // and verify the results respectively
+      // const originalQuery = collection.where('sort', '>', 1);
+      // const originalSnapshot = await originalQuery.get();
+      // const originalResult = originalSnapshot.docs.map(d => d.id);
+
+      const modifiedFirestore = new Firestore({
+        ...firestore.settings,
+        alwaysUseImplicitOrderBy: true,
+      });
+      const modifiedCollection = modifiedFirestore.collection(collection.id);
+      const query = modifiedCollection.where('sort', '>', 1);
+      const snapshot = await query.get();
+      const result = snapshot.docs.map(d => d.id);
+
+      // since alwaysUseImplicitOrderBy is true, we expect strict ordering.
+      expect(result).to.deep.equal(expectedOrder);
+    });
   });
 });

handwritten/firestore/dev/system-test/query.ts

@@ -25,6 +25,8 @@ import {google} from '../protos/firestore_v1_proto_api';
 import api = google.firestore.v1;
 import * as chaiAsPromised from 'chai-as-promised';
 import {setTimeoutHandler} from '../src/backoff';
+import * as extend from 'extend';
+
 use(chaiAsPromised);
 
 describe('aggregate query interface', () => {
@@ -100,6 +102,57 @@ describe('aggregate query interface', () => {
     });
   });
 
+  it('supports alwaysUseImplicitOrderBy', async () => {
+    const result: api.IRunAggregationQueryResponse = {
+      result: {
+        aggregateFields: {
+          aggregate_0: {integerValue: '99'},
+        },
+      },
+      readTime: {seconds: 5, nanos: 6},
+    };
+    const overrides: ApiOverride = {
+      runAggregationQuery: request => {
+        let actualStructuredQuery =
+          request!.structuredAggregationQuery?.structuredQuery;
+        actualStructuredQuery = extend(true, {}, actualStructuredQuery);
+        expect(actualStructuredQuery).to.deep.equal({
+          from: [{collectionId: 'collectionId'}],
+          where: {
+            fieldFilter: {
+              field: {fieldPath: 'foo'},
+              op: 'GREATER_THAN' as api.StructuredQuery.FieldFilter.Operator,
+              value: {stringValue: 'bar'},
+            },
+          },
+          orderBy: [
+            {
+              direction: 'ASCENDING' as api.StructuredQuery.Direction,
+              field: {fieldPath: 'foo'},
+            },
+            {
+              direction: 'ASCENDING' as api.StructuredQuery.Direction,
+              field: {fieldPath: '__name__'},
+            },
+          ],
+        });
+        return stream(result);
+      },
+    };
+
+    firestore = await createInstance(overrides, {
+      alwaysUseImplicitOrderBy: true,
+    });
+
+    const query = firestore
+      .collection('collectionId')
+      .where('foo', '>', 'bar')
+      .count();
+    return query.get().then(results => {
+      expect(results.data().count).to.be.equal(99);
+    });
+  });
+
   it('handles stream exception at initialization', async () => {
     let attempts = 0;
     const query = firestore.collection('collectionId').count();

handwritten/firestore/dev/test/aggregateQuery.ts

@@ -609,6 +609,48 @@ describe('query interface', () => {
     expect(results.readTime.isEqual(new Timestamp(5, 6))).to.be.true;
   });
 
+  it('supports alwaysUseImplicitOrderBy with limitToLast', async () => {
+    const overrides: ApiOverride = {
+      runQuery: request => {
+        queryEquals(
+          request,
+          where(fieldFilters('foo', 'GREATER_THAN_OR_EQUAL', 'bar')),
+          {
+            orderBy: [
+              {
+                field: {fieldPath: 'foo'},
+                direction: 'DESCENDING',
+              },
+              {
+                field: {fieldPath: '__name__'},
+                direction: 'DESCENDING',
+              },
+            ],
+            limit: {value: 1},
+          },
+        );
+        return stream({readTime: {seconds: 5, nanos: 6}});
+      },
+    };
+
+    firestore = await createInstance(overrides, {
+      alwaysUseImplicitOrderBy: true,
+    });
+    const query = firestore
+      .collection('collectionId')
+      .where('foo', '>=', 'bar')
+      .limitToLast(1);
+    await query.get();
+  });
+
+  it('throws for limitToLast without orderBy', async () => {
+    firestore = await createInstance();
+    const query = firestore.collection('collectionId').limitToLast(1);
+    expect(() => query.toProto()).to.throw(
+      'limitToLast() queries require specifying at least one orderBy() clause.',
+    );
+  });
+
   it('retries on stream failure', async () => {
     let attempts = 0;
     const overrides: ApiOverride = {