Skip to content

Commit

Permalink
fix(firestore): correct handling of nested FieldPath in orderBy c…
Browse files Browse the repository at this point in the history
…lauses (#6814)
  • Loading branch information
Lyokone authored Jan 10, 2023
1 parent 54f6012 commit 84d7bbe
Show file tree
Hide file tree
Showing 8 changed files with 92 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,21 @@ private void applyOrders(ReadableArray orders) {
List<Object> ordersList = toArrayList(orders);

for (Object o : ordersList) {
Map<String, String> order = (Map) o;
Map<String, Object> order = (Map) o;

String fieldPath = order.get("fieldPath");
String direction = order.get("direction");
if (order.get("fieldPath") instanceof List) {
ArrayList fieldPathArray = (ArrayList) order.get("fieldPath");
String[] segmentArray = (String[]) fieldPathArray.toArray(new String[0]);
FieldPath fieldPath = FieldPath.of(segmentArray);
String direction = (String) order.get("direction");

query = query.orderBy(Objects.requireNonNull(fieldPath), Query.Direction.valueOf(direction));
query = query.orderBy(Objects.requireNonNull(fieldPath), Query.Direction.valueOf(direction));
} else {
String fieldPath = (String) order.get("fieldPath");
String direction = (String) order.get("direction");

query = query.orderBy(Objects.requireNonNull(fieldPath), Query.Direction.valueOf(direction));
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/e2e/DocumentSnapshot/get.e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('firestore().doc() -> snapshot.get()', function () {
snapshot.get(123);
return Promise.reject(new Error('Did not throw an Error.'));
} catch (error) {
error.message.should.containEql("'fieldPath' expected type string or FieldPath");
error.message.should.containEql("'fieldPath' expected type string, array or FieldPath");
return Promise.resolve();
}
});
Expand Down
36 changes: 36 additions & 0 deletions packages/firestore/e2e/Query/where.e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,42 @@ describe('firestore().collection().where()', function () {
});
});

it('returns when combining greater than and lesser than on the same nested field', async function () {
const colRef = firebase.firestore().collection(`${COLLECTION}/filter/greaterandless`);

await Promise.all([
colRef.doc('doc1').set({ foo: { bar: 1 } }),
colRef.doc('doc2').set({ foo: { bar: 2 } }),
colRef.doc('doc3').set({ foo: { bar: 3 } }),
]);

const snapshot = await colRef
.where('foo.bar', '>', 1)
.where('foo.bar', '<', 3)
.orderBy('foo.bar')
.get();

snapshot.size.should.eql(1);
});

it('returns when combining greater than and lesser than on the same nested field using FieldPath', async function () {
const colRef = firebase.firestore().collection(`${COLLECTION}/filter/greaterandless`);

await Promise.all([
colRef.doc('doc1').set({ foo: { bar: 1 } }),
colRef.doc('doc2').set({ foo: { bar: 2 } }),
colRef.doc('doc3').set({ foo: { bar: 3 } }),
]);

const snapshot = await colRef
.where(new firebase.firestore.FieldPath('foo', 'bar'), '>', 1)
.where(new firebase.firestore.FieldPath('foo', 'bar'), '<', 3)
.orderBy(new firebase.firestore.FieldPath('foo', 'bar'))
.get();

snapshot.size.should.eql(1);
});

it('returns with where array-contains filter', async function () {
const colRef = firebase.firestore().collection(`${COLLECTION}/filter/array-contains`);

Expand Down
19 changes: 15 additions & 4 deletions packages/firestore/ios/RNFBFirestore/RNFBFirestoreQuery.m
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,22 @@ - (void)applyFilters {

- (void)applyOrders {
for (NSDictionary *order in _orders) {
NSString *fieldPath = order[@"fieldPath"];
NSString *direction = order[@"direction"];
bool isDescending = [direction isEqualToString:@"DESCENDING"];
if ([order[@"fieldPath"] isKindOfClass:[NSArray class]]) {
NSArray *fieldPathArray = order[@"fieldPath"];
NSString *direction = order[@"direction"];
bool isDescending = [direction isEqualToString:@"DESCENDING"];

_query = [_query queryOrderedByField:fieldPath descending:isDescending];
FIRFieldPath *fieldPath = [[FIRFieldPath alloc] initWithFields:fieldPathArray];

_query = [_query queryOrderedByFieldPath:fieldPath descending:isDescending];

} else {
NSString *fieldPath = order[@"fieldPath"];
NSString *direction = order[@"direction"];
bool isDescending = [direction isEqualToString:@"DESCENDING"];

_query = [_query queryOrderedByField:fieldPath descending:isDescending];
}
}
}

Expand Down
12 changes: 9 additions & 3 deletions packages/firestore/lib/FirestoreDocumentSnapshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*
*/

import { isString } from '@react-native-firebase/app/lib/common';
import { isArray, isString } from '@react-native-firebase/app/lib/common';
import FirestoreDocumentReference, {
provideDocumentSnapshotClass,
} from './FirestoreDocumentReference';
Expand Down Expand Up @@ -78,9 +78,13 @@ export default class FirestoreDocumentSnapshot {
get(fieldPath) {
// TODO: ehesp: How are SnapshotOptions handled?

if (!isString(fieldPath) && !(fieldPath instanceof FirestoreFieldPath)) {
if (
!isString(fieldPath) &&
!(fieldPath instanceof FirestoreFieldPath) &&
!Array.isArray(fieldPath)
) {
throw new Error(
"firebase.firestore() DocumentSnapshot.get(*) 'fieldPath' expected type string or FieldPath.",
"firebase.firestore() DocumentSnapshot.get(*) 'fieldPath' expected type string, array or FieldPath.",
);
}

Expand All @@ -92,6 +96,8 @@ export default class FirestoreDocumentSnapshot {
} catch (e) {
throw new Error(`firebase.firestore() DocumentSnapshot.get(*) 'fieldPath' ${e.message}.`);
}
} else if (isArray(fieldPath)) {
path = new FirestoreFieldPath(...fieldPath);
} else {
// Is already field path
path = fieldPath;
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/lib/FirestoreQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ import {
isUndefined,
} from '@react-native-firebase/app/lib/common';
import NativeError from '@react-native-firebase/app/lib/internal/NativeFirebaseError';
import { FirestoreAggregateQuery } from './FirestoreAggregate';
import FirestoreDocumentSnapshot from './FirestoreDocumentSnapshot';
import FirestoreFieldPath, { fromDotSeparatedString } from './FirestoreFieldPath';
import FirestoreQuerySnapshot from './FirestoreQuerySnapshot';
import { FirestoreAggregateQuery } from './FirestoreAggregate';
import { parseSnapshotArgs } from './utils';

let _id = 0;
Expand Down
20 changes: 13 additions & 7 deletions packages/firestore/lib/FirestoreQueryModifiers.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
*/

import { isNumber } from '@react-native-firebase/app/lib/common';
import FirestoreFieldPath, { DOCUMENT_ID } from './FirestoreFieldPath';
import { buildNativeArray, generateNativeData } from './utils/serialize';
import { DOCUMENT_ID } from './FirestoreFieldPath';

const OPERATORS = {
'==': 'EQUAL',
Expand Down Expand Up @@ -74,11 +74,17 @@ export default class FirestoreQueryModifiers {
}

get filters() {
return this._filters.map(f => ({ ...f, fieldPath: f.fieldPath._toArray() }));
return this._filters.map(f => ({
...f,
fieldPath: f.fieldPath instanceof FirestoreFieldPath ? f.fieldPath._toArray() : f.fieldPath,
}));
}

get orders() {
return this._orders;
return this._orders.map(f => ({
...f,
fieldPath: f.fieldPath instanceof FirestoreFieldPath ? f.fieldPath._toArray() : f.fieldPath,
}));
}

get options() {
Expand Down Expand Up @@ -337,7 +343,7 @@ export default class FirestoreQueryModifiers {

orderBy(fieldPath, directionStr) {
const order = {
fieldPath: fieldPath._toPath(),
fieldPath: fieldPath,
direction: directionStr ? DIRECTIONS[directionStr.toLowerCase()] : DIRECTIONS.asc,
};

Expand All @@ -348,7 +354,7 @@ export default class FirestoreQueryModifiers {
validateOrderBy() {
// Ensure order hasn't been called on the same field
if (this._orders.length > 1) {
const orders = this._orders.map($ => $.fieldPath);
const orders = this._orders.map($ => $.fieldPath._toPath());
const set = new Set(orders);

if (set.size !== orders.length) {
Expand All @@ -371,7 +377,7 @@ export default class FirestoreQueryModifiers {
const orderFieldPath = order.fieldPath;
if (filter.operator === OPERATORS['==']) {
// Any where() fieldPath parameter cannot match any orderBy() parameter when '==' operand is invoked
if (filterFieldPath === orderFieldPath) {
if (filterFieldPath === orderFieldPath._toPath()) {
throw new Error(
`Invalid query. Query.orderBy() parameter: ${orderFieldPath} cannot be the same as your Query.where() fieldPath parameter: ${filterFieldPath}`,
);
Expand All @@ -386,7 +392,7 @@ export default class FirestoreQueryModifiers {

if (INEQUALITY[filter.operator]) {
// Initial orderBy() parameter has to match every where() fieldPath parameter when inequality operator is invoked
if (filterFieldPath !== this._orders[0].fieldPath) {
if (filterFieldPath !== this._orders[0].fieldPath._toPath()) {
throw new Error(
`Invalid query. Initial Query.orderBy() parameter: ${orderFieldPath} has to be the same as the Query.where() fieldPath parameter(s): ${filterFieldPath} when an inequality operator is invoked `,
);
Expand Down
8 changes: 4 additions & 4 deletions packages/firestore/lib/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,18 @@ import {
} from '@react-native-firebase/app/lib/common';
import FirestoreFieldPath, { fromDotSeparatedString } from '../FirestoreFieldPath';

export function extractFieldPathData(data, segmenets) {
export function extractFieldPathData(data, segments) {
if (!isObject(data)) {
return undefined;
}

const pathValue = data[segmenets[0]];
const pathValue = data[segments[0]];

if (segmenets.length === 1) {
if (segments.length === 1) {
return pathValue;
}

return extractFieldPathData(pathValue, segmenets.slice(1));
return extractFieldPathData(pathValue, segments.slice(1));
}

export function parseUpdateArgs(args) {
Expand Down

1 comment on commit 84d7bbe

@vercel
Copy link

@vercel vercel bot commented on 84d7bbe Jan 10, 2023

Choose a reason for hiding this comment

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

Please sign in to comment.