Ensure params are always passed through to all service calls

Take a look at these two lines:

Line 244 in 6f5c547

    
           return this._get(id, { sequelize: { raw: false }, query: where }).then(instance => {

feathers-sequelize/lib/index.js

Line 258 in 6f5c547

    
           return instance.update(copy, {raw: false}).then(() => this._get(id, {sequelize: options}));

Lines like these do overrides such as raw: false, which is totally fine, but in the process they also fail to pass through the other attributes of the params object that was provided by the original caller of the service method (further up the callstack).

When a service call is made by some featherjs user's code, it's important to preserve the params object when making any subsequent service calls, as it's possible that the user of feathersjs intends to use these parameters as part of a getModel() override function in a custom service class (i.e. class MySpecialService extends Service with an overidden getModel()) For things like multi-tenancy schemes - hosting multiple domains/sites out of a single app instance such that each site needs to hit a different database. As far as I'm aware, a getModel() override is the best way of achieving that goal, and any practice of ignoring/clobbering the params passed in will cause problems for such a scheme.

For the most part I have not noticed an issue with this -- As far as I know, get, find, and most other service calls do not have this practice of completely overriding the params with their own value, but update does have this issue. I will submit a pull request for this.

There are some other bits that are of note:

Are there any other service calls that are making the same mistake of not passing through params (within sequelize, or for backends other than sequelize)?
Are there any tests that can be written to catch this kind of problem?

I can propose a possible test here:
Make a custom service like this:

let Service = require('feathers-sequelize').Service; // TODO: Test other service backends, too.
class ParamsTestService extends Service {
  getModel(params) {
    if (params.testValue !== 42)
    {
      throw new Error("TEST FAILURE: testValue should have been passed through to getModel()!");
    }
  }
}

Then have a test suite that runs a ton of different service calls against the custom service (updates/gets/etc) through both REST and socketio against various database backends, always passing {testValue:42} for params argument to those service calls. If any tests throw, this means some subsequent service call deeper in the callstack is failing to preserve the params, and it is a test failure.

Additionally, I think we'd also want to have a:

  if (hook.params.testValue !== 42)
    {
      throw new Error("TEST FAILURE: testValue should have been passed through to hooks!");
    }

Inside of the before app hooks for the testsuite. This is actually how I caught the problem in my setup -- I have something that checks the params for a certain attribute and throws if it's not there.

I'm not sure what feathersjs has in the way of a testsuite currently... if it already has something like this, it may be very easy to plug in the custom service aspect, if there's nothing like this currently then it could be a lot of work.

Added tests for custom attributes for custom getModel().

Fix some getModel() / parameter passing related test failures.

Added tests for custom attributes for custom getModel().

Fix some getModel() / parameter passing related test failures.

Added tests for custom attributes for custom getModel().

Fix some getModel() / parameter passing related test failures.

Added getModel override param passing tests and fixes (#265)

Navigation Menu

Repository navigation

Ensure `params` are always passed through to all service calls #234

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Ensure params are always passed through to all service calls #234

Description

Activity

AndrewJDR commented on Sep 6, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Ensure `params` are always passed through to all service calls #234