Pipelines: Test coverage of Pipelines API endpoint #2209
Conversation
| @@ -7,7 +7,15 @@ module.exports = { | |||
| schema: { | |||
| name: { | |||
| type: DataTypes.STRING, | |||
| allowNull: false | |||
| allowNull: false, | |||
| validate: { | |||
There was a problem hiding this comment.
Validation being handled at a model rather than controller level, is a pattern I would like us to start repeating throughout our codebase. @knolleary @Steve-Mcl @joepavitt
There was a problem hiding this comment.
@Pezmc I have no argument but can I trouble you to provide a little context please? I am not 100% on sequelize models and controllers and how every single piece fits together. I am assuming you raised this as you are aware of some best practice & it would improve our code quality and ... ?
Ta.
There was a problem hiding this comment.
Sure! Ultimately it is the model that is being validated, having validations and constraints at the model (and database level) means we can be 100% confident that all of our models are meeting the validation.
Right now, we usually validate the user input at the controller level, but there's nothing stopping an "invalid" model from being created, it also leads to duplicate validation logic spread throughout the app (e.g. to check a name is provided). Centralising this validation reduces code duplication, while also making our models more reliable.
Happy to spin up a separate issue (or drop into a huddle) to discuss further! I might end up writing this up as part of a "best practices" doc for us (thoughts @ZJvandeWeg @knolleary?)
There was a problem hiding this comment.
Controller validation should be done for security purposes and user feedback.
Models are indeed the level to validate on a table scale. There's always going the be some overlap.
Our code base has fairly fat controllers sadly, and do too much heavy lifting. More logic should reside in the model layer for a higher quality code base.
This should be documented indeed in developer docs. Would be great to do to raise the quality of contributions
Description
Originally added in: #2082
This PR adds comprehensive test coverage of the Pipelines API, catching and fixing a few bugs and API improvements along the way.
Related Issue(s)
Fixes #2171 for #2124
Checklist
flowforge.yml?flowforge/helmto update ConfigMap Templateflowforge/CloudProjectto update values for Staging/ProductionLabels
backportlabelarea:migrationlabel