Implementation of patchById

[Amir-61]

I just managed to do very initial implementation of patchById and opened this PR to have some discussion to see whether we agree about the context of different hooks considering the fact that in this method you would not have access to instance and currentInstance.

Apart from this I need to write a bunch of tests to verify that patchById works well and their operations hooks are triggered in the right way.

@bajtos PTAL :-)

Connect to https://github.com/strongloop-internal/scrum-loopback/issues/752
Thanks!

[bajtos]

I am proposing to use the same context as we have for updateAll:

• Model
• where
• data
• hookState
• options

Note that instance id can be extracted from the where clause, see https://github.com/strongloop/loopback/blob/4cd84dcd8af5f4f90ff62f506a6bf49e6fc2036b/lib/persisted-model.js#L1508-L1518.

We can eventually put getIdFromWhereByModelId into a public LoopBack API if there is enough demand.

This is the reason why I am reluctant to add instanceId: when we have both where and instanceId, and a hook changes one of these properties only, it's not clear which one takes precedence. I suppose we could make instanceId read-only, although the downside is that Object.defineProperty incurs performance penalty :(

[bajtos]

@Amir-61 one more thought to consider: eventually, I think it will be good to refactor lib/dao.js into multiple smaller files, one file per each operation/method: lib/dao/patch-by-id.js, lib/dao/create.js, etc. (@raymondfeng @superkhau is this in line with what you have in mind for juggler refactor?) Now that we are adding a new DAO method, I think it will be better to start following this future convention, to save us work in the future when we actually get to make the changes.

I am proposing the following:

1. Move the body of patchById to a new file lib/dao/patch-by-id.js.

module.exports = function patchById(...) {
  // ...
};

2. In lib/dao.js, import the method from the external file and save it on the DataAcessObject.

DataAccessObject.patchById = require('./dao/patch-by-id');

3. A similar approach can be applied to unit-tests. At the moment, most DAO methods have tests in test/manipulation.test.js and there is also giant test/persistence-hooks.suite.js. I have already created infrastructure for one-file-per-feature for operation hooks in Implement operation hooks for EmbedsOne methods #904, I am proposing to put your new tests into test/operation-hooks.suite/dao-patch-by-id.suite.js.

4. As for test/manipulation.test.js, I'd create test/dao.suite/index.js similar to test/operation-hooks.suite/index.js, put your new tests into test/dao.suite/patch-by-id.suite.js, and then load test/dao.suite/index.js from test/manipulation.test.js to make sure they are run as part of the suite run by connectors.

Thoughts?

[superkhau]

The refactor we talked about was smaller in scope but the ideas you're proposing are in line with the steps after. As you say, we might as well get it started now since you're already working on it. Also, the way to align tests into suites by feature is also what I had in mind.

[bajtos]

Please move the new file under a new folder, i.e. dao/patch-by-id.js.