Skip to content

Commit c9abf67

Browse files
committed
feat(firestore): global option to turn on implicit orderby
1 parent f82d304 commit c9abf67

8 files changed

Lines changed: 277 additions & 10 deletions

File tree

handwritten/firestore/dev/src/index.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -783,6 +783,13 @@ export class Firestore implements firestore.Firestore {
783783
validateBoolean('settings.ssl', settings.ssl);
784784
}
785785

786+
if (settings.alwaysUseImplicitOrderBy !== undefined) {
787+
validateBoolean(
788+
'settings.alwaysUseImplicitOrderBy',
789+
settings.alwaysUseImplicitOrderBy,
790+
);
791+
}
792+
786793
if (settings.maxIdleChannels !== undefined) {
787794
validateInteger('settings.maxIdleChannels', settings.maxIdleChannels, {
788795
minValue: 0,
@@ -845,6 +852,14 @@ export class Firestore implements firestore.Firestore {
845852
return this._databaseId || DEFAULT_DATABASE_ID;
846853
}
847854

855+
/**
856+
* Whether to always use implicit order by clauses.
857+
* @internal
858+
*/
859+
get alwaysUseImplicitOrderBy(): boolean {
860+
return this._settings.alwaysUseImplicitOrderBy ?? false;
861+
}
862+
848863
/**
849864
* Returns the root path of the database. Validates that
850865
* `initializeIfNeeded()` was called before.

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

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1475,6 +1475,7 @@ export class Query<
14751475
toProto(
14761476
transactionOrReadTime?: Uint8Array | Timestamp | api.ITransactionOptions,
14771477
explainOptions?: firestore.ExplainOptions,
1478+
forceImplicitOrderBy?: boolean,
14781479
): api.IRunQueryRequest {
14791480
const projectId = this.firestore.projectId;
14801481
const databaseId = this.firestore.databaseId;
@@ -1483,18 +1484,25 @@ export class Query<
14831484
databaseId,
14841485
);
14851486

1486-
const structuredQuery = this.toStructuredQuery();
1487+
const structuredQuery = this.toStructuredQuery(forceImplicitOrderBy);
14871488

14881489
// For limitToLast queries, the structured query has to be translated to a version with
14891490
// reversed ordered, and flipped startAt/endAt to work properly.
14901491
if (this._queryOptions.limitType === LimitType.Last) {
1491-
if (!this._queryOptions.hasFieldOrders()) {
1492+
const forceImplicit =
1493+
forceImplicitOrderBy ||
1494+
(this._firestore as any).alwaysUseImplicitOrderBy;
1495+
const fieldOrders = forceImplicit
1496+
? this.createImplicitOrderBy()
1497+
: this._queryOptions.fieldOrders;
1498+
1499+
if (!fieldOrders || fieldOrders.length === 0) {
14921500
throw new Error(
14931501
'limitToLast() queries require specifying at least one orderBy() clause.',
14941502
);
14951503
}
14961504

1497-
structuredQuery.orderBy = this._queryOptions.fieldOrders!.map(order => {
1505+
structuredQuery.orderBy = fieldOrders.map(order => {
14981506
// Flip the orderBy directions since we want the last results
14991507
const dir =
15001508
order.direction === 'DESCENDING' ? 'ASCENDING' : 'DESCENDING';
@@ -1564,7 +1572,9 @@ export class Query<
15641572
return bundledQuery;
15651573
}
15661574

1567-
private toStructuredQuery(): api.IStructuredQuery {
1575+
private toStructuredQuery(
1576+
forceImplicitOrderBy?: boolean,
1577+
): api.IStructuredQuery {
15681578
const structuredQuery: api.IStructuredQuery = {
15691579
from: [{}],
15701580
};
@@ -1586,10 +1596,17 @@ export class Query<
15861596
).toProto();
15871597
}
15881598

1589-
if (this._queryOptions.hasFieldOrders()) {
1590-
structuredQuery.orderBy = this._queryOptions.fieldOrders.map(o =>
1591-
o.toProto(),
1592-
);
1599+
// orders
1600+
const forceImplicit =
1601+
forceImplicitOrderBy ||
1602+
(this._firestore as any).alwaysUseImplicitOrderBy;
1603+
let fieldOrders = this._queryOptions.fieldOrders;
1604+
if (forceImplicit) {
1605+
fieldOrders = this.createImplicitOrderBy();
1606+
}
1607+
1608+
if (fieldOrders.length > 0) {
1609+
structuredQuery.orderBy = fieldOrders.map(o => o.toProto());
15931610
}
15941611

15951612
structuredQuery.startAt = this.toCursor(this._queryOptions.startAt);

handwritten/firestore/dev/src/watch.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -941,7 +941,7 @@ export class QueryWatch<
941941
}
942942

943943
getTarget(resumeToken?: Uint8Array): google.firestore.v1.ITarget {
944-
const query = this.query.toProto();
944+
const query = this.query.toProto(undefined, undefined, true);
945945
return {query, targetId: WATCH_TARGET_ID, resumeToken};
946946
}
947947
}

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

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2291,5 +2291,66 @@ describe.skipClassic('Query and Pipeline Compare - Enterprise DB', () => {
22912291
expectDocs(await compareQueryAndPipeline(query), 'doc2', 'doc3', 'doc4');
22922292
expectDocs(await queryWithCursor.get(), 'doc2', 'doc3', 'doc4');
22932293
});
2294+
2295+
it('alwaysUseImplicitOrderBy returns same results', async () => {
2296+
const collection = getTestRoot();
2297+
const docs = {
2298+
doc01: {sort: 1},
2299+
doc02: {sort: 2},
2300+
doc03: {sort: 3},
2301+
doc04: {sort: 4},
2302+
doc05: {sort: 5},
2303+
doc06: {sort: 6},
2304+
doc07: {sort: 7},
2305+
doc08: {sort: 8},
2306+
doc09: {sort: 9},
2307+
doc10: {sort: 10},
2308+
};
2309+
2310+
for (const [id, data] of Object.entries(docs)) {
2311+
await collection.doc(id).set(data);
2312+
}
2313+
2314+
const expectedOrder = [
2315+
'doc02',
2316+
'doc03',
2317+
'doc04',
2318+
'doc05',
2319+
'doc06',
2320+
'doc07',
2321+
'doc08',
2322+
'doc09',
2323+
'doc10',
2324+
];
2325+
2326+
const originalQuery = collection.where('sort', '>', 1);
2327+
const originalSnapshot = await originalQuery.get();
2328+
const originalResult = originalSnapshot.docs.map(d => d.id);
2329+
2330+
// In Enterprise, the order might not be strictly deterministic without implicit order by
2331+
// unless it's Standard edition. Standard Edition always has implicit order by.
2332+
// This test is intended to run against Enterprise where possible.
2333+
// If it's Standard, originalResult will be equal to expectedOrder.
2334+
// If it's Enterprise, it might not be.
2335+
2336+
const modifiedFirestore = new Firestore({
2337+
...firestore.settings,
2338+
alwaysUseImplicitOrderBy: true,
2339+
} as any);
2340+
const modifiedCollection = modifiedFirestore.collection(collection.id);
2341+
const query = modifiedCollection.where('sort', '>', 1);
2342+
const snapshot = await query.get();
2343+
const result = snapshot.docs.map(d => d.id);
2344+
2345+
// since alwaysUseImplicitOrderBy is true, we expect strict ordering.
2346+
expect(result).to.deep.equal(expectedOrder);
2347+
2348+
if (originalResult.length === expectedOrder.length) {
2349+
// We can't easily check if it's Enterprise vs Standard from within the test
2350+
// without more setup, but we can at least verify that our new option
2351+
// produces the expected deterministic result.
2352+
expect(result).to.have.members(originalResult);
2353+
}
2354+
});
22942355
});
22952356
});

handwritten/firestore/dev/test/aggregateQuery.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import {google} from '../protos/firestore_v1_proto_api';
2525
import api = google.firestore.v1;
2626
import * as chaiAsPromised from 'chai-as-promised';
2727
import {setTimeoutHandler} from '../src/backoff';
28+
import * as extend from 'extend';
29+
2830
use(chaiAsPromised);
2931

3032
describe('aggregate query interface', () => {
@@ -100,6 +102,59 @@ describe('aggregate query interface', () => {
100102
});
101103
});
102104

105+
it('supports alwaysUseImplicitOrderBy', async () => {
106+
const result: api.IRunAggregationQueryResponse = {
107+
result: {
108+
aggregateFields: {
109+
aggregate_0: {integerValue: '99'},
110+
},
111+
},
112+
readTime: {seconds: 5, nanos: 6},
113+
};
114+
const overrides: ApiOverride = {
115+
runAggregationQuery: request => {
116+
let actualStructuredQuery =
117+
request!.structuredAggregationQuery?.structuredQuery;
118+
actualStructuredQuery = extend(true, {}, actualStructuredQuery);
119+
expect(actualStructuredQuery).to.deep.equal(
120+
{
121+
from: [{collectionId: 'collectionId'}],
122+
where: {
123+
fieldFilter: {
124+
field: {fieldPath: 'foo'},
125+
op: 'GREATER_THAN' as api.StructuredQuery.FieldFilter.Operator,
126+
value: {stringValue: 'bar'},
127+
},
128+
},
129+
orderBy: [
130+
{
131+
direction: 'ASCENDING' as api.StructuredQuery.Direction,
132+
field: {fieldPath: 'foo'},
133+
},
134+
{
135+
direction: 'ASCENDING' as api.StructuredQuery.Direction,
136+
field: {fieldPath: '__name__'},
137+
},
138+
],
139+
},
140+
);
141+
return stream(result);
142+
},
143+
};
144+
145+
firestore = await createInstance(overrides, {
146+
alwaysUseImplicitOrderBy: true,
147+
} as any);
148+
149+
const query = firestore
150+
.collection('collectionId')
151+
.where('foo', '>', 'bar')
152+
.count();
153+
return query.get().then(results => {
154+
expect(results.data().count).to.be.equal(99);
155+
});
156+
});
157+
103158
it('handles stream exception at initialization', async () => {
104159
let attempts = 0;
105160
const query = firestore.collection('collectionId').count();

handwritten/firestore/dev/test/query.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,40 @@ describe('query interface', () => {
609609
expect(results.readTime.isEqual(new Timestamp(5, 6))).to.be.true;
610610
});
611611

612+
it('supports alwaysUseImplicitOrderBy with limitToLast', async () => {
613+
const overrides: ApiOverride = {
614+
runQuery: request => {
615+
queryEquals(
616+
request,
617+
where(fieldFilters('foo', 'GREATER_THAN_OR_EQUAL', 'bar')),
618+
{
619+
orderBy: [
620+
{
621+
field: {fieldPath: 'foo'},
622+
direction: 'DESCENDING',
623+
},
624+
{
625+
field: {fieldPath: '__name__'},
626+
direction: 'DESCENDING',
627+
},
628+
],
629+
limit: {value: 1},
630+
},
631+
);
632+
return stream({readTime: {seconds: 5, nanos: 6}});
633+
},
634+
};
635+
636+
firestore = await createInstance(overrides, {
637+
alwaysUseImplicitOrderBy: true,
638+
} as any);
639+
const query = firestore
640+
.collection('collectionId')
641+
.where('foo', '>=', 'bar')
642+
.limitToLast(1);
643+
await query.get();
644+
});
645+
612646
it('retries on stream failure', async () => {
613647
let attempts = 0;
614648
const overrides: ApiOverride = {

handwritten/firestore/dev/test/watch.ts

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -597,6 +597,12 @@ describe('Query watch', () => {
597597
parent: `projects/${PROJECT_ID}/databases/(default)/documents`,
598598
structuredQuery: {
599599
from: [{collectionId: 'col'}],
600+
orderBy: [
601+
{
602+
direction: 'ASCENDING' as api.StructuredQuery.Direction,
603+
field: {fieldPath: '__name__'},
604+
},
605+
],
600606
},
601607
},
602608
targetId,
@@ -628,6 +634,12 @@ describe('Query watch', () => {
628634
},
629635
},
630636
},
637+
orderBy: [
638+
{
639+
direction: 'ASCENDING' as api.StructuredQuery.Direction,
640+
field: {fieldPath: '__name__'},
641+
},
642+
],
631643
},
632644
},
633645
targetId,
@@ -667,7 +679,16 @@ describe('Query watch', () => {
667679
parent: `projects/${PROJECT_ID}/databases/(default)/documents`,
668680
structuredQuery: {
669681
from: [{collectionId: 'col'}],
670-
orderBy: [{direction: 'DESCENDING', field: {fieldPath: 'foo'}}],
682+
orderBy: [
683+
{
684+
direction: 'DESCENDING' as api.StructuredQuery.Direction,
685+
field: {fieldPath: 'foo'},
686+
},
687+
{
688+
direction: 'DESCENDING' as api.StructuredQuery.Direction,
689+
field: {fieldPath: '__name__'},
690+
},
691+
],
671692
},
672693
},
673694
targetId,
@@ -786,6 +807,60 @@ describe('Query watch', () => {
786807
);
787808
});
788809

810+
it('supports alwaysUseImplicitOrderBy', async () => {
811+
// Re-initialize with alwaysUseImplicitOrderBy: true
812+
firestore = await createInstance(
813+
{listen: () => listenCallback()},
814+
{alwaysUseImplicitOrderBy: true} as any,
815+
);
816+
const query = firestore.collection('col').where('foo', '>', 'bar');
817+
watchHelper = new WatchHelper(
818+
streamHelper,
819+
query,
820+
targetId,
821+
);
822+
823+
const expectedQueryJSON = {
824+
database: `projects/${PROJECT_ID}/databases/(default)`,
825+
addTarget: {
826+
query: {
827+
parent: `projects/${PROJECT_ID}/databases/(default)/documents`,
828+
structuredQuery: {
829+
from: [{collectionId: 'col'}],
830+
where: {
831+
fieldFilter: {
832+
field: {fieldPath: 'foo'},
833+
op: 'GREATER_THAN' as api.StructuredQuery.FieldFilter.Operator,
834+
value: {stringValue: 'bar'},
835+
},
836+
},
837+
orderBy: [
838+
{
839+
direction: 'ASCENDING' as api.StructuredQuery.Direction,
840+
field: {fieldPath: 'foo'},
841+
},
842+
{
843+
direction: 'ASCENDING' as api.StructuredQuery.Direction,
844+
field: {fieldPath: '__name__'},
845+
},
846+
],
847+
},
848+
},
849+
targetId,
850+
},
851+
};
852+
853+
return watchHelper.runTest(
854+
expectedQueryJSON,
855+
() => {
856+
watchHelper.sendAddTarget(targetId);
857+
watchHelper.sendCurrent();
858+
watchHelper.sendSnapshot(1);
859+
return watchHelper.await('snapshot');
860+
},
861+
);
862+
});
863+
789864
it('rejects an unknown target', () => {
790865
return watchHelper.runFailedTest(
791866
collQueryJSON(),

0 commit comments

Comments
 (0)