Repository: rickbergfalk/sqlpad
Pull request #551 (2020-02-24 19:20:23)

Config class and debug config values
Converts config to a class and adds connectionsFromConfig as a method. Adds additional debug logging at server startup to log config values and connections derived from configuration.

Also fixes incorrect log message about port not being available due to strict data type checks. 


Patch:
diff --git a/server/config.dev.ini b/server/config.dev.ini
index 89e37b73..ebcca7f1 100644
--- a/server/config.dev.ini
+++ b/server/config.dev.ini
@@ -1,4 +1,10 @@
-; INI config file that defines some connections for development purposes
+; INI config file for dev setup
+port = 3010
+baseUrl = "/sqlpad"
+debug = true
+dbPath = ../db
+appLogLevel = debug
+webLogLevel = debug
 
 [connections.devdbdriverid123]
 driver = mock
diff --git a/server/lib/config/index.js b/server/lib/config/index.js
index e43c670e..a1d18440 100644
--- a/server/lib/config/index.js
+++ b/server/lib/config/index.js
@@ -1,59 +1,157 @@
-const minimist = require('minimist');
+const _ = require('lodash');
+const appLog = require('../appLog');
+const drivers = require('../../drivers');
 const fromDefault = require('./fromDefault');
 const fromEnv = require('./fromEnv');
 const fromCli = require('./fromCli');
 const fromFile = require('./fromFile');
 const getOldConfigWarning = require('./getOldConfigWarning');
 
