diff --git a/packages/firestore/src/core/target.ts b/packages/firestore/src/core/target.ts index 7cc7087ba94..72a760a4529 100644 --- a/packages/firestore/src/core/target.ts +++ b/packages/firestore/src/core/target.ts @@ -31,13 +31,13 @@ import { isReferenceValue, MAX_VALUE, MIN_VALUE, + lowerBoundCompare, typeOrder, + upperBoundCompare, valueCompare, valueEquals, valuesGetLowerBound, - valuesGetUpperBound, - valuesMax, - valuesMin + valuesGetUpperBound } from '../model/values'; import { Value as ProtoValue } from '../protos/firestore_proto_api'; import { debugAssert, debugCast, fail } from '../util/assert'; @@ -293,13 +293,13 @@ export function targetGetNotInValues( /** * Returns a lower bound of field values that can be used as a starting point to - * scan the index defined by `fieldIndex`. Returns `null` if no lower bound + * scan the index defined by `fieldIndex`. Returns `MIN_VALUE` if no lower bound * exists. */ export function targetGetLowerBound( target: Target, fieldIndex: FieldIndex -): Bound | null { +): Bound { const values: ProtoValue[] = []; let inclusive = true; @@ -311,10 +311,6 @@ export function targetGetLowerBound( ? targetGetAscendingBound(target, segment.fieldPath, target.startAt) : targetGetDescendingBound(target, segment.fieldPath, target.startAt); - if (!segmentBound.value) { - // No lower bound exists - return null; - } values.push(segmentBound.value); inclusive &&= segmentBound.inclusive; } @@ -323,13 +319,13 @@ export function targetGetLowerBound( /** * Returns an upper bound of field values that can be used as an ending point - * when scanning the index defined by `fieldIndex`. Returns `null` if no + * when scanning the index defined by `fieldIndex`. Returns `MAX_VALUE` if no * upper bound exists. */ export function targetGetUpperBound( target: Target, fieldIndex: FieldIndex -): Bound | null { +): Bound { const values: ProtoValue[] = []; let inclusive = true; @@ -341,10 +337,6 @@ export function targetGetUpperBound( ? targetGetDescendingBound(target, segment.fieldPath, target.endAt) : targetGetAscendingBound(target, segment.fieldPath, target.endAt); - if (!segmentBound.value) { - // No upper bound exists - return null; - } values.push(segmentBound.value); inclusive &&= segmentBound.inclusive; } @@ -360,13 +352,14 @@ function targetGetAscendingBound( target: Target, fieldPath: FieldPath, bound: Bound | null -): { value: ProtoValue | undefined; inclusive: boolean } { - let value: ProtoValue | undefined = undefined; +): { value: ProtoValue; inclusive: boolean } { + let value: ProtoValue = MIN_VALUE; + let inclusive = true; // Process all filters to find a value for the current field segment for (const fieldFilter of targetGetFieldFiltersForPath(target, fieldPath)) { - let filterValue: ProtoValue | undefined = undefined; + let filterValue: ProtoValue = MIN_VALUE; let filterInclusive = true; switch (fieldFilter.op) { @@ -391,7 +384,12 @@ function targetGetAscendingBound( // Remaining filters cannot be used as lower bounds. } - if (valuesMax(value, filterValue) === filterValue) { + if ( + lowerBoundCompare( + { value, inclusive }, + { value: filterValue, inclusive: filterInclusive } + ) < 0 + ) { value = filterValue; inclusive = filterInclusive; } @@ -404,7 +402,12 @@ function targetGetAscendingBound( const orderBy = target.orderBy[i]; if (orderBy.field.isEqual(fieldPath)) { const cursorValue = bound.position[i]; - if (valuesMax(value, cursorValue) === cursorValue) { + if ( + lowerBoundCompare( + { value, inclusive }, + { value: cursorValue, inclusive: bound.inclusive } + ) < 0 + ) { value = cursorValue; inclusive = bound.inclusive; } @@ -424,13 +427,13 @@ function targetGetDescendingBound( target: Target, fieldPath: FieldPath, bound: Bound | null -): { value: ProtoValue | undefined; inclusive: boolean } { - let value: ProtoValue | undefined = undefined; +): { value: ProtoValue; inclusive: boolean } { + let value: ProtoValue = MAX_VALUE; let inclusive = true; // Process all filters to find a value for the current field segment for (const fieldFilter of targetGetFieldFiltersForPath(target, fieldPath)) { - let filterValue: ProtoValue | undefined = undefined; + let filterValue: ProtoValue = MAX_VALUE; let filterInclusive = true; switch (fieldFilter.op) { @@ -456,7 +459,12 @@ function targetGetDescendingBound( // Remaining filters cannot be used as upper bounds. } - if (valuesMin(value, filterValue) === filterValue) { + if ( + upperBoundCompare( + { value, inclusive }, + { value: filterValue, inclusive: filterInclusive } + ) > 0 + ) { value = filterValue; inclusive = filterInclusive; } @@ -469,7 +477,12 @@ function targetGetDescendingBound( const orderBy = target.orderBy[i]; if (orderBy.field.isEqual(fieldPath)) { const cursorValue = bound.position[i]; - if (valuesMin(value, cursorValue) === cursorValue) { + if ( + upperBoundCompare( + { value, inclusive }, + { value: cursorValue, inclusive: bound.inclusive } + ) > 0 + ) { value = cursorValue; inclusive = bound.inclusive; } diff --git a/packages/firestore/src/local/indexeddb_index_manager.ts b/packages/firestore/src/local/indexeddb_index_manager.ts index 5fc10a25baf..c75a8ab21d4 100644 --- a/packages/firestore/src/local/indexeddb_index_manager.ts +++ b/packages/firestore/src/local/indexeddb_index_manager.ts @@ -285,9 +285,9 @@ export class IndexedDbIndexManager implements IndexManager { index!.indexId, arrayValues, lowerBoundEncoded, - !!lowerBound && lowerBound.inclusive, + lowerBound.inclusive, upperBoundEncoded, - !!upperBound && upperBound.inclusive, + upperBound.inclusive, notInEncoded ); return PersistencePromise.forEach( @@ -331,9 +331,9 @@ export class IndexedDbIndexManager implements IndexManager { private generateIndexRanges( indexId: number, arrayValues: ProtoValue[] | null, - lowerBounds: Uint8Array[] | null, + lowerBounds: Uint8Array[], lowerBoundInclusive: boolean, - upperBounds: Uint8Array[] | null, + upperBounds: Uint8Array[], upperBoundInclusive: boolean, notInValues: Uint8Array[] ): IDBKeyRange[] { @@ -343,10 +343,7 @@ export class IndexedDbIndexManager implements IndexManager { // combined with the values from the query bounds. const totalScans = (arrayValues != null ? arrayValues.length : 1) * - Math.max( - lowerBounds != null ? lowerBounds.length : 1, - upperBounds != null ? upperBounds.length : 1 - ); + Math.max(lowerBounds.length, upperBounds.length); const scansPerArrayElement = totalScans / (arrayValues != null ? arrayValues.length : 1); @@ -356,22 +353,18 @@ export class IndexedDbIndexManager implements IndexManager { ? this.encodeSingleElement(arrayValues[i / scansPerArrayElement]) : EMPTY_VALUE; - const lowerBound = lowerBounds - ? this.generateLowerBound( - indexId, - arrayValue, - lowerBounds[i % scansPerArrayElement], - lowerBoundInclusive - ) - : this.generateEmptyBound(indexId); - const upperBound = upperBounds - ? this.generateUpperBound( - indexId, - arrayValue, - upperBounds[i % scansPerArrayElement], - upperBoundInclusive - ) - : this.generateEmptyBound(indexId + 1); + const lowerBound = this.generateLowerBound( + indexId, + arrayValue, + lowerBounds[i % scansPerArrayElement], + lowerBoundInclusive + ); + const upperBound = this.generateUpperBound( + indexId, + arrayValue, + upperBounds[i % scansPerArrayElement], + upperBoundInclusive + ); const notInBound = notInValues.map(notIn => this.generateLowerBound( @@ -420,19 +413,6 @@ export class IndexedDbIndexManager implements IndexManager { return inclusive ? entry.successor() : entry; } - /** - * Generates an empty bound that scopes the index scan to the current index - * and user. - */ - private generateEmptyBound(indexId: number): IndexEntry { - return new IndexEntry( - indexId, - DocumentKey.empty(), - EMPTY_VALUE, - EMPTY_VALUE - ); - } - private getFieldIndex( transaction: PersistenceTransaction, target: Target @@ -572,11 +552,8 @@ export class IndexedDbIndexManager implements IndexManager { private encodeBound( fieldIndex: FieldIndex, target: Target, - bound: Bound | null - ): Uint8Array[] | null { - if (bound == null) { - return null; - } + bound: Bound + ): Uint8Array[] { return this.encodeValues(fieldIndex, target, bound.position); } diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts index 3c6c4b648ec..3466c2c1ee6 100644 --- a/packages/firestore/src/model/values.ts +++ b/packages/firestore/src/model/values.ts @@ -78,7 +78,7 @@ export function typeOrder(value: Value): TypeOrder { if (isServerTimestamp(value)) { return TypeOrder.ServerTimestampValue; } else if (isMaxValue(value)) { - return TypeOrder.ArrayValue; + return TypeOrder.MaxValue; } return TypeOrder.ObjectValue; } else { @@ -350,6 +350,14 @@ function compareArrays(left: ArrayValue, right: ArrayValue): number { } function compareMaps(left: MapValue, right: MapValue): number { + if (left === MAX_VALUE.mapValue && right === MAX_VALUE.mapValue) { + return 0; + } else if (left === MAX_VALUE.mapValue) { + return 1; + } else if (right === MAX_VALUE.mapValue) { + return -1; + } + const leftMap = left.fields || {}; const leftKeys = Object.keys(leftMap); const rightMap = right.fields || {}; @@ -670,28 +678,38 @@ export function valuesGetUpperBound(value: Value): Value { } } -export function valuesMax( - left: Value | undefined, - right: Value | undefined -): Value | undefined { - if (left === undefined) { - return right; - } else if (right === undefined) { - return left; - } else { - return valueCompare(left, right) > 0 ? left : right; +export function lowerBoundCompare( + left: { value: Value; inclusive: boolean }, + right: { value: Value; inclusive: boolean } +): number { + const cmp = valueCompare(left.value, right.value); + if (cmp !== 0) { + return cmp; } + + if (left.inclusive && !right.inclusive) { + return -1; + } else if (!left.inclusive && right.inclusive) { + return 1; + } + + return 0; } -export function valuesMin( - left: Value | undefined, - right: Value | undefined -): Value | undefined { - if (left === undefined) { - return right; - } else if (right === undefined) { - return left; - } else { - return valueCompare(left, right) < 0 ? left : right; +export function upperBoundCompare( + left: { value: Value; inclusive: boolean }, + right: { value: Value; inclusive: boolean } +): number { + const cmp = valueCompare(left.value, right.value); + if (cmp !== 0) { + return cmp; } + + if (left.inclusive && !right.inclusive) { + return 1; + } else if (!left.inclusive && right.inclusive) { + return -1; + } + + return 0; } diff --git a/packages/firestore/test/unit/local/index_manager.test.ts b/packages/firestore/test/unit/local/index_manager.test.ts index 7970679a265..f2882775a4e 100644 --- a/packages/firestore/test/unit/local/index_manager.test.ts +++ b/packages/firestore/test/unit/local/index_manager.test.ts @@ -886,6 +886,35 @@ describe('IndexedDbIndexManager', async () => { await verifyResults(queryWithRestrictedBound, 'coll/val4'); }); + it('cannot expand result set from a cursor', async () => { + await indexManager.addFieldIndex( + fieldIndex('coll', { fields: [['c', IndexKind.ASCENDING]] }) + ); + await indexManager.addFieldIndex( + fieldIndex('coll', { fields: [['c', IndexKind.DESCENDING]] }) + ); + await addDoc('coll/val1', { 'a': 1, 'b': 1, 'c': 3 }); + await addDoc('coll/val2', { 'a': 2, 'b': 2, 'c': 2 }); + + let testingQuery = queryWithStartAt( + queryWithAddedOrderBy( + queryWithAddedFilter(query('coll'), filter('c', '>', 2)), + orderBy('c', 'asc') + ), + bound([2], /* inclusive= */ true) + ); + await verifyResults(testingQuery, 'coll/val1'); + + testingQuery = queryWithStartAt( + queryWithAddedOrderBy( + queryWithAddedFilter(query('coll'), filter('c', '<', 3)), + orderBy('c', 'desc') + ), + bound([3], /* inclusive= */ true) + ); + await verifyResults(testingQuery, 'coll/val2'); + }); + it('support advances queries', async () => { // This test compares local query results with those received from the Java // Server SDK. diff --git a/packages/firestore/test/unit/model/target.test.ts b/packages/firestore/test/unit/model/target.test.ts index 5932c3b20ef..624dc520660 100644 --- a/packages/firestore/test/unit/model/target.test.ts +++ b/packages/firestore/test/unit/model/target.test.ts @@ -29,7 +29,12 @@ import { targetGetArrayValues } from '../../../src/core/target'; import { IndexKind } from '../../../src/model/field_index'; -import { canonicalId, valueEquals } from '../../../src/model/values'; +import { + canonicalId, + MAX_VALUE, + MIN_VALUE, + valueEquals +} from '../../../src/model/values'; import { blob, bound, @@ -202,10 +207,12 @@ describe('Target Bounds', () => { const index = fieldIndex('c', { fields: [['foo', IndexKind.ASCENDING]] }); const lowerBound = targetGetLowerBound(target, index); - expect(lowerBound).to.be.null; + expect(lowerBound?.position[0]).to.equal(MIN_VALUE); + expect(lowerBound?.inclusive).to.be.true; const upperBound = targetGetUpperBound(target, index); - expect(upperBound).to.be.null; + expect(upperBound?.position[0]).to.equal(MAX_VALUE); + expect(upperBound?.inclusive).to.be.true; }); it('orderBy query with filter', () => { @@ -234,7 +241,8 @@ describe('Target Bounds', () => { verifyBound(lowerBound, true, 'bar'); const upperBound = targetGetUpperBound(target, index); - expect(upperBound).to.be.null; + expect(upperBound?.position[0]).to.equal(MAX_VALUE); + expect(upperBound?.inclusive).to.be.true; }); it('startAt query with filter', () => { @@ -329,7 +337,8 @@ describe('Target Bounds', () => { const index = fieldIndex('c', { fields: [['foo', IndexKind.ASCENDING]] }); const lowerBound = targetGetLowerBound(target, index); - expect(lowerBound).to.be.null; + expect(lowerBound?.position[0]).to.equal(MIN_VALUE); + expect(lowerBound?.inclusive).to.be.true; const upperBound = targetGetUpperBound(target, index); verifyBound(upperBound, true, 'bar');