From 60604a585e25f0bba349e2f66ae15e299abe624c Mon Sep 17 00:00:00 2001 From: Sugat Bajracharya Date: Thu, 8 Aug 2024 16:32:45 +0545 Subject: [PATCH] Change skip to cursor in all places and refactor fetchAndFilter function in local/person.ts --- shared-libs/cht-datasource/src/index.ts | 8 ++-- .../cht-datasource/src/libs/data-context.ts | 10 ++-- .../cht-datasource/src/local/person.ts | 46 ++++++++++--------- shared-libs/cht-datasource/src/person.ts | 18 ++++---- .../cht-datasource/src/remote/person.ts | 4 +- shared-libs/cht-datasource/test/index.spec.ts | 6 +-- .../test/libs/data-context.spec.ts | 8 ++-- .../cht-datasource/test/local/person.spec.ts | 16 +++---- .../cht-datasource/test/person.spec.ts | 29 ++++++------ .../cht-datasource/test/remote/person.spec.ts | 8 ++-- 10 files changed, 78 insertions(+), 75 deletions(-) diff --git a/shared-libs/cht-datasource/src/index.ts b/shared-libs/cht-datasource/src/index.ts index b893d2c742f..a88f3f34d64 100644 --- a/shared-libs/cht-datasource/src/index.ts +++ b/shared-libs/cht-datasource/src/index.ts @@ -89,16 +89,16 @@ export const getDatasource = (ctx: DataContext) => { /** * Returns an array of people for the provided page specifications. * @param personType the type of people to return + * @param cursor a value representing the index of the first person to return * @param limit the maximum number of people to return. Default is 100. - * @param skip the number of people to skip. Default is 0. * @returns an array of people for the provided page specifications * @throws Error if no type is provided or if the type is not for a person * @throws Error if the provided limit is `<= 0` - * @throws Error if the provided skip is `< 0` + * @throws Error if the provided cursor is `< 0` * @see {@link getByType} which provides the same data, but without having to manually account for paging */ - getPageByType: (personType: string, limit = 100, skip = 0) => ctx.bind(Person.v1.getPage)( - Qualifier.byContactType(personType), limit, skip + getPageByType: (personType: string, cursor = '0', limit = 100) => ctx.bind(Person.v1.getPage)( + Qualifier.byContactType(personType), cursor, limit ), /** diff --git a/shared-libs/cht-datasource/src/libs/data-context.ts b/shared-libs/cht-datasource/src/libs/data-context.ts index 72d523ed8da..d2b9c225db4 100644 --- a/shared-libs/cht-datasource/src/libs/data-context.ts +++ b/shared-libs/cht-datasource/src/libs/data-context.ts @@ -42,20 +42,20 @@ export const adapt = ( /** @internal */ export const getDocumentStream = async function* ( - fetchFunction: (args: S, l: number, s: number) => Promise>, + fetchFunction: (args: S, s: string, l: number) => Promise>, fetchFunctionArgs: S ): AsyncGenerator { const limit = 100; - let skip = 0; - const hasMoreResults = () => skip !== -1; + let cursor = '0'; + const hasMoreResults = () => cursor !== '-1'; do { - const docs = await fetchFunction(fetchFunctionArgs, limit, skip); + const docs = await fetchFunction(fetchFunctionArgs, cursor, limit); for (const doc of docs.data) { yield doc; } - skip = Number(docs.cursor); + cursor = docs.cursor; } while (hasMoreResults()); }; diff --git a/shared-libs/cht-datasource/src/local/person.ts b/shared-libs/cht-datasource/src/local/person.ts index 8452d512243..265a5bca446 100644 --- a/shared-libs/cht-datasource/src/local/person.ts +++ b/shared-libs/cht-datasource/src/local/person.ts @@ -63,8 +63,8 @@ export namespace v1 { export const getPage = ({ medicDb, settings }: LocalDataContext) => { return async ( personType: ContactTypeQualifier, + cursor: string, limit: number, - skip: number ): Promise> => { const personTypes = contactTypeUtils.getPersonTypes(settings.getAll()); const personTypesIds = personTypes.map((item) => item.id); @@ -73,43 +73,45 @@ export namespace v1 { throw new Error(`Invalid person type: ${personType.contactType}`); } + // Adding a number skip variable here so as not to confuse ourselves + const skip = Number(cursor) || 0; const getDocsByPage = queryDocsByKey(medicDb, 'medic-client/contacts_by_type'); const fetchAndFilter = async ( currentLimit: number, currentSkip: number, - personDocs: Person.v1.Person[], - totalDocsFetched = 0, + currentPersonDocs: Person.v1.Person[] = [], ): Promise> => { const docs = await getDocsByPage([personType.contactType], currentLimit, currentSkip); - if (docs.length === 0) { - return { data: personDocs, cursor: '-1' }; - } + const noMoreResults = docs.length < currentLimit; + const newPersonDocs = docs.filter((doc): doc is Person.v1.Person => isPerson(settings, doc, doc?._id)); + const overFetchCount = currentPersonDocs.length + newPersonDocs.length - limit || 0; + const totalPeople = [...currentPersonDocs, ...newPersonDocs].slice(0, limit); - const tempFilteredDocs = docs.filter((doc): doc is Person.v1.Person => isPerson(settings, doc, doc?._id)); + if (noMoreResults) { + return { data: totalPeople, cursor: '-1' }; + } - personDocs.push(...tempFilteredDocs); - totalDocsFetched += docs.length; + if (totalPeople.length === limit) { + const nextSkip = currentSkip + currentLimit - overFetchCount; - if (personDocs.length >= limit) { - let cursor: number; - if (docs.length < currentLimit) { - cursor = -1; - } else { - cursor = skip + totalDocsFetched - (personDocs.length - limit); - } - return { data: personDocs.slice(0, limit), cursor: cursor.toString() }; + return { data: totalPeople, cursor: nextSkip.toString() }; } + // Re-fetch twice as many docs as we need to limit number of recursions + const missingCount = currentLimit - newPersonDocs.length; + logger.debug(`Found [${missingCount.toString()}] invalid persons. Re-fetching additional records.`); + const nextLimit = missingCount * 2; + const nextSkip = currentSkip + currentLimit; + return fetchAndFilter( - (currentLimit - tempFilteredDocs.length) * 2, - currentSkip + currentLimit, - personDocs, - totalDocsFetched + nextLimit, + nextSkip, + totalPeople, ); }; - return fetchAndFilter(limit, skip, []); + return fetchAndFilter(limit, skip); }; }; } diff --git a/shared-libs/cht-datasource/src/person.ts b/shared-libs/cht-datasource/src/person.ts index bca674c5007..6963b7f5211 100644 --- a/shared-libs/cht-datasource/src/person.ts +++ b/shared-libs/cht-datasource/src/person.ts @@ -47,9 +47,9 @@ export namespace v1 { } }; - const assertSkip = (skip: unknown) => { - if (typeof skip !== 'number' || !Number.isInteger(skip) || skip < 0) { - throw new Error(`The skip must be a non-negative number: [${String(skip)}]`); + const assertCursor = (cursor: unknown) => { + if (typeof cursor !== 'string' || Number(cursor) < 0) { + throw new Error(`The cursor must be a stringified non-negative number: [${String(cursor)}]`); } }; @@ -97,8 +97,8 @@ export namespace v1 { /** * Returns an array of people for the provided page specifications. * @param personType the type of people to return + * @param cursor the number of people to skip. Default is 0. * @param limit the maximum number of people to return. Default is 100. - * @param skip the number of people to skip. Default is 0. * @returns an array of people for the provided page specifications. * @throws Error if no type is provided or if the type is not for a person * @throws Error if the provided `limit` value is `<=0` @@ -106,14 +106,14 @@ export namespace v1 { */ const curriedFn = async ( personType: ContactTypeQualifier, + cursor = '0', limit = 100, - skip = 0 ): Promise> => { assertTypeQualifier(personType); assertLimit(limit); - assertSkip(skip); + assertCursor(cursor); - return fn(personType, limit, skip); + return fn(personType, cursor, limit); }; return curriedFn; }; @@ -135,10 +135,10 @@ export namespace v1 { * @returns a generator for fetching all people with the given type * @throws Error if no type is provided or if the type is not for a person */ - const curriedGen = async function* (personType: ContactTypeQualifier): AsyncGenerator { + const curriedGen = (personType: ContactTypeQualifier): AsyncGenerator => { assertTypeQualifier(personType); const getPage = context.bind(v1.getPage); - yield* getDocumentStream(getPage, personType); + return getDocumentStream(getPage, personType); }; return curriedGen; }; diff --git a/shared-libs/cht-datasource/src/remote/person.ts b/shared-libs/cht-datasource/src/remote/person.ts index 0604c0c8b4a..120d357644b 100644 --- a/shared-libs/cht-datasource/src/remote/person.ts +++ b/shared-libs/cht-datasource/src/remote/person.ts @@ -25,9 +25,9 @@ export namespace v1 { /** @internal */ export const getPage = (remoteContext: RemoteDataContext) => ( personType: ContactTypeQualifier, + cursor: string, limit: number, - skip: number ): Promise> => getPeople(remoteContext)( - {'limit': limit.toString(), 'skip': skip.toString(), 'contactType': personType.contactType} + {'limit': limit.toString(), 'contactType': personType.contactType, cursor} ); } diff --git a/shared-libs/cht-datasource/test/index.spec.ts b/shared-libs/cht-datasource/test/index.spec.ts index 1ee95acaec5..0f7a728abcb 100644 --- a/shared-libs/cht-datasource/test/index.spec.ts +++ b/shared-libs/cht-datasource/test/index.spec.ts @@ -132,15 +132,15 @@ describe('CHT Script API - getDatasource', () => { dataContextBind.returns(personGetPage); const personType = 'person'; const limit = 2; - const skip = 1; + const cursor = '1'; const personTypeQualifier = { contactType: personType }; const byContactType = sinon.stub(Qualifier, 'byContactType').returns(personTypeQualifier); - const returnedPeople = await person.getPageByType(personType, limit, skip); + const returnedPeople = await person.getPageByType(personType, cursor, limit); expect(returnedPeople).to.equal(expectedPeople); expect(dataContextBind.calledOnceWithExactly(Person.v1.getPage)).to.be.true; - expect(personGetPage.calledOnceWithExactly(personTypeQualifier, limit, skip)).to.be.true; + expect(personGetPage.calledOnceWithExactly(personTypeQualifier, cursor, limit)).to.be.true; expect(byContactType.calledOnceWithExactly(personType)).to.be.true; }); diff --git a/shared-libs/cht-datasource/test/libs/data-context.spec.ts b/shared-libs/cht-datasource/test/libs/data-context.spec.ts index b2107106357..1164e7bca8a 100644 --- a/shared-libs/cht-datasource/test/libs/data-context.spec.ts +++ b/shared-libs/cht-datasource/test/libs/data-context.spec.ts @@ -122,7 +122,7 @@ describe('context lib', () => { describe('getDocumentStream', () => { let fetchFunctionStub: SinonStub; const limit = 100; - const skip = 0; + const cursor = '0'; beforeEach(() => { fetchFunctionStub = sinon.stub(); @@ -143,7 +143,7 @@ describe('context lib', () => { } expect(results).to.deep.equal(mockDocs); - expect(fetchFunctionStub.calledOnceWithExactly(extraArg, limit, skip)).to.be.true; + expect(fetchFunctionStub.calledOnceWithExactly(extraArg, cursor, limit)).to.be.true; }); it('should handle multiple pages', async () => { @@ -166,8 +166,8 @@ describe('context lib', () => { expect(results).to.deep.equal([...mockDocs1, ...mockDocs2]); expect(fetchFunctionStub.callCount).to.equal(2); - expect(fetchFunctionStub.firstCall.args).to.deep.equal([extraArg, limit, skip]); - expect(fetchFunctionStub.secondCall.args).to.deep.equal([extraArg, limit, skip + limit]); + expect(fetchFunctionStub.firstCall.args).to.deep.equal([extraArg, cursor, limit]); + expect(fetchFunctionStub.secondCall.args).to.deep.equal([extraArg, (Number(cursor) + limit).toString(), limit]); }); it('should handle empty result', async () => { diff --git a/shared-libs/cht-datasource/test/local/person.spec.ts b/shared-libs/cht-datasource/test/local/person.spec.ts index 58a7e6ad418..89f4065b1db 100644 --- a/shared-libs/cht-datasource/test/local/person.spec.ts +++ b/shared-libs/cht-datasource/test/local/person.spec.ts @@ -227,7 +227,7 @@ describe('local person', () => { describe('getPage', () => { const limit = 3; - const skip = 0; + const cursor = '0'; const personIdentifier = 'person'; const personTypeQualifier = {contactType: personIdentifier} as const; const invalidPersonTypeQualifier = { contactType: 'invalid' } as const; @@ -253,7 +253,7 @@ describe('local person', () => { data: docs }; - const res = await Person.v1.getPage(localContext)(personTypeQualifier, limit, skip); + const res = await Person.v1.getPage(localContext)(personTypeQualifier, cursor, limit); expect(res).to.deep.equal(expectedResult); expect(settingsGetAll.callCount).to.equal(4); @@ -261,7 +261,7 @@ describe('local person', () => { expect( queryDocsByKeyOuter.calledOnceWithExactly(localContext.medicDb, 'medic-client/contacts_by_type') ).to.be.true; - expect(queryDocsByKeyInner.calledOnceWithExactly([personIdentifier], limit, skip)).to.be.true; + expect(queryDocsByKeyInner.calledOnceWithExactly([personIdentifier], limit, Number(cursor))).to.be.true; expect(isPerson.callCount).to.equal(3); expect(isPerson.getCall(0).args).to.deep.equal([settings, doc]); expect(isPerson.getCall(1).args).to.deep.equal([settings, doc]); @@ -269,7 +269,7 @@ describe('local person', () => { }); it('throws an error if person identifier is invalid/does not exist', async () => { - await expect(Person.v1.getPage(localContext)(invalidPersonTypeQualifier, limit, skip)).to.be.rejectedWith( + await expect(Person.v1.getPage(localContext)(invalidPersonTypeQualifier, cursor, limit)).to.be.rejectedWith( `Invalid person type: ${invalidPersonTypeQualifier.contactType}` ); @@ -287,7 +287,7 @@ describe('local person', () => { cursor: '-1' }; - const res = await Person.v1.getPage(localContext)(personTypeQualifier, limit, skip); + const res = await Person.v1.getPage(localContext)(personTypeQualifier, cursor, limit); expect(res).to.deep.equal(expectedResult); expect(settingsGetAll.calledOnce).to.be.true; @@ -295,7 +295,7 @@ describe('local person', () => { expect( queryDocsByKeyOuter.calledOnceWithExactly(localContext.medicDb, 'medic-client/contacts_by_type') ).to.be.true; - expect(queryDocsByKeyInner.calledOnceWithExactly([personIdentifier], limit, skip)).to.be.true; + expect(queryDocsByKeyInner.calledOnceWithExactly([personIdentifier], limit, Number(cursor))).to.be.true; expect(isPerson.notCalled).to.be.true; }); @@ -311,7 +311,7 @@ describe('local person', () => { data: docs }; - const res = await Person.v1.getPage(localContext)(personTypeQualifier, limit, skip); + const res = await Person.v1.getPage(localContext)(personTypeQualifier, cursor, limit); expect(res).to.deep.equal(expectedResult); expect(settingsGetAll.callCount).to.equal(7); @@ -320,7 +320,7 @@ describe('local person', () => { queryDocsByKeyOuter.calledOnceWithExactly(localContext.medicDb, 'medic-client/contacts_by_type') ).to.be.true; expect(queryDocsByKeyInner.callCount).to.be.equal(2); - expect(queryDocsByKeyInner.firstCall.args).to.deep.equal([[personIdentifier], limit, skip]); + expect(queryDocsByKeyInner.firstCall.args).to.deep.equal([[personIdentifier], limit, Number(cursor)]); expect(queryDocsByKeyInner.secondCall.args).to.deep.equal([[personIdentifier], 4, 3]); expect(isPerson.callCount).to.equal(6); expect(isPerson.getCall(0).args).to.deep.equal([settings, doc]); diff --git a/shared-libs/cht-datasource/test/person.spec.ts b/shared-libs/cht-datasource/test/person.spec.ts index e6b5b5922a5..d9313840216 100644 --- a/shared-libs/cht-datasource/test/person.spec.ts +++ b/shared-libs/cht-datasource/test/person.spec.ts @@ -128,12 +128,11 @@ describe('person', () => { describe('getPage', () => { const people = [{ _id: 'person1' }, { _id: 'person2' }, { _id: 'person3' }] as Person.v1.Person[]; - const cursor = '-1'; + const cursor = '1'; const pageData = { data: people, cursor }; const limit = 3; - const skip = 1; const invalidLimit = -1; - const invalidSkip = -1; + const invalidCursor = '-1'; const personTypeQualifier = {contactType: 'person'} as const; const invalidQualifier = { contactType: 'invalid' } as const; let getPage: SinonStub; @@ -147,12 +146,12 @@ describe('person', () => { isContactTypeQualifier.returns(true); getPage.resolves(pageData); - const result = await Person.v1.getPage(dataContext)(personTypeQualifier, limit, skip); + const result = await Person.v1.getPage(dataContext)(personTypeQualifier, cursor, limit); expect(result).to.equal(pageData); expect(assertDataContext.calledOnceWithExactly(dataContext)).to.be.true; expect(adapt.calledOnceWithExactly(dataContext, Local.Person.v1.getPage, Remote.Person.v1.getPage)).to.be.true; - expect(getPage.calledOnceWithExactly(personTypeQualifier, limit, skip)).to.be.true; + expect(getPage.calledOnceWithExactly(personTypeQualifier, cursor, limit)).to.be.true; expect(isContactTypeQualifier.calledOnceWithExactly((personTypeQualifier))).to.be.true; }); @@ -171,7 +170,7 @@ describe('person', () => { it('throws an error if the qualifier is invalid', async () => { isContactTypeQualifier.returns(false); - await expect(Person.v1.getPage(dataContext)(invalidQualifier, limit, skip)) + await expect(Person.v1.getPage(dataContext)(invalidQualifier, cursor, limit)) .to.be.rejectedWith(`Invalid type [${JSON.stringify(invalidQualifier)}].`); expect(assertDataContext.calledOnceWithExactly(dataContext)).to.be.true; @@ -184,7 +183,7 @@ describe('person', () => { isContactTypeQualifier.returns(true); getPage.resolves(people); - await expect(Person.v1.getPage(dataContext)(personTypeQualifier, invalidLimit, skip )) + await expect(Person.v1.getPage(dataContext)(personTypeQualifier, cursor, invalidLimit)) .to.be.rejectedWith(`limit must be a positive number`); expect(assertDataContext.calledOnceWithExactly(dataContext)).to.be.true; @@ -193,12 +192,12 @@ describe('person', () => { expect(getPage.notCalled).to.be.true; }); - it('throws an error if skip is invalid', async () => { + it('throws an error if cursor is invalid', async () => { isContactTypeQualifier.returns(true); getPage.resolves(people); - await expect(Person.v1.getPage(dataContext)(personTypeQualifier, limit, invalidSkip)) - .to.be.rejectedWith(`skip must be a non-negative number`); + await expect(Person.v1.getPage(dataContext)(personTypeQualifier, invalidCursor, limit)) + .to.be.rejectedWith(`The cursor must be a stringified non-negative number: [${String(invalidCursor)}]`); expect(assertDataContext.calledOnceWithExactly(dataContext)).to.be.true; expect(adapt.calledOnceWithExactly(dataContext, Local.Person.v1.getPage, Remote.Person.v1.getPage)).to.be.true; @@ -219,6 +218,9 @@ describe('person', () => { yield person; } }; + const emptyMockGenerator = function* () { + // empty + }; let personGetPage: sinon.SinonStub; let getDocumentStream: sinon.SinonStub; @@ -248,7 +250,7 @@ describe('person', () => { it('should handle empty result set', async () => { isContactTypeQualifier.returns(true); - getDocumentStream.returns([]); + getDocumentStream.returns(emptyMockGenerator()); const generator = Person.v1.getAll(dataContext)(personTypeQualifier); const res = await generator.next(); @@ -273,9 +275,8 @@ describe('person', () => { it('should throw an error for invalid personType', async () => { isContactTypeQualifier.returns(false); - const generator = Person.v1.getAll(dataContext)(personTypeQualifier); - await expect(generator.next()).to.be.rejectedWith(`Invalid type [${JSON.stringify(personTypeQualifier)}].`); - + expect(() => Person.v1.getAll(dataContext)(personTypeQualifier)) + .to.throw(`Invalid type [${JSON.stringify(personTypeQualifier)}]`); expect(assertDataContext.calledOnceWithExactly(dataContext)).to.be.true; expect(personGetPage.notCalled).to.be.true; expect(isContactTypeQualifier.calledOnceWithExactly(personTypeQualifier)).to.be.true; diff --git a/shared-libs/cht-datasource/test/remote/person.spec.ts b/shared-libs/cht-datasource/test/remote/person.spec.ts index a2634acaf3e..d578f27402a 100644 --- a/shared-libs/cht-datasource/test/remote/person.spec.ts +++ b/shared-libs/cht-datasource/test/remote/person.spec.ts @@ -71,11 +71,11 @@ describe('remote person', () => { describe('getPage', () => { const limit = 3; - const skip = 1; + const cursor = '1'; const personTypeQualifier = {contactType: 'person'}; const queryParam = { limit: limit.toString(), - skip: skip.toString(), + cursor, ...personTypeQualifier }; @@ -84,7 +84,7 @@ describe('remote person', () => { const expectedResponse = { data: doc, cursor: '-1' }; getResourcesInner.resolves(expectedResponse); - const result = await Person.v1.getPage(remoteContext)(personTypeQualifier, limit, skip); + const result = await Person.v1.getPage(remoteContext)(personTypeQualifier, cursor, limit); expect(result).to.equal(expectedResponse); expect(getResourcesOuter.calledOnceWithExactly(remoteContext, 'api/v1/person')).to.be.true; @@ -94,7 +94,7 @@ describe('remote person', () => { it('returns empty array if docs are not found', async () => { getResourcesInner.resolves([]); - const result = await Person.v1.getPage(remoteContext)(personTypeQualifier, limit, skip); + const result = await Person.v1.getPage(remoteContext)(personTypeQualifier, cursor, limit); expect(result).to.deep.equal([]); expect(getResourcesOuter.calledOnceWithExactly(remoteContext, 'api/v1/person')).to.be.true;