Repository: rickbergfalk/sqlpad
Pull request #452 (2019-08-01 02:48:26)

Simplify config module
More config cleanup favoring simplicity


Patch:
diff --git a/server/app.js b/server/app.js
index d5bec9ef..d2266ba7 100644
--- a/server/app.js
+++ b/server/app.js
@@ -5,16 +5,15 @@ const express = require('express');
 const helmet = require('helmet');
 const session = require('express-session');
 const FileStore = require('session-file-store')(session);
-const configUtil = require('./lib/config');
+const config = require('./lib/config');
 const version = require('./lib/version');
-const {
-  baseUrl,
-  googleClientId,
-  googleClientSecret,
-  publicUrl,
-  dbPath,
-  debug
-} = configUtil.getPreDbConfig();
+
+const baseUrl = config.get('baseUrl');
+const googleClientId = config.get('googleClientId');
+const googleClientSecret = config.get('googleClientSecret');
+const publicUrl = config.get('publicUrl');
+const dbPath = config.get('dbPath');
+const debug = config.get('debug');
 
 // Cookie secrets are generated randomly at server start
 // SQLPad (currently) is designed for running as a single instance
@@ -83,17 +82,6 @@ if (debug) {
   app.use(morgan('dev'));
 }
 
-// Add config helper to req
-app.use(async function(req, res, next) {
-  try {
-    req.config = configUtil.getHelper();
-    next();
-  } catch (error) {
-    console.error('Error getting config helper', error);
-    next(error);
-  }
-});
-
 /*  Passport setup
 ============================================================================= */
 require('./middleware/passport.js');
diff --git a/server/drivers/index.js b/server/drivers/index.js
index 95961f4c..6ef65fa4 100644
--- a/server/drivers/index.js
+++ b/server/drivers/index.js
@@ -1,8 +1,10 @@
 const uuid = require('uuid');
-const { debug } = require('../lib/config').getPreDbConfig();
+const config = require('../lib/config');
 const utils = require('./utils');
 const getMeta = require('../lib/getMeta');
 
+const debug = config.get('debug');
+
 const drivers = {};
 
 /**
diff --git a/server/lib/cipher.js b/server/lib/cipher.js
index ed7d4cca..af03dc07 100644
--- a/server/lib/cipher.js
+++ b/server/lib/cipher.js
@@ -1,6 +1,8 @@
 const crypto = require('crypto');
 const algorithm = 'aes256';
-const { passphrase } = require('../lib/config').getPreDbConfig();
+const config = require('../lib/config');
+
+const passphrase = config.get('passphrase');
 
 module.exports = function(text) {
   const myCipher = crypto.createCipher(algorithm, passphrase);
diff --git a/server/lib/config/index.js b/server/lib/config/index.js
index 5db837d3..52dd1656 100644
--- a/server/lib/config/index.js
+++ b/server/lib/config/index.js
@@ -22,80 +22,36 @@ const cliConfig = fromCli(argv);
 const savedCliConfig = fromCli(savedArgv);
 const envConfig = fromEnv();
 
-function setBy(cliConfig, savedCliConfig, envConfig, key) {
-  if (cliConfig[key]) {
-    return 'cli';
-  } else if (savedCliConfig[key]) {
-    return 'saved cli';
-  } else if (envConfig[key]) {
-    return 'env';
-  } else {
-    return 'default';
+const all = Object.assign(
+  {},
+  defaultConfig,
+  envConfig,
+  savedCliConfig,
+  cliConfig
+);
+
+exports.get = function get(key) {
+  if (!key) {
+    throw new Error('key must be provided');
   }
-}
 
-/**
- * TODO FIXME XXX - this is no longer necessary as config helper is all the config.
- * there is no more db to consider
- * @returns {object} configMap
- */
-exports.getPreDbConfig = function getPreDbConfig() {
-  return Object.assign({}, defaultConfig, envConfig, savedCliConfig, cliConfig);
+  if (!all.hasOwnProperty(key)) {
+    throw new Error(`config item ${key} not defined in configItems.js`);
+  }
+  return all[key];
 };
 
