Patching entity through service causes the socket entity to be wrongly updated

[claustres]

Steps to reproduce

This behavior seems to have appeared after the migration to V3.

Authenticate a user which data model has two properties email and name. Patch one of the property like app.service('users').patch(id, { name: 'xxx' }). After patch the user entity attached to the socket has lost the email property. It will cause any hook or operation depending on it to stop working on next service call.

Expected behavior

The socket entity should be correctly patched.

Actual behavior

The socket entity is updated instead of being patched.

This code could be the problem since the same function is called for both update or patch operations and this function delete properties based on the input object.

System configuration

Module versions:

"@feathersjs/authentication": "^2.1.7",
"@feathersjs/authentication-jwt": "^2.0.1",
"@feathersjs/authentication-local": "^1.2.1",
"@feathersjs/authentication-oauth2": "^1.1.0",
"@feathersjs/client": "^3.5.3",
"@feathersjs/configuration": "^1.0.2",
"@feathersjs/errors": "^3.3.0",
"@feathersjs/express": "^1.2.3",
"@feathersjs/feathers": "^3.1.7",
"@feathersjs/socketio": "^3.2.2"

NodeJS version: 8

Operating System: Windows

[claustres]

If I am not wrong about this issue the proposed patch is to still call the updateEntity function but with an additional parameter indicating the operation. Based on this we could skip the code starting from here in case of a patch. Could work on a PR but I have to learn about lerna first I guess 8-)

[daffl]

Hopefully no need to learn Lerna, npm install cd'ing into the package and running npm test there should do everything you need. This is also one of the problems that should be solved via #1045.

[daffl]

Would still be nice to get some of this maybe sorted out before. #991 is probably related to this as well.

[claustres]

I wonder about one thing with lerna, if I do a fork of feathers to patch eg the authentication module, then submit a PR, then how can I use my fork in my app waiting for the release ? Previously I could point my package.json toward the master branch of the feathers-authentication repo but now everything is in the same repo is it still possible ?

[claustres]

I ran into issues with the new lerna repo.

First npm run test does not work saying '..' is not a recognized command when running ../../node_modules/.bin/mocha --opts ../../mocha.opts.

Second the tests seems to halt without exiting after running those at root level, tests in subdirectories are not executed.

I am running Node.js 8.9 under Windows 10, this might be linked.

For now I propose to PR against the old repo, I can PR here as well but I cannot run the tests for now.

[bertho-zero]

@claustres Can you try by wrapping the path in quottes ?

../../node_modules/.bin/mocha --opts '../../mocha.opt'

[claustres]

@bertho-zero just tested and it does not work , what works is:

"scripts": {
    "test": "..\\..\\node_modules\\.bin\\mocha --opts ../../mocha.opts"
  }

I guess previously we had something like:

"mocha": "mocha --opts mocha.opts"

with the package installed locally, which allowed to directly call mocha.

Don't know how to make it work cross-platform.

[bertho-zero]

And by adding the node command before ? like:

"test": "node '../../node_modules/.bin/mocha' --opts '../../mocha.opt'"

[claustres]

Result:

basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^

SyntaxError: missing ) after argument list
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:599:28)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:676:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3

Correct me if I am wrong but the .bin directory contains bash scripts or windows command files, not node files, so this could not work.

The problem seems to be related to command path with slash under Windows. If I add mocha as a dev dependency of the package then this works:

"scripts": {
  "test": "mocha --opts ../../mocha.opts"
}

npm run adds ./node_modules/.bin to your PATH so that now it can correctly find the mocha command without requiring to use a relative path.

However I do not know lerna enough to see if this can cause a problem to add mocha in each package.

[daffl]

Ah sorry about that, I haven't gotten a chance to test this on Windows yet. I think #1051 should help with this problem.

[bertho-zero]

Both .bin/mocha and .bin/_mocha starts with a node shebang: #!/usr/bin/env node.

Lerna's interest is to have only one system for testing, linting, and so on. Mocha is installed once on root and reused in packages.

The ideal in a monorepo would be to use Jest and the projects. (https://jestjs.io/docs/en/configuration.html#projects-array-string-projectconfig)

[claustres]

Ah sorry about that, I haven't gotten a chance to test this on Windows yet. I think #1051 should help with this problem.

Yes that's exactly what I tested fine but I don't know how it works with lerna to ensure you have a "shared" mocha between all packages.

[daffl]

@claustres Can you try again with latest master. It should install the mocha binary for your operating system and run without needing the folder separator.

[claustres]

I ran into another issue caused by the entity update and by using MongoDB with feathers-sync. When the socket initializes the user is retrieved from the DB and his id property is an ObjectID. Then if the user is patched, because event data are serialized in MongoDB when using feathers-sync, his id becomes a string after the entity update. This causes any subsequent operation based on the id to fail in MongoDB.

I wonder if it would not be better to ignore the id on entity update or to make the update function customizable to handle the various possible business cases.

I also wonder more generally about listening to events with feathers-sync and similar issues. Indeed, in this case the events all go through the serialization process, even on the initiator server. By serializing event data each server listening to events receives "client-like data" so that we should apply any relevant conversion functions like services do with hooks (eg for ObjectIDs, Dates, etc.) to ensure consistency.

Let me know if it is already possible to do something with respect to this using the current Feathers or what you think about that for future improvements.

[claustres]

@daffl Sorry for the delay, I've just tested your changes and now the tests run fine, thanks.

[lock]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.