Adding excludes option

[vinerich]

This PR adds an excludes: [] option which can be used to exclude certain paths of the versioned object from being versioned.

Excludes are defined as follows

ExcludeSchema.plugin(patchHistory, {
  mongoose,
  name: 'excludePatches',
  excludes: ['/hidden', '/object/hiddenProperty', '/array/*/hiddenProperty'],
})

A exclude of /hidden would exclude all paths that start with /hidden. For arrays I introduced the * symbol to specifiy a specific property inside the array to be excluded. One could ofcourse also specify a number to only exclude one array element.

Problems

Add the moment I have one failing test. This tests what patches are written when an object is saved initially. At the moment I do the comparison on the path-property of the json-patch. Since json-patch adds only the top-level-properties when an object is added: e.g. op:'add', path:'/object', value:'{ hiddenProperty: 'test' }' it would take a little more effort to make the exclude work for this kind of operation.

@codepunkt Do you think this behaviour would be necessary for consistency or can we only support the excludes option for updates() and not for initial writing?

[vinerich]

At the moment I do the comparison on the path-property of the json-patch. Since json-patch adds only the top-level-properties when an object is added: e.g. op:'add', path:'/object', value:'{ hiddenProperty: 'test' }' it would take a little more effort to make the exclude work for this kind of operation.

I added a comparison to make exclude work for this case.

@codepunkt Please take your time to review the changes introduced. Especially in 9cf4bab. I tried to comment the method deepRemovePath() to full extend, but sure I didn't. If there is something unclear, feel free to ask.

I enhanced the test for most cases I could think of this evening but there are probably more to cover, which brings me to my question. I'd really like this feature in the release, because I need it 😄. The logic is "rather" complicated and cause of that I would take it more as a beta-feature than as a fully tested one. Can't think of a case that would break it right now, but you can never be sure..

Would you include it in the release or do you say we need to do more test to be sure it works as intended?

Of course I'll provide documentation in the README if you choose to include it. Thanks in advance for your review.

[codepunkt]

I haven't been using mongodb or mongoose for a few years, so i can't comment on these parts. However, i think i understand the exclude use case.

I'm unsure about the deepRemovePath stuff because i don't understand how fast-json-patch diffentiates between nested paths and having objects as the value.

Documentation in the readme would be nice!

[vinerich]

Hey! At first thanks for your review, I appreciate it! I'll try to include your suggestions and make the code more understandable. Probably should've done this one more time before pushing it, but yesterday it seems reasonably good to me.

I'm unsure about the deepRemovePath stuff because i don't understand how fast-json-patch diffentiates between nested paths and having objects as the value.

As I understand it, the first added property is always a top-level-property, so it should always result in something like op:'add', path:'/top-level, value:'{ ... }'. Every other operation results in the lower-most property path it can get, to resemble the operation.

Taking this object as a starting point: { top: {} } Adding a third-level-property /top/second/third gives a patch like op:'add', path:'/top, value:'{ second: { third: { ... }}}'. Replacing this property after will give op:'replace', path:'/top/second/third, value:'{ ... }'. Removing this property after will give op:'remove', path:'/top/second/third.

Does that clear it up a little? I'd say that these cases are covered, because at first I check if the starting-path is the same

const deepRemovePath = (patch, pathToExclude) => {
  const patchPath = getArrayFromPath(patch.path)

  // first check if the base path of the json-patch overlaps with the path we want to exclude
  if (isPathCovered(patchPath, pathToExclude)) {
    let value = patch.value

and here I start to traverse at the matching index in pathToExclude

// because the paths overlap start at patchPath.length
    // e.g.
    // patch: { path:'/object', value:{ property: 'test' } }
    // pathToExclude: '/object/property'
    // need to start at array idx 1, because value starts at idx 0
    for (let i = patchPath.length; i < pathToExclude.length - 1; i++) {

Documentation will take place tomorrow, I think I need some sleep.

PS.: It is hard to break down the code, and especially the thoughts I had while writing it and how I expect it to work. I hope it is understandable. Thanks again for the review!!!

[codepunkt]

It's understandable, you're doing a really good job here!

[vinerich]

@codepunkt Thanks for approving! If you are fine with the README changes feel free to merge! If you want me to include another case or specify something further let me know! Also if any bugs encounter with this feature, summon me and I'll have a look!

[codepunkt]

@vinerich I don't see any README changes yet 😀

[vinerich]

@codepunkt There you go. The commit didn't work because I don't have my MongoDB on localhost and forgot to change it back after stash 😄