-/**
- * Gets config helper using all config sources
- * @returns {Promise} configHelper
- */
-exports.getHelper = function getHelper() {
-  const all = Object.assign(
-    {},
-    defaultConfig,
-    envConfig,
-    savedCliConfig,
-    cliConfig
-  );
+exports.getUiConfig = () => {
+  return definitions
+    .filter(item => item.uiDependency)
+    .reduce((configMap, item) => {
+      configMap[item.key] = all[item.key];
+      return configMap;
+    }, {});
+};
 
-  const configHelper = {
-    get: key => {
-      if (!all.hasOwnProperty(key)) {
-        throw new Error(`config item ${key} not defined in configItems.js`);
-      }
-      return all[key];
-    },
-    getConfigItems: () => {
-      return definitions.map(definition => {
-        return Object.assign({}, definition, {
-          effectiveValue: all[definition.key],
-          effectiveValueSource: setBy(
-            cliConfig,
-            savedCliConfig,
-            envConfig,
-            definition.key
-          ),
-          envValue: envConfig[definition.key],
-          cliValue: cliConfig[definition.key],
-          savedCliValue: savedCliConfig[definition.key]
-        });
-      });
-    },
-    getUiConfig: () => {
-      return definitions
-        .filter(item => item.uiDependency)
-        .reduce((configMap, item) => {
-          configMap[item.key] = all[item.key];
-          return configMap;
-        }, {});
-    },
-    smtpConfigured: () =>
-      all.smtpHost &&
-      all.smtpUser &&
-      all.smtpFrom &&
-      all.smtpPort &&
-      all.publicUrl,
-    googleAuthConfigured: () =>
-      all.publicUrl && all.googleClientId && all.googleClientSecret
-  };
+exports.smtpConfigured = () =>
+  all.smtpHost && all.smtpUser && all.smtpFrom && all.smtpPort && all.publicUrl;
 
-  return configHelper;
-};
+exports.googleAuthConfigured = () =>
+  all.publicUrl && all.googleClientId && all.googleClientSecret;
diff --git a/server/lib/db.js b/server/lib/db.js
index 8292ac5c..453811bb 100644
--- a/server/lib/db.js
+++ b/server/lib/db.js
@@ -1,7 +1,12 @@
 const path = require('path');
 const datastore = require('nedb-promise');
 const mkdirp = require('mkdirp');
-const { admin, dbPath, debug, port } = require('./config').getPreDbConfig();
+const config = require('./config');
+
+const admin = config.get('admin');
+const dbPath = config.get('dbPath');
+const debug = config.get('debug');
+const port = config.get('port');
 
 mkdirp.sync(path.join(dbPath, '/cache'));
 
diff --git a/server/lib/decipher.js b/server/lib/decipher.js
index 13b587bb..48ca185b 100644
--- a/server/lib/decipher.js
+++ b/server/lib/decipher.js
@@ -1,6 +1,7 @@
 const crypto = require('crypto');
 const algorithm = 'aes256';
-const { passphrase } = require('../lib/config').getPreDbConfig();
+const config = require('../lib/config');
+const passphrase = config.get('passphrase');
 
 /**
  * @param {string} gibberish ciphered value that needs deciphering
diff --git a/server/lib/email.js b/server/lib/email.js
index 2d0bdfc3..f1d4519c 100644
--- a/server/lib/email.js
+++ b/server/lib/email.js
@@ -1,15 +1,15 @@
 const nodemailer = require('nodemailer');
-const configUtil = require('./config');
-const { baseUrl, port, publicUrl } = require('./config').getPreDbConfig();
+const config = require('./config');
 
 /**
  * Get full sqlpad url
  * @param {string} path - path (leading slash)
  */
 function fullUrl(path) {
+  const port = config.get('port');
   const urlPort = port === 80 ? '' : ':' + port;
-  const urlPublicUrl = publicUrl;
-  const urlBaseUrl = baseUrl;
+  const urlPublicUrl = config.get('publicUrl');
+  const urlBaseUrl = config.get('baseUrl');
   return `${urlPublicUrl}${urlPort}${urlBaseUrl}${path}`;
 }
 
