From f4041ed68c6d24afe93325757be54906a3bf1ff7 Mon Sep 17 00:00:00 2001 From: pepe Date: Wed, 10 Nov 2021 19:33:37 -0500 Subject: [PATCH 1/2] fix: A bug fix Description: Store the fields that have been ordered by with (and throw if you try to orderBy twice in the same field) instead of just checking if any fields have been ordered by. An orderBy function cannot be called more than once in the same query expression, except on different fields. Fixing issue https://github.com/wovalle/fireorm/issues/266 --- src/BaseFirestoreRepository.spec.ts | 18 +++++++++++++++++- src/QueryBuilder.ts | 19 +++++++++++++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/BaseFirestoreRepository.spec.ts b/src/BaseFirestoreRepository.spec.ts index 97a40b7..0c9c51f 100644 --- a/src/BaseFirestoreRepository.spec.ts +++ b/src/BaseFirestoreRepository.spec.ts @@ -126,7 +126,16 @@ describe('BaseFirestoreRepository', () => { albumsSubColl.orderByAscending('releaseDate').orderByDescending('releaseDate'); }).toThrow(); }); + + it('must succeed when orderBy* function is called more than once in the same expression with different fields', async () => { + const pt = await bandRepository.findById('porcupine-tree'); + const albumsSubColl = pt.albums; + expect(() => { + albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); + }).toBeTruthy(); + }); }); + }); describe('orderByDescending', () => { it('must order repository objects', async () => { @@ -164,8 +173,15 @@ describe('BaseFirestoreRepository', () => { albumsSubColl.orderByAscending('releaseDate').orderByDescending('releaseDate'); }).toThrow(); }); + + it('must succeed when orderBy* function is called more than once in the same expression with different fields', async () => { + const pt = await bandRepository.findById('porcupine-tree'); + const albumsSubColl = pt.albums; + expect(() => { + albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); + }).toBeTruthy(); + }); }); - }); describe('findById', () => { it('must find by id', async () => { diff --git a/src/QueryBuilder.ts b/src/QueryBuilder.ts index 5ea60c8..73fea9a 100644 --- a/src/QueryBuilder.ts +++ b/src/QueryBuilder.ts @@ -17,6 +17,7 @@ export class QueryBuilder implements IQueryBuilder { protected limitVal: number; protected orderByObj: IOrderByParams; protected customQueryFunction?: ICustomQuery; + protected orderByFields: Set = new Set(); constructor(protected executor: IQueryExecutor) {} @@ -144,12 +145,19 @@ export class QueryBuilder implements IQueryBuilder { } orderByAscending(prop: IWherePropParam) { - if (this.orderByObj) { + const fieldProp: string = typeof prop == 'string' ? prop : ''; + const alreadyOrderedByField = this.orderByFields.has(fieldProp); + + if (this.orderByObj && alreadyOrderedByField) { throw new Error( 'An orderBy function cannot be called more than once in the same query expression' ); } + if (!alreadyOrderedByField && fieldProp) { + this.orderByFields.add(fieldProp); + } + this.orderByObj = { fieldPath: this.extractWhereParam(prop), directionStr: 'asc', @@ -159,12 +167,19 @@ export class QueryBuilder implements IQueryBuilder { } orderByDescending(prop: IWherePropParam) { - if (this.orderByObj) { + const fieldProp: string = typeof prop == 'string' ? prop : ''; + const alreadyOrderedByField = this.orderByFields.has(fieldProp); + + if (this.orderByObj && alreadyOrderedByField) { throw new Error( 'An orderBy function cannot be called more than once in the same query expression' ); } + if (!alreadyOrderedByField && fieldProp) { + this.orderByFields.add(fieldProp); + } + this.orderByObj = { fieldPath: this.extractWhereParam(prop), directionStr: 'desc', From f94aa04a94233ec6f87108558426c779ed1c39b4 Mon Sep 17 00:00:00 2001 From: pepe Date: Thu, 11 Nov 2021 12:33:46 -0500 Subject: [PATCH 2/2] Added more test cases for corner case scenarios --- src/BaseFirestoreRepository.spec.ts | 36 +++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/BaseFirestoreRepository.spec.ts b/src/BaseFirestoreRepository.spec.ts index 0c9c51f..405f7af 100644 --- a/src/BaseFirestoreRepository.spec.ts +++ b/src/BaseFirestoreRepository.spec.ts @@ -127,12 +127,20 @@ describe('BaseFirestoreRepository', () => { }).toThrow(); }); + it('must throw an Error if an orderBy* function is called more than once in the same expression ascending', async () => { + const pt = await bandRepository.findById('porcupine-tree'); + const albumsSubColl = pt.albums; + expect(() => { + albumsSubColl.orderByAscending('releaseDate').orderByAscending('releaseDate'); + }).toThrow(); + }); + it('must succeed when orderBy* function is called more than once in the same expression with different fields', async () => { const pt = await bandRepository.findById('porcupine-tree'); const albumsSubColl = pt.albums; expect(() => { albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); - }).toBeTruthy(); + }).not.toThrow(); }); }); }); @@ -174,12 +182,36 @@ describe('BaseFirestoreRepository', () => { }).toThrow(); }); + it('must throw an Error if an orderBy* function is called more than once in the same expression descending', async () => { + const pt = await bandRepository.findById('porcupine-tree'); + const albumsSubColl = pt.albums; + expect(() => { + albumsSubColl.orderByDescending('releaseDate').orderByDescending('releaseDate'); + }).toThrow(); + }); + it('must succeed when orderBy* function is called more than once in the same expression with different fields', async () => { const pt = await bandRepository.findById('porcupine-tree'); const albumsSubColl = pt.albums; expect(() => { albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); - }).toBeTruthy(); + }).not.toThrow(); + }); + + it('must succeed when orderBy* function is called more than once in the same expression with different fields ascending', async () => { + const pt = await bandRepository.findById('porcupine-tree'); + const albumsSubColl = pt.albums; + expect(() => { + albumsSubColl.orderByAscending('releaseDate').orderByAscending('name'); + }).not.toThrow(); + }); + + it('must succeed when orderBy* function is called more than once in the same expression with different fields descending', async () => { + const pt = await bandRepository.findById('porcupine-tree'); + const albumsSubColl = pt.albums; + expect(() => { + albumsSubColl.orderByDescending('releaseDate').orderByDescending('name'); + }).not.toThrow(); }); });