Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid adding duplicate orderBys in startAfter() #1453

Merged
merged 2 commits into from
Mar 29, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions dev/src/reference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
    ...
   }


if (fieldFilter.isInequalityFilter()) {
fieldOrders.push(new FieldOrder(fieldFilter.field));
break;
Expand Down
29 changes: 27 additions & 2 deletions dev/test/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {
} from './util/helpers';

import api = google.firestore.v1;
import protobuf = google.protobuf;

const PROJECT_ID = 'test-project';
const DATABASE_ROOT = `projects/${PROJECT_ID}/databases/(default)`;
Expand Down Expand Up @@ -285,9 +286,24 @@ function endAt(
return {endAt: cursor};
}

export function queryEqualsWithParent(
/**
* Returns the timestamp value for the provided readTimes, or the default
* readTime value used in tests if no values are provided.
*/
export function readTime(
seconds?: number,
nanos?: number
): protobuf.ITimestamp {
if (seconds === undefined && nanos === undefined) {
return {seconds: '5', nanos: 6};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to cast to seconds since the Timestamp class always converts it to string for some reason. Why do we do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proto3 JSON represents int64 as strings, and the seconds component of Timestamps is an int64.

}
return {seconds: String(seconds), nanos: nanos};
}

export function queryEqualsWithParentAndReadTime(
actual: api.IRunQueryRequest | undefined,
parent: string,
readTime?: protobuf.ITimestamp,
...protoComponents: api.IStructuredQuery[]
): void {
expect(actual).to.not.be.undefined;
Expand Down Expand Up @@ -315,6 +331,10 @@ export function queryEqualsWithParent(
];
}

if (readTime) {
query.readTime = readTime;
}

// 'extend' removes undefined fields in the request object. The backend
// ignores these fields, but we need to manually strip them before we compare
// the expected and the actual request.
Expand All @@ -326,7 +346,12 @@ export function queryEquals(
actual: api.IRunQueryRequest | undefined,
...protoComponents: api.IStructuredQuery[]
): void {
queryEqualsWithParent(actual, '', ...protoComponents);
queryEqualsWithParentAndReadTime(
actual,
'',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is ''? Can you add a comment?

/* readTime= */ undefined,
...protoComponents
);
}

function bundledQueryEquals(
Expand Down
50 changes: 47 additions & 3 deletions dev/test/recursive-delete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ import {
import {
allDescendants,
fieldFilters,
orderBy,
queryEquals,
queryEqualsWithParent,
queryEqualsWithParentAndReadTime,
readTime,
result,
select,
startAt as queryStartAt,
} from './query';
import {
createRequest,
Expand Down Expand Up @@ -147,9 +150,10 @@ describe('recursiveDelete() method:', () => {
it('for nested collections', async () => {
const overrides: ApiOverride = {
runQuery: req => {
queryEqualsWithParent(
queryEqualsWithParentAndReadTime(
req,
'root/doc',
/* readTime= */ undefined,
select('__name__'),
allDescendants(/* kindless= */ true),
fieldFilters(
Expand All @@ -173,9 +177,10 @@ describe('recursiveDelete() method:', () => {
it('documents', async () => {
const overrides: ApiOverride = {
runQuery: req => {
queryEqualsWithParent(
queryEqualsWithParentAndReadTime(
req,
'root/doc',
/* readTime= */ undefined,
select('__name__'),
allDescendants(/* kindless= */ true)
);
Expand All @@ -187,6 +192,45 @@ describe('recursiveDelete() method:', () => {
firestore = await createInstance(overrides);
return firestore.recursiveDelete(firestore.doc('root/doc'));
});

it('creates retry query after stream exception with last received doc', async () => {
let callCount = 0;
const overrides: ApiOverride = {
runQuery: request => {
callCount++;
if (callCount === 1) {
return stream(result('doc1'), new Error('failed in test'));
} else {
queryEqualsWithParentAndReadTime(
request,
/* parent= */ '',
readTime(),
select('__name__'),
allDescendants(/* kindless= */ true),
orderBy('__name__', 'ASCENDING'),
queryStartAt(false, {
referenceValue:
`projects/${PROJECT_ID}/databases/(default)/` +
'documents/collectionId/doc1',
}),
fieldFilters(
'__name__',
'GREATER_THAN_OR_EQUAL',
startAt('root'),
'__name__',
'LESS_THAN',
endAt('root')
)
);
return stream();
}
},
batchWrite: () => response(successResponse(1)),
};

const firestore = await createInstance(overrides);
await firestore.recursiveDelete(firestore.collection('root'));
});
});

describe('deletes', () => {
Expand Down