Skip to content

fix(document): handle nested paths in optimisticConcurrency include/exclude#16094

Open
mahmoodhamdi wants to merge 1 commit intoAutomattic:masterfrom
mahmoodhamdi:fix/issue-16054-optimisticConcurrency-nested-paths
Open

fix(document): handle nested paths in optimisticConcurrency include/exclude#16094
mahmoodhamdi wants to merge 1 commit intoAutomattic:masterfrom
mahmoodhamdi:fix/issue-16054-optimisticConcurrency-nested-paths

Conversation

@mahmoodhamdi
Copy link
Contributor

Problem

When using optimisticConcurrency with include arrays or exclude objects, nested paths are not handled properly.

Exclude case: Excluding profile does not exclude profile.firstName. Modifying profile.firstName still triggers versioning even though its parent is excluded.

const userSchema = new Schema({
  profile: profileSchema,
  balance: Number
}, { optimisticConcurrency: { exclude: ['profile'] } });

user.profile.firstName = 'Bob';
user.$__delta();
// BUG: user.$__.version === VERSION_ALL
// EXPECTED: user.$__.version === undefined

Include case: Including profile.address.country does not trigger versioning when setting profile.address (the parent).

Root Cause

The existing code uses Set.has() for exact path matching, which doesn't account for parent-child path relationships.

Solution

Added two helper functions:

  • _isExcludedByOptCon(path, excludeSet): a path is excluded if it matches exactly OR any of its parent prefixes is in the exclude set (e.g. profile excludes profile.firstName)
  • _isIncludedByOptCon(path, includeSet): a path is included if it matches exactly, OR any parent prefix is included, OR any included path is a child of the modified path (e.g. modifying profile.address triggers when profile.address.country is included)

Testing

Added 5 new test cases in test/versioning.test.js covering:

  1. Exclude: parent exclusion cascades to children
  2. Exclude: non-excluded paths still trigger version alongside excluded children
  3. Include: parent inclusion cascades to children
  4. Include: modifying parent triggers when child is included
  5. Include: non-included nested paths don't trigger

All 22 optimisticConcurrency tests pass.

Fixes #16054

…xclude

When using `optimisticConcurrency` with an include array or an exclude
object, nested paths were not handled properly. Excluding `profile`
did not exclude `profile.firstName`, and including
`profile.address.country` did not trigger versioning when setting
`profile.address`.

Added two helper functions that check parent-child path relationships:
- `_isExcludedByOptCon`: a path is excluded if it or any parent prefix
  is in the exclude set
- `_isIncludedByOptCon`: a path is included if it or any parent prefix
  is in the include set, or if any included path is a child of the
  modified path

Fixes Automattic#16054
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes optimisticConcurrency path matching so include/exclude configurations correctly account for nested path relationships (parent ↔ child), addressing gh-16054.

Changes:

  • Update Document#$__delta() optimistic concurrency checks to treat nested paths as relevant when appropriate (prefix/descendant matching rather than exact Set.has()).
  • Add internal helpers for include/exclude nested-path evaluation.
  • Add a focused test suite for nested include/exclude behavior in optimistic concurrency.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/document.js Implements nested-path aware include/exclude checks for optimistic concurrency in $__delta() via new helper functions.
test/versioning.test.js Adds regression tests covering nested include/exclude behavior for optimistic concurrency.

Comment on lines +5609 to +5616
// Check if any included path is a child (e.g. modifying 'profile.address' is
// relevant when 'profile.address.country' is included)
const prefix = path + '.';
for (const p of includeSet) {
if (p.startsWith(prefix)) {
return true;
}
}
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

A couple of performance related suggestions.

return true;
}
// Check if any parent prefix is included (e.g. 'profile' includes 'profile.name')
const pieces = path.split('.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to make sure to check if path contains '.' before splitting for performance

* handling nested paths in both directions.
*/

function _isIncludedByOptCon(path, includeSet) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see if there is a way to reuse split values in this function similar to hasIncludedChildren for projections in lib/document.js. This check is very expensive - lots of split() calls, loops. It would be ideal to avoid this by precomputing the split values from this.$__schema.options.optimisticConcurrency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] optimisticConcurrency nested paths

3 participants