fix(poucdb-find-mergeAndedSelectors): improve handling of merging of selectors #9082
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Setup
Let's consider a case where the massageSelector fails to render some branches
with query
We expect that the doc with
_id : 5to be returned. But the actual return doesn't include that.Possible problem
the two selector
{b: {$gte:2}}and{b: {$gte:4}}gets merged into one{b: {$gte:4}}. However, due to the logic implemented at the below segment, we lose this merged selector.https://github.com/pouchdb/pouchdb/blob/f2c665a2a885437b9cea80dda62c02a93a137c1e/packages/node_modules/pouchdb-selector-core/src/utils.js#L96-L101
We can come up with more of similar cases where we lose the selector. So, removing it might be a good approach. However, then we actually fail to cover the case it was actually meant to handle ie
{b: {$eq:2}}and{b: {$eq:4}}. Which should not be merged into{b: {$eq:4}}due to the current implementation ofmergeEqfunctionhttps://github.com/pouchdb/pouchdb/blob/f2c665a2a885437b9cea80dda62c02a93a137c1e/packages/node_modules/pouchdb-selector-core/src/utils.js#L212-L221
Proposed Solution
I propose a simple fix to this. At first we can remove code segment
if (Object.keys(merged).length <= longest) { return; }. Then for conflicting$eqconditions, we add a$neto the condition, thus causing the selector to be false by default. Although this is a bit hacky, but it will make the logic a bit easier. Later, we can filter the selector based on this contradiction instead of length.function mergeEq(value, fieldMatchers) { // these all have less specificity than the $eq // TODO: check for user errors here delete fieldMatchers.$gt; delete fieldMatchers.$gte; delete fieldMatchers.$lt; delete fieldMatchers.$lte; delete fieldMatchers.$ne; + if ( '$eq' in fieldMatchers && fieldMatchers.$eq !== value ) { + mergeNe(value, fieldMatchers); + } fieldMatchers.$eq = value; }Alternative Solution
Alternatively, we can also introduce a new field like
$falacyor something, which will be set to true when such contradictions arise. Then like before we can filter the selector based on that.Testing
I have tested the change against some edge cases. So, far it seems to be working. And it passes all the tests too for
TYPE=find PLUGINS=pouchdb-find COUCH_HOST='http://admin:password@localhost:5984' npm testNote 1
Although
$eqhas more specificity than the others, but deleting the other operators might not be the best choice here. Having an operator like$gte : 10and merging it with$eq : 5should keep both, as the correct merged selector is an always false selector. But deleting$gtewill cause it to change its behavior.