@@ -36,8 +36,6 @@ function sendInvite(to) {
 }
 
 async function send(to, subject, text, html) {
-  const config = configUtil.getHelper();
-
   if (!config.smtpConfigured()) {
     console.error('email.send() called without being configured');
     return;
diff --git a/server/lib/version.js b/server/lib/version.js
index 1ba36d61..b9cc3144 100644
--- a/server/lib/version.js
+++ b/server/lib/version.js
@@ -1,7 +1,7 @@
 const packageJson = require('../package.json');
 const latestVersion = require('latest-version');
 const semverDiff = require('semver-diff');
-const configUtil = require('./config');
+const config = require('./config');
 
 const ONE_DAY = 1000 * 60 * 60 * 24;
 
@@ -26,7 +26,6 @@ function logUpdateAvailable(version) {
 
 async function checkForUpdate() {
   try {
-    const config = configUtil.getHelper();
     if (config.get('disableUpdateCheck')) {
       return;
     }
diff --git a/server/middleware/must-be-authenticated-or-chart-link-noauth.js b/server/middleware/must-be-authenticated-or-chart-link-noauth.js
index 3dc55356..3dad0d9a 100644
--- a/server/middleware/must-be-authenticated-or-chart-link-noauth.js
+++ b/server/middleware/must-be-authenticated-or-chart-link-noauth.js
@@ -1,10 +1,10 @@
 const passport = require('passport');
+const config = require('../lib/config');
 
 // If authenticated continue
 // If not and auth header is present, try authenticated with http basic
 // Otherwise redirect user to signin
 module.exports = function mustBeAuthenticatedOrChartLinkNoAuth(req, res, next) {
-  const { config } = req;
   if (req.isAuthenticated() || !config.get('tableChartLinksRequireAuth')) {
     return next();
   }
diff --git a/server/middleware/must-be-authenticated.js b/server/middleware/must-be-authenticated.js
index 672d4bc6..ccfcdc0f 100644
--- a/server/middleware/must-be-authenticated.js
+++ b/server/middleware/must-be-authenticated.js
@@ -1,10 +1,10 @@
 const passport = require('passport');
+const config = require('../lib/config');
 
 // If authenticated continue
 // If not and auth header is present, try authenticated with http basic
 // Otherwise redirect user to signin
 module.exports = function mustBeAuthenticated(req, res, next) {
-  const { config } = req;
   if (req.isAuthenticated()) {
     return next();
   }
diff --git a/server/middleware/passport.js b/server/middleware/passport.js
index 7847c837..72559f0c 100644
--- a/server/middleware/passport.js
+++ b/server/middleware/passport.js
@@ -3,15 +3,14 @@ const PassportLocalStrategy = require('passport-local').Strategy;
 const PassportGoogleStrategy = require('passport-google-oauth20').Strategy;
 const BasicStrategy = require('passport-http').BasicStrategy;
 const User = require('../models/User.js');
-const configUtil = require('../lib/config');
+const config = require('../lib/config');
 const checkWhitelist = require('../lib/check-whitelist.js');
-const {
-  baseUrl,
-  googleClientId,
-  googleClientSecret,
-  publicUrl,
-  disableUserpassAuth
-} = require('../lib/config').getPreDbConfig();
+
+const baseUrl = config.get('baseUrl');
+const googleClientId = config.get('googleClientId');
+const googleClientSecret = config.get('googleClientSecret');
+const publicUrl = config.get('publicUrl');
+const disableUserpassAuth = config.get('disableUserpassAuth');
 
 passport.serializeUser(function(user, done) {
   done(null, user.id);
@@ -112,7 +111,6 @@ async function passportGoogleStrategyHandler(
   }
 
   try {
-    const config = configUtil.getHelper();
     let [openAdminRegistration, user] = await Promise.all([
       User.adminRegistrationOpen(),
       User.findOneByEmail(email)
diff --git a/server/models/Cache.js b/server/models/Cache.js
index fb784a3f..b2f4e599 100644
--- a/server/models/Cache.js
+++ b/server/models/Cache.js
@@ -4,7 +4,8 @@ const Joi = require('joi');
 const db = require('../lib/db.js');
 const xlsx = require('node-xlsx');
 const json2csv = require('json2csv');
-const { dbPath } = require('../lib/config').getPreDbConfig();
+const config = require('../lib/config');
+const dbPath = config.get('dbPath');
 
 const schema = {
   _id: Joi.string().optional(), // will be auto-gen by nedb
diff --git a/server/models/Query.js b/server/models/Query.js
index 90d51969..8bc85fa2 100644
--- a/server/models/Query.js
+++ b/server/models/Query.js
@@ -1,5 +1,5 @@
 const db = require('../lib/db.js');
-const configUtil = require('../lib/config');
+const config = require('../lib/config');
 const Joi = require('joi');
 const request = require('request');
 
@@ -88,7 +88,6 @@ Query.prototype.save = function save() {
 };
 
 Query.prototype.pushQueryToSlackIfSetup = function() {
-  const config = configUtil.getHelper();
   const SLACK_WEBHOOK = config.get('slackWebhook');
   if (SLACK_WEBHOOK) {
     const PUBLIC_URL = config.get('publicUrl');
diff --git a/server/routes/app.js b/server/routes/app.js
index 3ad3e8f7..2a23bfdf 100644
--- a/server/routes/app.js
+++ b/server/routes/app.js
@@ -3,13 +3,13 @@ const passport = require('passport');
 const version = require('../lib/version.js');
 const User = require('../models/User.js');
 const sendError = require('../lib/sendError');
+const config = require('../lib/config');
 
 // NOTE: this route needs a wildcard because it is fetched as a relative url
 // from the front-end. The static SPA does not know if sqlpad is mounted at
 // the root of a domain or if there is a base-url provided in the config
 router.get('*/api/app', async (req, res) => {
   try {
-    const { config } = req;
     const adminRegistrationOpen = await User.adminRegistrationOpen();
     const currentUser =
       req.isAuthenticated() && req.user
diff --git a/server/routes/download-results.js b/server/routes/download-results.js
index 4104e3c7..427219f2 100644
--- a/server/routes/download-results.js
+++ b/server/routes/download-results.js
@@ -1,10 +1,10 @@
 const fs = require('fs');
 const router = require('express').Router();
 const Cache = require('../models/Cache.js');
+const config = require('../lib/config');
 
 router.get('/download-results/:cacheKey.csv', async function(req, res, next) {
   try {
-    const { config } = req;
     if (config.get('allowCsvDownload')) {
       const cache = await Cache.findOneByCacheKey(req.params.cacheKey);
       if (!cache) {
@@ -27,7 +27,6 @@ router.get('/download-results/:cacheKey.csv', async function(req, res, next) {
 
 router.get('/download-results/:cacheKey.xlsx', async function(req, res, next) {
   try {
-    const { config } = req;
     if (config.get('allowCsvDownload')) {
       const cache = await Cache.findOneByCacheKey(req.params.cacheKey);
       if (!cache) {
diff --git a/server/routes/forgot-password.js b/server/routes/forgot-password.js
index a152c81a..b7ba2620 100644
--- a/server/routes/forgot-password.js
+++ b/server/routes/forgot-password.js
@@ -3,10 +3,9 @@ const uuid = require('uuid');
 const User = require('../models/User.js');
 const email = require('../lib/email');
 const sendError = require('../lib/sendError');
+const config = require('../lib/config');
 
 router.post('/api/forgot-password', async function(req, res) {
-  const { config } = req;
-
   if (!req.body.email) {
     return sendError(res, null, 'Email address must be provided');
   }
diff --git a/server/routes/oauth.js b/server/routes/oauth.js
index 6407f750..282e99b0 100644
--- a/server/routes/oauth.js
+++ b/server/routes/oauth.js
@@ -1,6 +1,7 @@
 const passport = require('passport');
 const router = require('express').Router();
-const { baseUrl } = require('../lib/config').getPreDbConfig();
+const config = require('../lib/config');
+const baseUrl = config.get('baseUrl');
 
 router.get(
   '/auth/google',
diff --git a/server/routes/queries.js b/server/routes/queries.js
index 5ef5d1ca..3060eb5b 100644
--- a/server/routes/queries.js
+++ b/server/routes/queries.js
@@ -3,6 +3,7 @@ const Query = require('../models/Query.js');
 const mustBeAuthenticated = require('../middleware/must-be-authenticated.js');
 const mustBeAuthenticatedOrChartLink = require('../middleware/must-be-authenticated-or-chart-link-noauth.js');
 const sendError = require('../lib/sendError');
+const config = require('../lib/config');
 
 /*  render page routes
 ============================================================================= */
@@ -13,7 +14,7 @@ router.get('/queries/:_id', mustBeAuthenticatedOrChartLink, function(
   res,
   next
 ) {
-  const { config, query, params } = req;
+  const { query, params } = req;
   const { format } = query;
   if (format === 'table') {
     return res.redirect(config.get('baseUrl') + '/query-table/' + params._id);
diff --git a/server/routes/query-result.js b/server/routes/query-result.js
index 6570bca3..11f3ce3c 100644
--- a/server/routes/query-result.js
+++ b/server/routes/query-result.js
@@ -8,6 +8,7 @@ const Query = require('../models/Query.js');
 const mustBeAuthenticated = require('../middleware/must-be-authenticated.js');
 const mustBeAuthenticatedOrChartLink = require('../middleware/must-be-authenticated-or-chart-link-noauth.js');
 const sendError = require('../lib/sendError');
+const config = require('../lib/config');
 
 // This allows executing a query relying on the saved query text
 // Instead of relying on an open endpoint that executes arbitrary sql
@@ -45,7 +46,6 @@ router.get(
 router.post('/api/query-result', mustBeAuthenticated, async function(req, res) {
   const data = {
     cacheKey: req.body.cacheKey,
-    config: req.config,
     connectionId: req.body.connectionId,
     queryName: req.body.queryName,
     queryText: req.body.queryText,
@@ -61,7 +61,7 @@ router.post('/api/query-result', mustBeAuthenticated, async function(req, res) {
 });
 
 async function getQueryResult(data) {
-  const { connectionId, config, cacheKey, queryName, queryText, user } = data;
+  const { connectionId, cacheKey, queryName, queryText, user } = data;
   const connection = await connections.findOneById(connectionId);
 
   if (!connection) {
diff --git a/server/routes/signup-signin-signout.js b/server/routes/signup-signin-signout.js
index 9f7a9669..d5524ada 100644
--- a/server/routes/signup-signin-signout.js
+++ b/server/routes/signup-signin-signout.js
@@ -3,14 +3,11 @@ const router = require('express').Router();
 const checkWhitelist = require('../lib/check-whitelist');
 const User = require('../models/User.js');
 const sendError = require('../lib/sendError');
-
-// NOTE: getting config here during module init is okay
-// since these configs are set via env or cli
-const { disableUserpassAuth } = require('../lib/config').getPreDbConfig();
+const config = require('../lib/config');
 
 async function handleSignup(req, res, next) {
   try {
-    const whitelistedDomains = req.config.get('whitelistedDomains');
+    const whitelistedDomains = config.get('whitelistedDomains');
 
     if (req.body.password !== req.body.passwordConfirmation) {
       return sendError(res, null, 'Passwords do not match');
@@ -58,7 +55,7 @@ function sendSuccess(req, res) {
 
 /*  Some routes should only exist if userpath auth is enabled
 ============================================================================= */
-if (!disableUserpassAuth) {
+if (!config.get('disableUserpassAuth')) {
   router.post(
     '/api/signup',
     handleSignup,
diff --git a/server/routes/users.js b/server/routes/users.js
index 2db065d8..82a586fd 100644
--- a/server/routes/users.js
+++ b/server/routes/users.js
@@ -4,6 +4,7 @@ const email = require('../lib/email');
 const mustBeAdmin = require('../middleware/must-be-admin.js');
 const mustBeAuthenticated = require('../middleware/must-be-authenticated.js');
 const sendError = require('../lib/sendError');
+const config = require('../lib/config');
 
 router.get('/api/users', mustBeAuthenticated, async function(req, res) {
   try {
@@ -16,7 +17,6 @@ router.get('/api/users', mustBeAuthenticated, async function(req, res) {
 
 // create/whitelist/invite user
 router.post('/api/users', mustBeAdmin, async function(req, res) {
-  const { config } = req;
   try {
     let user = await User.findOneByEmail(req.body.email);
     if (user) {
diff --git a/server/server.js b/server/server.js
index 0149ed82..4c129d9a 100755
--- a/server/server.js
+++ b/server/server.js
@@ -8,17 +8,19 @@ const detectPort = require('detect-port');
 // Parse command line flags to see if anything special needs to happen
 require('./lib/cli-flow.js');
 
+const config = require('./lib/config');
+
 const app = require('./app');
-const {
-  baseUrl,
-  ip,
-  port,
-  httpsPort,
-  certPassphrase,
-  keyPath,
-  certPath,
-  systemdSocket
-} = require('./lib/config').getPreDbConfig();
+
+const baseUrl = config.get('baseUrl');
+const ip = config.get('ip');
+const port = config.get('port');
+const httpsPort = config.get('port');
+const certPassphrase = config.get('certPassphrase');
+const keyPath = config.get('keyPath');
+const certPath = config.get('certPath');
+const systemdSocket = config.get('systemdSocket');
+
 const db = require('./lib/db');
 
 function isFdObject(ob) {


Test patch:
diff --git a/server/test/lib/config.js b/server/test/lib/config.js
index 03fac1ff..075e7c99 100644
--- a/server/test/lib/config.js
+++ b/server/test/lib/config.js
@@ -1,11 +1,9 @@
 const assert = require('assert');
-const configUtil = require('../../lib/config');
+const config = require('../../lib/config');
 
-const configItems = require('../../lib/config/configItems');
 const fromDefault = require('../../lib/config/fromDefault');
 const fromEnv = require('../../lib/config/fromEnv');
 const fromCli = require('../../lib/config/fromCli');
-const nonUiConfig = require('../../lib/config').getPreDbConfig();
 
 describe('config', function() {
   it('default', function() {
@@ -29,10 +27,6 @@ describe('config', function() {
     assert.equal(conf.certPath, 'cert/path', 'certPath');
     assert.equal(conf.admin, 'admin@email.com', 'admin');
   });
-
-  it('nonUI', function() {
-    assert.equal(Object.keys(nonUiConfig).length, configItems.length);
-  });
 });
 
 describe('lib/config', function() {
@@ -46,11 +40,9 @@ describe('lib/config', function() {
   // This makes testing difficult when a sqlpad configuration is saved.
   // Loading a config should likely be explicit
   it('.get() should get a value provided by default', async function() {
-    const config = configUtil.getHelper();
     assert.equal(config.get('httpsPort'), 443, 'httpsPort=443');
   });
   it('.get() should only accept key in config items', async function() {
-    const config = configUtil.getHelper();
     assert.throws(() => config.get('non-existent-key'), Error);
   });
 });
diff --git a/server/test/lib/email.js b/server/test/lib/email.js
index 731e8249..b82b1871 100644
--- a/server/test/lib/email.js
+++ b/server/test/lib/email.js
@@ -1,8 +1,7 @@
 const email = require('../../lib/email.js');
-const configUtil = require('../../lib/config');
+const config = require('../../lib/config');
 
 describe('lib/email.js', async function() {
-  const config = configUtil.getHelper();
   if (config.smtpConfigured() && process.env.SQLPAD_TEST_EMAIL) {
     it('should send invites', function() {
       return email.sendInvite(process.env.SQLPAD_TEST_EMAIL);