From 71691b14c208293758d15dc019be1362ec963e6a Mon Sep 17 00:00:00 2001 From: Peter Potucek Date: Mon, 10 Oct 2022 16:27:39 +0100 Subject: [PATCH] Handle duplication of results when filtering using nested fields --- src/__tests__/cache.test.js | 40 ++++++++++- src/__tests__/datasource.test.js | 114 ++++++++++++++++++++++++++++++- src/cache.js | 9 ++- 3 files changed, 156 insertions(+), 7 deletions(-) diff --git a/src/__tests__/cache.test.js b/src/__tests__/cache.test.js index 7afa079..c01f18b 100644 --- a/src/__tests__/cache.test.js +++ b/src/__tests__/cache.test.js @@ -20,11 +20,19 @@ const docs = { one: { _id: objectID, foo: 'bar', - tags: ['foo', 'bar'] + tags: ['foo', 'bar'], + nested: { + field1: 'one', + field2: 'two' + } }, two: { _id: ObjectId(), - foo: 'bar' + foo: 'bar', + nested: { + field1: 'two', + field2: 'one' + } }, three: { nested: { @@ -214,6 +222,30 @@ describe('createCachingMethods', () => { expect(collection.find.mock.calls.length).toBe(1) }) + it(`doesn't mix results of pending calls using nested field filters with differing values`, async () => { + const pendingDocs1 = api.findByFields({ + 'nested.field1': 'one', + 'nested.field2': 'two' + }) + const pendingDocs2 = api.findByFields({ + 'nested.field1': 'two', + 'nested.field2': 'one' + }) + + const [foundDocs1, foundDocs2] = await Promise.all([ + pendingDocs1, + pendingDocs2 + ]) + + expect(foundDocs1[0]).toBe(docs.one) + expect(foundDocs1.length).toBe(1) + expect(foundDocs2[0]).toBe(docs.two) + expect(foundDocs2.length).toBe(1) + + expect(collection.find.mock.calls.length).toBe(1) + console.log(collection.find.mock.calls[0]) + }) + it(`dataloader caches each value individually when finding by a single field`, async () => { await api.findByFields({ tags: ['foo', 'baz'] }, { ttl: 1 }) @@ -318,12 +350,14 @@ describe('isValidObjectIdString', () => { describe('getNestedValue', () => { it('works', () => { const obj = { - nested: { foo: 'bar', fooB: '', fooC: null, fooE: { inner: true } } + nested: { foo: 'bar', fooB: '', fooC: null, fooE: { inner: true } }, + ['top.level']: 'baz' } expect(getNestedValue(obj, 'nested.foo')).toEqual('bar') expect(getNestedValue(obj, 'nested.fooB')).toEqual('') expect(getNestedValue(obj, 'nested.fooC')).toEqual(null) expect(getNestedValue(obj, 'nested.fooD')).toBeUndefined() expect(getNestedValue(obj, 'nested.fooE.inner')).toBe(true) + expect(getNestedValue(obj, 'top.level')).toBe('baz') }) }) diff --git a/src/__tests__/datasource.test.js b/src/__tests__/datasource.test.js index 14b1c76..3e7715b 100644 --- a/src/__tests__/datasource.test.js +++ b/src/__tests__/datasource.test.js @@ -49,6 +49,8 @@ describe('Mongoose', () => { let userCollection let alice let nestedBob + let nestedCharlie + let nestedDan beforeAll(async () => { const userSchema = new Schema({ name: 'string' }) @@ -64,7 +66,28 @@ describe('Mongoose', () => { nestedBob = await userCollection.findOneAndReplace( { name: 'Bob' }, - { name: 'Bob', nested: { _id: objectID, field1: 'value1', field2: '' } }, + { + name: 'Bob', + nested: { _id: objectID, field1: 'value1', field2: 'value1' } + }, + { new: true, upsert: true } + ) + + nestedCharlie = await userCollection.findOneAndReplace( + { name: 'Charlie' }, + { + name: 'Charlie', + nested: { field1: 'value2', field2: 'value2' } + }, + { new: true, upsert: true } + ) + + nestedDan = await userCollection.findOneAndReplace( + { name: 'Dan' }, + { + name: 'Dan', + nested: { field1: 'value1', field2: 'value2' } + }, { new: true, upsert: true } ) }) @@ -130,9 +153,94 @@ describe('Mongoose', () => { expect(user.name).toBe('Bob') const res1 = await users.findByFields({ 'nested.field1': 'value1' }) - const res2 = await users.findByFields({ 'nested.field2': 'value1' }) - + const res2 = await users.findByFields({ 'nested.field2': '' }) expect(res1[0].name).toBe('Bob') expect(res2[0]).toBeUndefined() }) + + test('nested findByFields with single filter and batching', async () => { + const users = new Users(userCollection) + users.initialize() + + let pendingDocs = [] + for (var i = 1; i <= 3; i++) { + pendingDocs.push(users.findByFields({ 'nested.field1': `value${i}` })) + } + + /* + Intent here, with Promise.All, is to force batching to happen in the underlying dataloader library. + + This results in the following optimized filter to be passed to MongoDb: + + filter: { + 'nested.field1': { '$in': [ 'value1', 'value2', 'value3' ] } + } + + This in turn correctly matches Bob, Charlie and Dan records. + + Bob and Dan match filters passed to the first invocation of findByFields function: { 'nested.field1': [ 'value1' ] } + + Charlie matches filters passed to the second invocation of findByFields function: { 'nested.field1': [ 'value2' ] } + */ + + const docs = await Promise.all(pendingDocs) + + expect(docs[0][0].name).toBe('Bob') + expect(docs[0][1].name).toBe('Dan') + expect(docs[0].length).toBe(2) + + expect(docs[1][0].name).toBe('Charlie') + expect(docs[1].length).toBe(1) + + expect(docs[2][0]).toBeUndefined() + expect(docs[2].length).toBe(0) + + expect(docs.length).toBe(3) + }) + + test('nested findByFields with multiple filters and batching', async () => { + const users = new Users(userCollection) + users.initialize() + + let pendingDocs = [] + for (var i = 1; i <= 3; i++) { + pendingDocs.push( + users.findByFields({ + 'nested.field1': `value${i}`, + 'nested.field2': `value${i}` + }) + ) + } + + /* + Intent here, with Promise.All, is to force batching to happen in the underlying dataloader library. + This results in the following optimized filter to be passed to MongoDb: + + filter: { + 'nested.field1': { '$in': [ 'value1', 'value2', 'value3' ] }, + 'nested.field2': { '$in': [ 'value1', 'value2', 'value3' ] } + } + + This in turn correctly matches Bob, Charlie and Dan records. + + However, only Bob and Charlie match original filters passed to findByFields function, so only those should be returned. + + { 'nested.field1': [ 'value1' ], 'nested.field2': [ 'value1' ] }, + { 'nested.field1': [ 'value2' ], 'nested.field2': [ 'value2' ] } + + */ + + const docs = await Promise.all(pendingDocs) + + expect(docs[0][0].name).toBe('Bob') + expect(docs[0].length).toBe(1) + + expect(docs[1][0].name).toBe('Charlie') + expect(docs[1].length).toBe(1) + + expect(docs[2][0]).toBeUndefined() + expect(docs[2].length).toBe(0) + + expect(docs.length).toBe(3) + }) }) diff --git a/src/cache.js b/src/cache.js index d36fdc6..0a7dcdf 100644 --- a/src/cache.js +++ b/src/cache.js @@ -46,7 +46,13 @@ export function prepFields(fields) { // getNestedValue({ nested: { foo: 'bar' } }, 'nested.foo') // => 'bar' +// getNestedValue({ 'top.level': value }, 'top.level') +// => 'value' export function getNestedValue(object, string) { + if (string in object) { + return object[string] + } + string = string.replace(/\[(\w+)\]/g, '.$1') // convert indexes to properties string = string.replace(/^\./, '') // strip a leading dot var a = string.split('.') @@ -76,7 +82,8 @@ const orderDocs = (fieldsArray, docs) => ? fieldValue.map(val => idToString(val)) : [idToString(fieldValue)] - const docValue = doc[fieldName] + const docValue = getNestedValue(doc, fieldName) + const docValuesArr = Array.isArray(docValue) ? docValue.map(val => idToString(val)) : [idToString(docValue)]