Update and patch manipulate data object

[kruschid]

Currently I'm using this library in one of my projects. I created a service to use the update and patch methods in order to manipulate a few documents and ran in some trouble with that. After passing my data object to these methods I encountered that the _id attribute was deleted in each data object. I looked for the lines caused the problem and found the following lines:

https://github.com/feathersjs/feathers-nedb/blob/master/src/index.js#L114
https://github.com/feathersjs/feathers-nedb/blob/master/src/index.js#L115
https://github.com/feathersjs/feathers-nedb/blob/master/src/index.js#L131

I was able to find a workaround but this implementation breaks with a principles of maintainable code. (Don't modify objects you don't own)

I didn't expect such an effect. What do you think?

[marshallswain]

@kruschid, did you try removing those lines to see the errors that you get as a result? You can't submit _id as part of the update or patch. It needs to be separated out into a different argument. By the time you get your document back, it should have an _id in place. We do have test coverage for this across all of the adapters, but it's possible we are missing a use case, I guess.

[kruschid]

@marshallswain, I understand your concerns. Yes you are right there is an _id by the time I get my document back. But in my case a have a mocha script with a before hook that pushes a bunch of documents to the db and an after hook that remove those documents. An there are some some tests that test a library that depend on feathers-nedb:

documentA = {_id:1, name:'..'};
documentB = {_id:2, ...};
...
items = [documentA, documentB, ...]

before( function(done){
  service.create(items).then( function(){
    done()
  });
});

it('should ...', function() {
  // here some tests that run components that depend on feathers service update 
});

before( function(done){
  ids = items.map( function(x){ return x._id } ) // surprisingly the _id attribute disappeared from each document 
  service.remove(null, ids).then( function(){
    done()
  });
});

According to NeDB documentation you can explicitly set an _id attribute wich is quite handy in many test cases. I also have some tests where I need to compare documents:

it('should ...', function(done){
  // updates certain items using feathers service 
  component.conditionalUpdate(items).then(function(items){
    // I don't care about the items provided by the callback in this test
    // here I need to ensure that certain documents weren't manipulated in the db
    service.get(documentA._id).then(function(document){
      document.should.be.deep.equal(documentA);
    });
  });
});

I just tried to comment out those three lines in the update method and it didn't cause any errors. What I didn't try is to find out what will happen if I call update with a modified _id.

[ekryski]

@kruschid yup totally fair. We should be using Object.assign and not explicitly deleting keys (as we are doing) in case you are referencing them (which you are in this case). You interested in making a PR? 😁

@marshallswain I think it's just that we don't pass in objects with existing ids. We structured our tests differently (not the way I think we should have) and so the tests never encounter this issue. It's probably an issue across a few of the adapters.

[daffl]

Closed via #30