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

Slight improvement: validate configuration #113

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .prettierrc.json
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
{
}
"printWidth": 80
}
3 changes: 2 additions & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ services:
command: sh -c "./wait-for.sh db:3306 -- npm run start:dev"

db:
image: mysql:5.6.44
platform: linux/x86_64
image: mysql:5.6.51
container_name: db
restart: unless-stopped
ports:
Expand Down
1,406 changes: 1,109 additions & 297 deletions package-lock.json

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@
"node-cron": "^2.0.3",
"pm2": "^4.5.0",
"resource-router-middleware": "^0.6.0",
"twilio": "^3.49.4"
"twilio": "^3.71.1"
},
"devDependencies": {
"@babel/cli": "^7.11.6",
"@babel/cli": "^7.16.0",
"@babel/core": "^7.11.6",
"@babel/node": "^7.10.5",
"@babel/plugin-proposal-optional-chaining": "^7.11.0",
Expand All @@ -52,6 +52,7 @@
"eslint-plugin-mocha": "^8.0.0",
"mocha": "^8.1.3",
"node-random-name": "^1.0.1",
"nodemon": "^2.0.15",
"nyc": "^15.1.0",
"prettier": "2.1.2",
"proxyquire": "^2.1.3",
Expand Down
21 changes: 9 additions & 12 deletions src/api/handlers/appointments/postAppointments.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import bodyParser from "body-parser";
import * as appointments from "../../../models/appointments";
import * as sendReminders from "../../../services/reminders";
import api from "../../../api";
import config from "../../../config";
import { config } from "../../../config";

config.jwtConfig.jwtSecret = "secret";

Expand Down Expand Up @@ -291,17 +291,14 @@ describe("POST /api/appointments", () => {
it("should fail on letters in phone number", function (done) {
const appointment = { ...baseAppointment };
appointment.patientPhoneNumber = "A555555555";
request(app)
.post("/api/appointments")
.send(appointment)
.expect(
500,
{
message:
'"patientPhoneNumber" with value "A555555555" fails to match the required pattern: /^\\d+$/',
},
done
);
request(app).post("/api/appointments").send(appointment).expect(
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the linting is handling this correctly but generally we have tried to do one new line per chained call.

Copy link
Author

Choose a reason for hiding this comment

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

It was for the linter. The printWidth property. It wasn't there.
I just added it.
So yeah now it's breaking down the chain calls to multiple lines if it's more than 80 chars.

500,
{
message:
'"patientPhoneNumber" with value "A555555555" fails to match the required pattern: /^\\d+$/',
},
done
);
});

it("should fail on whitespace only phone number", function (done) {
Expand Down
6 changes: 3 additions & 3 deletions src/api/handlers/auth/getToken.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import jwt from "jsonwebtoken";
import { jwtConfig } from "../../../config";
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to stick with the approach of importing what we need for config values!

import { config } from "../../../config";
import { getUserByEmail, hashPassword } from "../../../models/users";

/*
Expand Down Expand Up @@ -39,9 +39,9 @@ export async function getToken(req, res) {
const payload = {
userEmail: user.email,
};
const token = jwt.sign(payload, jwtConfig.jwtSecret, {
const token = jwt.sign(payload, config.jwtConfig.jwtSecret, {
algorithm: "HS256",
expiresIn: jwtConfig.jwtTokenExpiry,
expiresIn: config.jwtConfig.jwtTokenExpiry,
});

return res.json({
Expand Down
2 changes: 1 addition & 1 deletion src/api/handlers/auth/getToken.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import bodyParser from "body-parser";

import * as users from "../../../models/users";
import api from "../../../api";
import config from "../../../config";
import { config } from "../../../config";

config.jwtConfig.jwtSecret = "secret";

Expand Down
29 changes: 28 additions & 1 deletion src/config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const Joi = require("@hapi/joi");
const isDevelopment = process.env.NODE_ENV === "development";

module.exports = {
const config = {
port: process.env.PORT || 8080,
protocol: isDevelopment ? "http" : "https",
bodyLimit: "100kb",
Expand All @@ -19,3 +20,29 @@ module.exports = {
timezone: "America/Toronto",
},
};

module.exports.config = config;

module.exports.validateConfig = () => {
const { error } = Joi.object({
port: Joi.number().required(),
protocol: Joi.string().required().valid("http", "https"),
bodyLimit: Joi.string().required(),
corsHeaders: Joi.array().items(Joi.string()).required(),
twilioConfig: Joi.object({
number: Joi.string().required(),
accountSid: Joi.string().required(),
authToken: Joi.string().required(),
}).required(),
jwtConfig: Joi.object({
jwtSecret: Joi.string().required(),
jwtTokenExpiry: Joi.number().required(),
}).required(),
scheduler: Joi.object({
cron: Joi.string().required(),
timezone: Joi.string().required(),
}).required(),
}).validate(config);

if (error) throw new Error(error.message);
};
96 changes: 50 additions & 46 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,49 +6,53 @@ import jwt from "express-jwt";
import http from "http";
import morgan from "morgan";
import api from "./api";
import config from "./config";
import reminderScheduler from './services/scheduler'

let app = express();
app.server = http.createServer(app);

// logger
app.use(morgan("dev"));

// 3rd party middleware
app.use(
cors({
exposedHeaders: config.corsHeaders,
})
);

app.use(bodyParser.urlencoded({ extended: true }));

app.use(
bodyParser.json({
limit: config.bodyLimit,
})
);

reminderScheduler.start();

app.use(
jwt({
secret: config.jwtConfig.jwtSecret,
algorithms: ["HS256"],
}).unless({
path: [
"/api/twilio/reply", // Authentication handled by Twilio middleware
"/api/ping", // Open health check
"/api/auth", // Used to login
],
})
);

app.use("/api", api({ config }));

app.server.listen(config.port, () => {
console.log(`Started on port ${app.server.address().port}`);
});

export default app;
import { config, validateConfig } from "./config";
import reminderScheduler from "./services/scheduler";

class Application {
constructor() {
this.app = express();
}

start() {
validateConfig();
Copy link
Member

Choose a reason for hiding this comment

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

This will display the config error and won't start but do we want it to be because of the exception or because of the error? For example, if you add a try/catch block around validate config you would see

Error: "twilioConfig.number" is required
    at module.exports.validateConfig (/media/wwan/git/sanctuary-patient-api/src/config.js:47:20)
    at Application.start (/media/wwan/git/sanctuary-patient-api/src/index.js:20:7)
    at Object.<anonymous> (/media/wwan/git/sanctuary-patient-api/src/index.js:63:13)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Module._compile (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (node:internal/modules/cjs/loader:1147:10)
    at Object.newLoader [as .js] (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:104:7)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
[nodemon] app crashed - waiting for file changes before starting...

where right now we see:

/media/wwan/git/sanctuary-patient-api/src/config.js:50
  if (error) throw new Error(error.message);
             ^

Error: "twilioConfig.number" is required
    at module.exports.validateConfig (/media/wwan/git/sanctuary-patient-api/src/config.js:47:20)
    at Application.start (/media/wwan/git/sanctuary-patient-api/src/index.js:18:7)
    at Object.<anonymous> (/media/wwan/git/sanctuary-patient-api/src/index.js:57:13)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Module._compile (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (node:internal/modules/cjs/loader:1147:10)
    at Object.newLoader [as .js] (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:104:7)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

Node.js v17.0.1

Copy link
Author

Choose a reason for hiding this comment

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

If we add try/catch block, the problem is, it will continue the program.
But if we do not run the app at all, I think it will be more helpful for developers to figure out why.
What do you think?


this.app.server = http.createServer(this.app);
this.app.use(morgan("dev"));
// 3rd party middleware
this.app.use(cors({ exposedHeaders: config.corsHeaders }));
this.app.use(bodyParser.urlencoded({ extended: true }));
this.app.use(bodyParser.json({ limit: config.bodyLimit }));

// logger

reminderScheduler.start();

this.app.use(
jwt({ secret: config.jwtConfig.jwtSecret, algorithms: ["HS256"] }).unless(
{
path: [
"/api/twilio/reply", // Authentication handled by Twilio middleware
"/api/ping", // Open health check
"/api/auth", // Used to login
],
}
)
);

this.app.use("/api", api({ config }));

this.app.server.listen(config.port, () => {
console.log(`Started on port ${this.app.server.address().port}`);
});
}

stop() {
this.app.server.close();
}
}

const application = new Application();
application.start();

export default application;
2 changes: 1 addition & 1 deletion src/services/reminders.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
import { createMessage } from "../models/communications";
import TemplatesModel from "../models/templates";
import { sendMessage } from "./twilioClient";
import config from "../config";
import { config } from "../config";

const daysFromNow = (interval) => {
return moment()
Expand Down
2 changes: 1 addition & 1 deletion src/services/scheduler.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
var CronJob = require("cron").CronJob;

import { sendReminders } from "../services/reminders";
import config from "../config";
import { config } from "../config";

const tz = config.scheduler.timezone;

Expand Down
9 changes: 6 additions & 3 deletions src/services/twilioClient.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { twilioConfig } from "../config";
import { config } from "../config";

const twilio = require("twilio");
const countryCode = "+1";

export const webhook = twilio.webhook;

export const sendMessage = (phoneNumber, message) => {
const client = twilio(twilioConfig.accountSid, twilioConfig.authToken);
const client = twilio(
config.twilioConfig.accountSid,
config.twilioConfig.authToken
);
if (phoneNumber.length !== 10) {
throw Error("Phone number must be 10 digits long.");
}
Expand All @@ -17,7 +20,7 @@ export const sendMessage = (phoneNumber, message) => {

return client.messages.create({
body: message,
from: twilioConfig.number,
from: config.twilioConfig.number,
to: `${countryCode}${phoneNumber}`,
});
};
Expand Down