Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine submission middlewares into controllers #149

Closed
liangyuanruo opened this issue Aug 17, 2020 · 4 comments
Closed

Combine submission middlewares into controllers #149

liangyuanruo opened this issue Aug 17, 2020 · 4 comments
Assignees
Labels
contribute free for contributors to pick up P2 planned for next 1-2 months

Comments

@liangyuanruo
Copy link
Contributor

liangyuanruo commented Aug 17, 2020

Blocked by #144

Many of our middleware functions are being chained together and used as services, which is an anti-pattern. This causes problems such as having long-range object and type dependencies, which must be overridden by the developer with type assertions.

A better solution where possible, is to merge middlewares into a single controller function as far as possible so that dependencies are clearly defined.

@liangyuanruo liangyuanruo added the P2 planned for next 1-2 months label Aug 17, 2020
@arshadali172
Copy link
Contributor

  • Webhook controller should be removed and webhook service referenced in the submission controller

@liangyuanruo
Copy link
Contributor Author

@karrui @tshuli i had a discussion with @mantariksh today, and we thought that instead of chaining middlewares, we can group relevant parts of the celebrate middleware together.

For example, instead of doing this:

app.route(...).post(
  celebrate({
    body: Joi.object({
        valueA: Joi.string(),
        valueB: Joi.boolean()
    })
  })
  middlewareUsingValueA, // naked middleware function with implicit
  middlewareUsingValueB, // dependency on upstream middleware for protection
)

We can do this instead:

// Exported together so that it's not possible to
// use middleware without protection
export const middlewareUsingValueA = [
  celebrate({
    body: Joi.object({
        valueA: Joi.string(),
    })
  }),
  (req, res, next) => {...} // or implemented as a separate function
]

export const middlewareUsingValueB = [
  celebrate({
    body: Joi.object({
        valueB: Joi.string(),
    })
  }),
  (req, res, next) => {...}
]

app.route(...).post(
  middlewareUsingValueA,
  middlewareUsingValueB
)

@liangyuanruo
Copy link
Contributor Author

@frankchn could we get your help with merging the middlewares for the encrypt mode submission API /v2/submissions/encrypt/:formId([a-fA-F0-9]{24}), following the pattern for the email mode submission API? It's a fairly large refactor, but the end goal should look similar to EmailSubmissionRouter and the handleEmailSubmission function.

Happy to discuss further if necessary!

@r00dgirl r00dgirl changed the title Combine middlewares on each route into controllers; add integration tests Combine middlewares on each route into controllers Mar 10, 2021
@liangyuanruo liangyuanruo changed the title Combine middlewares on each route into controllers Combine submission middlewares into controllers Mar 10, 2021
@liangyuanruo liangyuanruo added the contribute free for contributors to pick up label Mar 10, 2021
@r00dgirl r00dgirl added this to the typescriptify backend milestone Mar 10, 2021
@liangyuanruo
Copy link
Contributor Author

liangyuanruo commented Mar 29, 2021

Note: to also refactor associated routes for

  • VerifiedContentMiddleware.encryptVerifiedSpcpFields
  • EncryptSubmissionMiddleware.prepareEncryptSubmission
  • encryptSubmissions.saveResponseToDb
  • webhookVerifiedContentFactory.post
  • SubmissionsMiddleware.sendEmailConfirmations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribute free for contributors to pick up P2 planned for next 1-2 months
Projects
None yet
Development

No branches or pull requests

7 participants