fix: avoid adding duplicate orderBys in startAfter() #1453
Conversation
product-auto-label
bot
added
the
api: firestore
google-cla
bot
added
the
cla: yes
thebrianchen
force-pushed
the
bc/rd-logs
branch
from
e8ff476
to
2cf01c2
Compare
Codecov Report
@@ Coverage Diff @@
## bc/rc-main #1453 +/- ##
==============================================
+ Coverage 98.22% 98.52% +0.30%
==============================================
Files 32 32
Lines 19565 19684 +119
Branches 1372 1303 -69
==============================================
+ Hits 19217 19393 +176
+ Misses 344 286 -58
- Partials 4 5 +1
Continue to review full report at Codecov.
|
| nanos?: number | ||
| ): protobuf.ITimestamp { | ||
| if (seconds === undefined && nanos === undefined) { | ||
| return {seconds: '5', nanos: 6}; |
There was a problem hiding this comment.
We have to cast to seconds since the Timestamp class always converts it to string for some reason. Why do we do this?
There was a problem hiding this comment.
Proto3 JSON represents int64 as strings, and the seconds component of Timestamps is an int64.
dev/src/reference.ts
Outdated
| @@ -1521,6 +1521,9 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> { | |||
| // If no explicit ordering is specified, use the first inequality to | |||
| // define an implicit order. | |||
| for (const fieldFilter of this._queryOptions.fieldFilters) { | |||
| if (FieldPath.documentId().isEqual(fieldFilter.field)) { | |||
| hasDocumentId = true; | |||
| } | |||
There was a problem hiding this comment.
I think this would be cleaner if we ran this in two steps:
// If no explicit ordering is specified, use the first inequality to
// define an implicit order.
if (fieldOrders.length === 0) {
for (const fieldFilter of this._queryOptions.fieldFilters) {
if (fieldFilter.isInequalityFilter()) {
fieldOrders.push(new FieldOrder(fieldFilter.field));
break;
}
}
}
const hasDocumentId = !!fieldOrders.find(
fieldOrder => FieldPath.documentId().isEqual(fieldOrder.field));
if (!hasDocumentId) {
...
}
| nanos?: number | ||
| ): protobuf.ITimestamp { | ||
| if (seconds === undefined && nanos === undefined) { | ||
| return {seconds: '5', nanos: 6}; |
There was a problem hiding this comment.
Proto3 JSON represents int64 as strings, and the seconds component of Timestamps is an int64.
dev/test/query.ts
Outdated
| queryEqualsWithParent(actual, '', ...protoComponents); | ||
| queryEqualsWithParentAndReadTime( | ||
| actual, | ||
| '', |
There was a problem hiding this comment.
What is ''? Can you add a comment?
Fixing a bug in recursive delete where invalid queries would be created on stream errors due to adding two
orderBy('__name__', ASCENDING)clauses when callingstartAfter()(location). The private methodcreateImplicitOrderByadds an implicit orderBy if there is is no documentId on the fieldOrder fields, but we didn't check the case where the documentId comes from the fieldPaths.