Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Make PouchLink querying system compatible with new PouchDB version #1506

Merged
merged 10 commits into from
Sep 9, 2024
150 changes: 84 additions & 66 deletions docs/api/cozy-pouch-link/classes/PouchLink.md

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions docs/api/cozy-stack-client.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,15 @@ Serves to dedupe equal queries requested at the same time</p>
</dd>
<dt><a href="#makeKeyFromPartialFilter">makeKeyFromPartialFilter</a> ⇒ <code>string</code></dt>
<dd><p>Process a partial filter to generate a string key</p>
<p>/!\ Warning: this method is similar to cozy-pouch-link mango.makeKeyFromPartialFilter()
If you edit this method, please check if the change is also needed in mango file</p>
</dd>
<dt><a href="#getIndexNameFromFields">getIndexNameFromFields</a> ⇒ <code>string</code></dt>
<dd><p>Name an index, based on its indexed fields and partial filter.</p>
<p>It follows this naming convention:
<code>by_{indexed_field1}_and_{indexed_field2}_filter_({partial_filter.key1}_{partial_filter.value1})_and_({partial_filter.key2}_{partial_filter.value2})</code></p>
<p>/!\ Warning: this method is similar to cozy-pouch-link mango.getIndexNameFromFields()
If you edit this method, please check if the change is also needed in mango file</p>
</dd>
<dt><a href="#transformSort">transformSort</a> ⇒ <code><a href="#MangoSort">MangoSort</a></code></dt>
<dd><p>Transform sort into Array</p>
Expand Down Expand Up @@ -2175,6 +2179,9 @@ Get the list of illegal characters in the file name
## makeKeyFromPartialFilter ⇒ <code>string</code>
Process a partial filter to generate a string key

/!\ Warning: this method is similar to cozy-pouch-link mango.makeKeyFromPartialFilter()
If you edit this method, please check if the change is also needed in mango file

**Kind**: global constant
**Returns**: <code>string</code> - - The string key of the processed partial filter

Expand All @@ -2190,6 +2197,9 @@ Name an index, based on its indexed fields and partial filter.
It follows this naming convention:
`by_{indexed_field1}_and_{indexed_field2}_filter_({partial_filter.key1}_{partial_filter.value1})_and_({partial_filter.key2}_{partial_filter.value2})`

/!\ Warning: this method is similar to cozy-pouch-link mango.getIndexNameFromFields()
If you edit this method, please check if the change is also needed in mango file

**Kind**: global constant
**Returns**: <code>string</code> - The index name, built from the fields

Expand Down
141 changes: 100 additions & 41 deletions packages/cozy-pouch-link/src/CozyPouchLink.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ export const isExpiredTokenError = pouchError => {
return expiredTokenError.test(pouchError.error)
}

