stockpile-co / api

The API for Stockpile, an app that manages stuff for organizations.
0 stars 1 forks source link

Fix use of Knex.modify() #177

Closed AdamVig closed 7 years ago

AdamVig commented 7 years ago

./services/filter-query.js, ./controllers/model.js, and ./services/endpoint.js use Knex's modify function incorrectly. See the documentation.

Instead of returning functions (in filter-query.js) and binding parameters (in endpoint.js), just call modify with a function as its first parameter, and the other parameters will be passed to the function.

AdamVig commented 7 years ago

This is easy to fix in services/filter-query.js, but difficult to fix in all of the individual controllers that supply knex.modify() functions.

The reason it is difficult is because the flow looks like this:

  1. controller declares a modify function ((req, queryBuilder) => { ... })
  2. controller passes modify function to endpoint service
  3. endpoint service binds req as the first parameter to the modify function
  4. endpoint service passes the modify function to the db service
  5. db service calls modify function like knex.modify(modifyFunction), expecting the function to accept queryBuilder as its first parameter (which it does, after req is bound in the endpoint service)

The fix would look like:

The problem with this is that it increases the coupling between the endpoint service and the db service, a problem that will be solved in #373 Refactor endpoint service. Once the endpoint service is refactored to not pass parameters through to the db service, each controller can directly pass req to the db service so that modify() can be used correctly.