-const argv = minimist(process.argv.slice(2));
+class Config {
+  constructor(argv) {
+    this.argv = argv;
 
-const defaultConfig = fromDefault();
-const envConfig = fromEnv();
-const [fileConfig, warnings] = fromFile();
-const cliConfig = fromCli(argv);
+    const defaultConfig = fromDefault();
+    const envConfig = fromEnv();
+    const [fileConfig, warnings] = fromFile();
+    const cliConfig = fromCli(argv);
 
-const all = { ...defaultConfig, ...envConfig, ...fileConfig, ...cliConfig };
+    const all = { ...defaultConfig, ...envConfig, ...fileConfig, ...cliConfig };
 
-// Clean string boolean values
-Object.keys(all).forEach(key => {
-  const value = all[key];
-  if (typeof value === 'string') {
-    if (value.trim().toLowerCase() === 'true') {
-      all[key] = true;
-    } else if (value.trim().toLowerCase() === 'false') {
-      all[key] = false;
+    // Clean string boolean values
+    Object.keys(all).forEach(key => {
+      const value = all[key];
+      if (typeof value === 'string') {
+        if (value.trim().toLowerCase() === 'true') {
+          all[key] = true;
+        } else if (value.trim().toLowerCase() === 'false') {
+          all[key] = false;
+        }
+      }
+    });
+
+    this.fileConfig = fileConfig;
+    this.warnings = warnings;
+    this.all = all;
+  }
+
+  get(key) {
+    const { connections, ...rest } = this.all;
+    if (!key) {
+      return rest;
     }
+
+    if (!this.all.hasOwnProperty(key)) {
+      throw new Error(`config item ${key} not defined in configItems.js`);
+    }
+
+    return this.all[key];
   }
-});
 
-exports.get = function get(key) {
-  if (!key) {
-    throw new Error('key must be provided');
+  getValidations() {
+    const errors = [];
+
+    // By default dbPath will exist as empty string, which is not valid
+    if (this.all.dbPath === '') {
+      errors.push(getOldConfigWarning());
+    }
+
+    return {
+      errors,
+      warnings: [...this.warnings]
+    };
+  }
+
+  smtpConfigured() {
+    return (
+      this.all.smtpHost &&
+      this.all.smtpUser &&
+      this.all.smtpFrom &&
+      this.all.smtpPort &&
+      this.all.publicUrl
+    );
   }
 
-  if (!all.hasOwnProperty(key)) {
-    throw new Error(`config item ${key} not defined in configItems.js`);
+  googleAuthConfigured() {
+    return (
+      this.all.publicUrl &&
+      this.all.googleClientId &&
+      this.all.googleClientSecret
+    );
   }
 
-  return all[key];
-};
+  /**
+   * Get connections from config.
+   * These are provided at runtime and not upserted
+   * This allows supporting cases where connections can be defined then later removed via config changes alone
+   *
+   * For environment variables:
+   * connection env vars must follow the format:
+   * SQLPAD_CONNECTIONS__<connectionId>__<connectionFieldName>
+   *
+   * <connectionId> can be any value to associate a grouping a fields to a connection instance
+   * If supplying a connection that was previously defined in the nedb database,
+   * this would map internally to connection._id object.
+   *
+   * <connectionFieldName> should be a field name identified in drivers.
+   *
+   * To define connections via envvars, `driver` field should be supplied.
+   * _id field is not required, as it is defined in second env var fragment.
+   *
+   * Example: SQLPAD_CONNECTIONS__ab123__sqlserverEncrypt=""
+   *
+   * From file, resulting parsed configuration from file is expected to follow format `connections.<id>.<fieldname>`
+   * {
+   *   connections: {
+   *     ab123: {
+   *       sqlserverEncrypt: true
+   *     }
+   *   }
+   * }
+   *
+   * @param {object} [env] - optional environment override for testing
+   * @returns {array<object>} arrayOfConnections
+   */
+  getConnections(env = process.env) {
+    // Create a map of connections from parsing environment variable
+    const connectionsMapFromEnv = Object.keys(env)
+      .filter(key => key.startsWith('SQLPAD_CONNECTIONS__'))
+      .reduce((connectionsMap, envVar) => {
+        // eslint-disable-next-line no-unused-vars
+        const [prefix, id, field] = envVar.split('__');
+        if (!connectionsMap[id]) {
+          connectionsMap[id] = {};
+        }
+        connectionsMap[id][field] = env[envVar];
+        return connectionsMap;
+      }, {});
 
-exports.getValidations = () => {
-  const errors = [];
+    // Get copy of connections from config file
+    const { connections } = _.cloneDeep(this.fileConfig);
 
-  // By default dbPath will exist as empty string, which is not valid
-  if (all.dbPath === '') {
-    errors.push(getOldConfigWarning());
-  }
+    // connections key from file matches format that is constructed from env
+    // merge the 2 together then create an array out of them
+    const connectionsMap = { ...connectionsMapFromEnv, ...connections };
 
-  return {
-    errors,
-    warnings: [...warnings]
-  };
-};
+    const connectionsFromConfig = [];
+    Object.keys(connectionsMap).forEach(id => {
+      try {
+        let connection = connectionsMap[id];
+        connection._id = id;
+        connection = drivers.validateConnection(connection);
+        connection.editable = false;
+        connectionsFromConfig.push(connection);
+      } catch (error) {
+        appLog.error(
+          error,
+          'Environment connection configuration failed for %s',
+          id
+        );
+      }
+    });
 
-exports.smtpConfigured = () =>
-  all.smtpHost && all.smtpUser && all.smtpFrom && all.smtpPort && all.publicUrl;
+    return connectionsFromConfig;
+  }
+}
 
-exports.googleAuthConfigured = () =>
-  all.publicUrl && all.googleClientId && all.googleClientSecret;
+module.exports = Config;
diff --git a/server/lib/connectionsFromConfig.js b/server/lib/connectionsFromConfig.js
deleted file mode 100644
index cd38751d..00000000
--- a/server/lib/connectionsFromConfig.js
+++ /dev/null
@@ -1,79 +0,0 @@
-const _ = require('lodash');
-const appLog = require('./appLog');
-const drivers = require('../drivers');
-const getConfigFromFile = require('./config/fromFile.js');
-const [configFromFile] = getConfigFromFile() || {};
-
-/**
- * Get connections from config.
- *
- * For environment variables:
- * connection env vars must follow the format:
- * SQLPAD_CONNECTIONS__<connectionId>__<connectionFieldName>
- *
- * <connectionId> can be any value to associate a grouping a fields to a connection instance
- * If supplying a connection that was previously defined in the nedb database,
- * this would map internally to connection._id object.
- *
- * <connectionFieldName> should be a field name identified in drivers.
- *
- * To define connections via envvars, `driver` field should be supplied.
- * _id field is not required, as it is defined in second env var fragment.
- *
- * Example: SQLPAD_CONNECTIONS__ab123__sqlserverEncrypt=""
- *
- * From file, resulting parsed configuration from file is expected to follow format `connections.<id>.<fieldname>`
- * {
- *   connections: {
- *     ab123: {
- *       sqlserverEncrypt: true
- *     }
- *   }
- * }
- *
- * @param {object} env
- * @returns {array<object>} arrayOfConnections
- */
-function getConnectionsFromConfig(env = process.env) {
-  // Create a map of connections from parsing environment variable
-  const connectionsMapFromEnv = Object.keys(env)
-    .filter(key => key.startsWith('SQLPAD_CONNECTIONS__'))
-    .reduce((connectionsMap, envVar) => {
-      // eslint-disable-next-line no-unused-vars
-      const [prefix, id, field] = envVar.split('__');
-      if (!connectionsMap[id]) {
-        connectionsMap[id] = {};
-      }
-      connectionsMap[id][field] = env[envVar];
-      return connectionsMap;
-    }, {});
-
-  // Get copy of connections from config file
-  const { connections } = _.cloneDeep(configFromFile);
-
-  // connections key from file matches format that is constructed from env
-  // merge the 2 together then create an array out of them
-  const connectionsMap = { ...connectionsMapFromEnv, ...connections };
-
-  const connectionsFromConfig = [];
-  Object.keys(connectionsMap).forEach(id => {
-    try {
-      let connection = connectionsMap[id];
-      connection._id = id;
-      connection = drivers.validateConnection(connection);
-      connection.editable = false;
-      connectionsFromConfig.push(connection);
-    } catch (error) {
-      appLog.error(
-        error,
-        'Environment connection configuration failed for %s',
-        id
-      );
-    }
-  });
-  return connectionsFromConfig;
-}
-
-module.exports = {
-  getConnectionsFromConfig
-};
diff --git a/server/models/connectionAccesses.js b/server/models/connectionAccesses.js
index 44efd360..487b8b6a 100644
--- a/server/models/connectionAccesses.js
+++ b/server/models/connectionAccesses.js
@@ -18,6 +18,10 @@ const schema = Joi.object({
 });
 
 class ConnectionAccesses {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
diff --git a/server/models/connections.js b/server/models/connections.js
index 906ca8ae..6b20d393 100644
--- a/server/models/connections.js
+++ b/server/models/connections.js
@@ -1,10 +1,12 @@
 const _ = require('lodash');
 const drivers = require('../drivers');
 const makeCipher = require('../lib/makeCipher');
-// TODO: during app init upsert these into db?
-const { getConnectionsFromConfig } = require('../lib/connectionsFromConfig');
 
 class Connections {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
@@ -30,7 +32,9 @@ class Connections {
       return this.decipherConnection(conn);
     });
 
-    const allConnections = connectionsFromDb.concat(getConnectionsFromConfig());
+    const allConnections = connectionsFromDb.concat(
+      this.config.getConnections()
+    );
     return _.sortBy(allConnections, c => c.name.toLowerCase());
   }
 
@@ -42,9 +46,9 @@ class Connections {
     }
 
     // If connection was not found in db try env
-    const connectionFromEnv = getConnectionsFromConfig().find(
-      connection => connection._id === id
-    );
+    const connectionFromEnv = this.config
+      .getConnections()
+      .find(connection => connection._id === id);
 
     return connectionFromEnv;
   }
diff --git a/server/models/queries.js b/server/models/queries.js
index 1a6823d6..3e7f41a6 100644
--- a/server/models/queries.js
+++ b/server/models/queries.js
@@ -44,6 +44,10 @@ const schema = Joi.object({
 });
 
 class Queries {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
diff --git a/server/models/queryHistory.js b/server/models/queryHistory.js
index 61bb8954..b7b68ba1 100644
--- a/server/models/queryHistory.js
+++ b/server/models/queryHistory.js
@@ -18,6 +18,10 @@ const schema = Joi.object({
 });
 
 class QueryHistory {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
diff --git a/server/models/resultCache.js b/server/models/resultCache.js
index 40b621d3..4181462c 100644
--- a/server/models/resultCache.js
+++ b/server/models/resultCache.js
@@ -18,6 +18,10 @@ BigInt.prototype.toJSON = function() {
 };
 
 class ResultCache {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
diff --git a/server/models/schemaInfo.js b/server/models/schemaInfo.js
index 13f96daa..3d1c0a95 100644
--- a/server/models/schemaInfo.js
+++ b/server/models/schemaInfo.js
@@ -3,6 +3,10 @@ function getCacheKey(connectionId) {
 }
 
 class SchemaInfo {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
diff --git a/server/models/users.js b/server/models/users.js
index 93d33db8..ae5f9cc9 100644
--- a/server/models/users.js
+++ b/server/models/users.js
@@ -27,6 +27,10 @@ const schema = Joi.object({
 });
 
 class Users {
+  /**
+   * @param {*} nedb
+   * @param {import('../lib/config')} config
+   */
   constructor(nedb, config) {
     this.nedb = nedb;
     this.config = config;
diff --git a/server/package.json b/server/package.json
index 74702969..b0149202 100644
--- a/server/package.json
+++ b/server/package.json
@@ -28,7 +28,7 @@
   },
   "scripts": {
     "prepublishOnly": "cd .. && ./scripts/build.sh",
-    "start": "node-dev server.js --dbPath ../db --port 3010 --debug --baseUrl /sqlpad --config './config.dev.ini' | pino-pretty",
+    "start": "node-dev server.js --config './config.dev.ini' | pino-pretty",
     "test": "rimraf ../dbtest && SQLPAD_DB_PATH='../dbtest' SQLPAD_DEBUG='true' SQLPAD_APP_LOG_LEVEL='silent' SQLPAD_WEB_LOG_LEVEL='silent' mocha test --timeout 10000 --recursive --exit",
     "fixlint": "eslint --fix '**/*.js'",
     "lint": "eslint '**/*.js'"
diff --git a/server/server.js b/server/server.js
index a5b05957..04246e6b 100755
--- a/server/server.js
+++ b/server/server.js
@@ -3,17 +3,22 @@
 const fs = require('fs');
 const http = require('http');
 const https = require('https');
+const minimist = require('minimist');
 const detectPort = require('detect-port');
 const makeApp = require('./app');
+const appLog = require('./lib/appLog');
+const Config = require('./lib/config');
+const { makeDb, getDb } = require('./lib/db');
 
 // Parse command line flags to see if anything special needs to happen
 require('./lib/cli-flow.js');
 
-const appLog = require('./lib/appLog');
-const config = require('./lib/config');
-const { makeDb, getDb } = require('./lib/db');
+const argv = minimist(process.argv.slice(2));
+const config = new Config(argv);
 
 appLog.setLevel(config.get('appLogLevel'));
+appLog.debug(config.get(), 'Final config values');
+appLog.debug(config.getConnections(), 'Connections from config');
 
 makeDb(config);
 
@@ -79,7 +84,7 @@ async function startServer(models) {
   if (keyPath && certPath) {
     // https only
     const _port = await detectPortOrSystemd(httpsPort);
-    if (!isFdObject(_port) && httpsPort !== _port) {
+    if (!isFdObject(_port) && parseInt(httpsPort, 10) !== parseInt(_port, 10)) {
       appLog.info(
         'Port %d already occupied. Using port %d instead.',
         httpsPort,
@@ -107,7 +112,7 @@ async function startServer(models) {
   } else {
     // http only
     const _port = await detectPortOrSystemd(port);
-    if (!isFdObject(_port) && port !== _port) {
+    if (!isFdObject(_port) && parseInt(port, 10) !== parseInt(_port, 10)) {
       appLog.info(
         'Port %d already occupied. Using port %d instead.',
         port,


Test patch:
diff --git a/server/test/lib/config.js b/server/test/lib/config.js
index 5df192c1..6d5c9532 100644
--- a/server/test/lib/config.js
+++ b/server/test/lib/config.js
@@ -1,5 +1,5 @@
 const assert = require('assert');
-const config = require('../../lib/config');
+const { config } = require('../utils');
 
 const fromDefault = require('../../lib/config/fromDefault');
 const fromEnv = require('../../lib/config/fromEnv');
diff --git a/server/test/lib/email.js b/server/test/lib/email.js
index 504feb09..df8d16c0 100644
--- a/server/test/lib/email.js
+++ b/server/test/lib/email.js
@@ -1,5 +1,5 @@
 const makeEmail = require('../../lib/email.js');
-const config = require('../../lib/config');
+const { config } = require('../utils');
 
 describe('lib/email.js', async function() {
   if (config.smtpConfigured() && process.env.SQLPAD_TEST_EMAIL) {
diff --git a/server/test/models/connections.js b/server/test/models/connections.js
index a7edc11e..35cb6fb3 100644
--- a/server/test/models/connections.js
+++ b/server/test/models/connections.js
@@ -1,8 +1,8 @@
 const assert = require('assert');
+const { config } = require('../utils');
 const { getDb } = require('../../lib/db');
-const { getConnectionsFromConfig } = require('../../lib/connectionsFromConfig');
 
-describe('getConnectionsFromConfig', function() {
+describe('config.getConnections', function() {
   let models;
 
   before(async function() {
@@ -11,12 +11,12 @@ describe('getConnectionsFromConfig', function() {
   });
 
   it('handles empty object', function() {
-    const cs = getConnectionsFromConfig({});
+    const cs = config.getConnections({});
     assert(Array.isArray(cs));
   });
 
   it('skips partial connections', function() {
-    const cs = getConnectionsFromConfig({
+    const cs = config.getConnections({
       SQLPAD_CONNECTIONS__abc__driver: 'postgres'
     });
     assert(Array.isArray(cs));
@@ -24,7 +24,7 @@ describe('getConnectionsFromConfig', function() {
   });
 
   it('parses connection properly', function() {
-    const cs = getConnectionsFromConfig({
+    const cs = config.getConnections({
       SQLPAD_CONNECTIONS__abc__driver: 'postgres',
       SQLPAD_CONNECTIONS__abc__name: 'env-postgres',
       SQLPAD_CONNECTIONS__abc__host: 'localhost',
diff --git a/server/test/utils.js b/server/test/utils.js
index 800026c4..38668b1f 100644
--- a/server/test/utils.js
+++ b/server/test/utils.js
@@ -1,11 +1,18 @@
 const assert = require('assert');
+const minimist = require('minimist');
 const request = require('supertest');
 const consts = require('../lib/consts');
-const config = require('../lib/config');
+const Config = require('../lib/config');
 const appLog = require('../lib/appLog');
 const { makeDb, getDb } = require('../lib/db');
 const makeApp = require('../app');
 
+const argv = minimist(process.argv.slice(2));
+
+// TODO - restructure test utils to allow injecting different configurations
+// config values can be supplied directly, parsing different sources can be tested separately
+const config = new Config(argv);
+
 makeDb(config);
 let app;
 
@@ -108,6 +115,7 @@ before(async function() {
 });
 
 module.exports = {
+  config,
   del,
   expectKeys,
   get,