const normalizeAll = (docs, doctype) => {
return docs.map(doc => jsonapi.normalizeDoc(doc, doctype))
const normalizeAll = client => (docs, doctype) => {
return docs.map(doc => jsonapi.normalizeDoc(doc, doctype, client))
}

/**
Expand Down Expand Up @@ -83,7 +83,7 @@ class PouchLink extends CozyLink {
constructor(opts) {
const options = defaults({}, opts, DEFAULT_OPTIONS)
super(options)
const { doctypes, doctypesReplicationOptions } = options
const { doctypes, doctypesReplicationOptions, ignoreWarmup } = options
this.options = options
if (!doctypes) {
throw new Error(
Expand All @@ -96,6 +96,7 @@ class PouchLink extends CozyLink {
this.storage = new PouchLocalStorage(
options.platform?.storage || platformWeb.storage
)
this.ignoreWarmup = ignoreWarmup
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't specify any warmup query, this is useless, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #1524


/** @type {Record<string, ReplicationStatus>} - Stores replication states per doctype */
this.replicationStatus = this.replicationStatus || {}
Expand Down Expand Up @@ -239,7 +240,7 @@ class PouchLink extends CozyLink {
* Emits an event (pouchlink:sync:end) when the sync (all doctypes) is done
*/
handleOnSync(doctypeUpdates) {
const normalizedData = mapValues(doctypeUpdates, normalizeAll)
const normalizedData = mapValues(doctypeUpdates, normalizeAll(this.client))
if (this.client) {
this.client.setData(normalizedData)
}
Expand Down Expand Up @@ -361,7 +362,7 @@ class PouchLink extends CozyLink {
return forward(operation)
}

if (await this.needsToWaitWarmup(doctype)) {
if (!this.ignoreWarmup && (await this.needsToWaitWarmup(doctype))) {
if (process.env.NODE_ENV !== 'production') {
logger.info(
`Tried to access local ${doctype} but not warmuped yet. Forwarding the operation to next link`
Expand Down Expand Up @@ -450,35 +451,77 @@ class PouchLink extends CozyLink {
return Boolean(this.indexes[name])
}

// This merge is necessary because PouchDB does not support partial indexes
mergePartialIndexInSelector(selector, partialFilter) {
if (partialFilter) {
logger.info(
`PouchLink: The query contains a partial index but PouchDB does not support it. ` +
`Hence, the partial index definition is used in the selector for in-memory evaluation, ` +
`which might impact expected performances. If this support is important in your use-case, ` +
`please let us know or help us contribute to PouchDB!`
)
return { ...selector, ...partialFilter }
}
return selector
/**
* Create the PouchDB index if not existing
*
* @param {Array} fields - Fields to index
* @param {object} indexOption - Options for the index
* @param {object} [indexOption.partialFilter] - partialFilter
* @param {string} [indexOption.indexName] - indexName
* @param {string} [indexOption.doctype] - doctype
* @returns {Promise<import('./types').PouchDbIndex>}
*/
async createIndex(fields, { partialFilter, indexName, doctype } = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the doctype should be an option, as the function cannot work without it

const absName = `${doctype}/${indexName}`
const db = this.pouches.getPouch(doctype)

const index = await db.createIndex({
index: {
fields,
ddoc: indexName,
indexName,
partial_filter_selector: partialFilter
}
})
this.indexes[absName] = index
return index
}

async ensureIndex(doctype, query) {
const fields = query.indexedFields || getIndexFields(query)
const name = getIndexNameFromFields(fields)
const absName = `${doctype}/${name}`
const db = this.pouches.getPouch(doctype)
if (this.indexes[absName]) {
return this.indexes[absName]
} else {
const index = await db.createIndex({
index: {
fields
}
/**
* Retrieve the PouchDB index if exist, undefined otherwise
*
* @param {string} doctype - The query's doctype
* @param {import('./types').MangoQueryOptions} options - The find options
* @param {string} indexName - The index name
* @returns {import('./types').PouchDbIndex | undefined}
*/
findExistingIndex(doctype, options, indexName) {
const absName = `${doctype}/${indexName}`
return this.indexes[absName]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this method name is copied on the one in DocumentCollection, but I feel like it is a bit deceptive here regarding its name: it checks if we have the index in memory, but if it is not found, it does not mean the index does not exist.
And if it's not found, we create the index: about that did we check that pouchdb correctly handle a creation of an existing index (i.e. it won't recreate the index) ? I think we did, but I'm not entirely sure 😄
In any case, I think we should:

  • either rename this function in something like hasIndexInMemory, either document a bit more the behaviour
  • add some comment in the index creation to specify that the creation of an exiting index will not recreate it


/**
* Handle index creation if it is missing.
*
* When an index is missing, we first check if there is one with a different
* name but the same definition. If there is none, we create the new index.
*
* /!\ Warning: this method is similar to DocumentCollection.handleMissingIndex()
* If you edit this method, please check if the change is also needed in DocumentCollection
*
* @param {string} doctype The mango selector
* @param {import('./types').MangoQueryOptions} options The find options
* @returns {Promise<import('./types').PouchDbIndex>} index
* @private
*/
async ensureIndex(doctype, options) {
let { indexedFields, partialFilter } = options

if (!indexedFields) {
indexedFields = getIndexFields(options)
}

const indexName = getIndexNameFromFields(indexedFields, partialFilter)

const existingIndex = this.findExistingIndex(doctype, options, indexName)
if (!existingIndex) {
return await this.createIndex(indexedFields, {
partialFilter,
indexName,
doctype
})
this.indexes[absName] = index
return index
} else {
return existingIndex
}
}

Expand All @@ -495,11 +538,6 @@ class PouchLink extends CozyLink {
partialFilter
}) {
const db = this.getPouch(doctype)
// The partial index is not supported by PouchDB, so we ensure the selector includes it
const mergedSelector = this.mergePartialIndexInSelector(
selector,
partialFilter
)
let res, withRows
if (id) {
res = await db.get(id)
Expand All @@ -509,14 +547,33 @@ class PouchLink extends CozyLink {
res = withoutDesignDocuments(res)
res.total_rows = null // pouch indicates the total number of docs in res.total_rows, even though we use "keys". Setting it to null avoids cozy-client thinking there are more docs to fetch.
withRows = true
} else if (!mergedSelector && !fields && !sort) {
} else if (!selector && !partialFilter && !fields && !sort) {
res = await allDocs(db, { include_docs: true, limit })
res = withoutDesignDocuments(res)
withRows = true
} else {
let findSelector = selector
const shouldAddId = !findSelector
if (shouldAddId) {
findSelector = {}
}
if (indexedFields) {
for (const indexedField of indexedFields) {
if (!Object.keys(findSelector).includes(indexedField)) {
findSelector[indexedField] = {
$gt: null
}
}
}
}
if (shouldAddId) {
findSelector['_id'] = {
$gt: null
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to force the _id selector if the mechanism on line 566-567 added a predicate.
Thus, we might refacto like this:

let findSelector = selector || {}
// ...do the stuff on lines 566...
findSelector = Object.keys(selector).length > 0 ? findSelector : { _id: { $gt: null } } // PouchDB does not accept empty selector 

And this whole behavior should definitely be tested, as suggested in my previous comment 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #1524

const findOpts = {
sort,
selector: mergedSelector,
selector: findSelector,
// same selector as Document Collection. We force _id.
// Fix https://github.com/cozy/cozy-client/issues/985
fields: fields ? [...fields, '_id', '_type', 'class'] : undefined,
Expand All @@ -525,15 +582,16 @@ class PouchLink extends CozyLink {
}
const index = await this.ensureIndex(doctype, {
...findOpts,
indexedFields
indexedFields,
partialFilter
})
findOpts.use_index = index.id
res = await find(db, findOpts)
res.offset = skip
res.limit = limit
withRows = true
}
return jsonapi.fromPouchResult(res, withRows, doctype)
return jsonapi.fromPouchResult(res, withRows, doctype, this.client)
}

async executeMutation(mutation, result, forward) {
Expand Down Expand Up @@ -561,7 +619,8 @@ class PouchLink extends CozyLink {
return jsonapi.fromPouchResult(
pouchRes,
false,
getDoctypeFromOperation(mutation)
getDoctypeFromOperation(mutation),
this.client
)
}

Expand Down
55 changes: 28 additions & 27 deletions packages/cozy-pouch-link/src/CozyPouchLink.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,28 +296,6 @@ describe('CozyPouchLink', () => {
})
})

it('should merge selector and partial filter definitions', () => {
const selector = { _id: { $gt: null } }
expect(link.mergePartialIndexInSelector(selector, {})).toEqual(selector)

const partialFilter = {
trashed: {
$exists: false
}
}
const expectedMergedSelector = {
_id: {
$gt: null
},
trashed: {
$exists: false
}
}
expect(link.mergePartialIndexInSelector(selector, partialFilter)).toEqual(
expectedMergedSelector
)
})

it("should add _id in the selected fields since CozyClient' store needs it", async () => {
find.mockReturnValue({ docs: [TODO_3, TODO_4] })
await setup()
Expand Down Expand Up @@ -476,7 +454,10 @@ describe('CozyPouchLink', () => {
cozyFromPouch: true,
done: false,
id: '1',
label: 'Buy bread'
label: 'Buy bread',
relationships: {
referenced_by: undefined
}
}
]
})
Expand Down Expand Up @@ -589,16 +570,31 @@ describe('CozyPouchLink', () => {
.where({})
.sortBy([{ name: 'asc' }])
await link.request(query)
expect(spy).toHaveBeenCalledWith({ index: { fields: ['name'] } })
expect(spy).toHaveBeenCalledWith({
index: {
ddoc: 'by_name',
fields: ['name'],
indexName: 'by_name',
partial_filter_selector: undefined
}
})
})

it('uses indexFields if provided', async () => {
spy = jest.spyOn(PouchDB.prototype, 'createIndex').mockReturnValue({})
await setup()
link.ensureIndex(TODO_DOCTYPE, {
await link.ensureIndex(TODO_DOCTYPE, {
indexedFields: ['myIndex']
})
expect(spy).toHaveBeenCalledWith({ index: { fields: ['myIndex'] } })
expect(spy).toHaveBeenCalled()
expect(spy).toHaveBeenCalledWith({
index: {
ddoc: 'by_myIndex',
fields: ['myIndex'],
indexName: 'by_myIndex',
partial_filter_selector: undefined
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth adding a simple test with an actual partialFilter

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #1524

})

it('uses the specified index', async () => {
Expand All @@ -615,9 +611,14 @@ describe('CozyPouchLink', () => {
})
const params = {
sort: undefined,
selector: {},
selector: {
myIndex2: {
$gt: null
}
},
fields: undefined,
limit: undefined,
partialFilter: undefined,
skip: undefined
}

Expand Down
Loading