Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Remove core instance dependency at plugin level #433

Merged
merged 7 commits into from
Oct 24, 2017

Conversation

mihaidma
Copy link
Contributor

@mihaidma mihaidma commented Oct 13, 2017

The purpose of this PR is to remove the dependency on a common Udaru Core instance passed as parameter to routes and other functionality at the plugin level.

The changes are the following:

  • All routes expect a decorated Udaru instance on request,
  • Routes Joi validation is used by directly requiring the Joi validation files. Validation is not gathered any more from a common Udaru instance,
  • The authorization checks made at route level are using the Udaru core instance decorated on request
  • The service has an option with wich the Udaru core decoration can be disabled (it is enabled by default). This allows Udaru core to be passed from outside from the service level.
  • Added a small change on the Factory used by tests to generate data in db. As it was using a default Udaru core (with default settings) added the possibility to pass the Udaru core instance from outside.

While doing the changes did also two fixes:

  • A db pool passed from outside should not be released by Udaru core,
  • config params were not properly passed from the plugin level to Udaru core level.

This dependency removal offers the users of Udaru the option to pass from outside their own Udaru core instance and in a multitenant environment pass different per tenant Udaru core Instances on each request. #430

Each step is made as a separate commit to make the review easier.

@coveralls
Copy link

coveralls commented Oct 13, 2017

Coverage Status

Coverage decreased (-0.05%) to 96.397% when pulling ddf0b59 on issue-430-connections-decorated into 55767b4 on master.

@dberesford
Copy link
Contributor

All looks good to me @mihaidma

@coveralls
Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.05%) to 96.397% when pulling 349a05f on issue-430-connections-decorated into 55767b4 on master.

Copy link
Member

@paolochiodi paolochiodi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I usually prefer explicit dependencies (in function parameters) to "hidden" dependencies using (i.e using request decoration), but I understand this is required to be able to use a different udaru core on different requests.

👍

@@ -4,7 +4,7 @@ const _ = require('lodash')
const Boom = require('boom')
const async = require('async')

function buildAuthorization (udaru, config) {
function buildAuthorization (config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we also remove config here and use the decorated one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 As the PR got big I'll merge it then see what enhancements we can come up with.

@mihaidma mihaidma merged commit dac5421 into master Oct 24, 2017
@mihaidma mihaidma deleted the issue-430-connections-decorated branch October 24, 2017 06:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants