From abbf1f724f4b45176d9c8cd46db1a94bef7c37f9 Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Thu, 17 Nov 2016 15:35:37 -0600 Subject: [PATCH] Verify the `sails.config.environment` and NODE_ENV settings as soon as they're available. If `sails.config.environment` is "production" and NODE_ENV is undefined, set NODE_ENV to production and log warning. If `sails.config.environment` is "production" and NODE_ENV is a string other than "production", fail to lift Sails with an error. --- lib/app/load.js | 42 ++++++++++++++++- test/integration/lift.lower.test.js | 72 +++++++++++++++++++++++------ 2 files changed, 98 insertions(+), 16 deletions(-) diff --git a/lib/app/load.js b/lib/app/load.js index b4f6eaac66..e44c368feb 100644 --- a/lib/app/load.js +++ b/lib/app/load.js @@ -1,6 +1,7 @@ var async = require('async'); var _ = require('@sailshq/lodash'); var util = require('util'); +var flaverr = require('flaverr'); var __Configuration = require('./configuration'); var __initializeHooks = require('./private/loadHooks'); @@ -49,8 +50,26 @@ module.exports = function(sails) { // and options that were passed in programmatically. config: [Configuration.load], + // Verify that the combination of Sails environment and NODE_ENV is valid + // as early as possible -- that is, as soon as we know for sure what the + // Sails environment is. + verifyEnv: ['config', function(results, cb) { + // If the userconfig hook is active, wait until it's finished to + // verify the environment, since it might be set in a config file. + if (_.isUndefined(sails.config.hooks) || (sails.config.hooks !== false && sails.config.hooks.userconfig !== false)) { + sails.on('hook:userconfig:loaded', verifyEnvironment); + } + // Otherwise verify it right now. The Sails environment will be + // whatever was set on the command line, or via the sails_environment + // env var, or defaulted to "development". + else { + verifyEnvironment(); + } + return cb(); + }], + // Load hooks into memory, with their middleware and routes - hooks: ['config', loadHooks], + hooks: ['verifyEnv', 'config', loadHooks], // Load actions from disk and config overrides controller: ['hooks', sails._controller.loadActionModules], @@ -164,6 +183,27 @@ module.exports = function(sails) { return cb(); } + function verifyEnvironment() { + // At this point, the Sails environment is set to its final value, + // whether it came from the command line or a config file. So we + // can now compare it to the NODE_ENV environment variable and + // act accordingly. This may involve changing NODE_ENV to "production", + // which we want to do as early as possible since dependencies might + // be relying on that value. + + // If the Sails environment is production, but NODE_ENV is undefined, + // log a warning and change NODE_ENV to "production". + if (sails.config.environment === 'production' && process.env.NODE_ENV !== 'production' ) { + if (_.isUndefined(process.env.NODE_ENV)) { + sails.log.debug('Detected Sails environment is "production", but NODE_ENV is `undefined`.'); + sails.log.debug('Automatically setting the NODE_ENV environment variable to "production".'); + process.env.NODE_ENV = 'production'; + } else { + throw flaverr('E_INVALID_NODE_ENV', new Error('When the Sails environment is set to "production", NODE_ENV must also be set to "production" (but it was set to "' + process.env.NODE_ENV + '" instead).')); + } + } + } + /** * Returns function which is fired when Sails is ready to go * diff --git a/test/integration/lift.lower.test.js b/test/integration/lift.lower.test.js index 83bfa0962d..e4d0ea0b23 100644 --- a/test/integration/lift.lower.test.js +++ b/test/integration/lift.lower.test.js @@ -217,14 +217,14 @@ describe('sails being lifted and lowered (e.g in a test framework)', function() }); - describe('with Sails environment set to `production`, and the Node environment not set to `production`', function() { + describe('with Sails environment set to `production`, and the Node environment is `undefined`', function() { var sailsApp; var originalNodeEnv; before(function() { originalNodeEnv = process.env.NODE_ENV; - process.env.NODE_ENV = 'development'; + delete process.env.NODE_ENV; }); after(function(done) { @@ -237,15 +237,15 @@ describe('sails being lifted and lowered (e.g in a test framework)', function() } }); - it('should log a warning', function(done) { + it('should change NODE_ENV to production and log a warning', function(done) { - var warns = []; + var debugs = []; var customLogger = { - level: 'warn', + level: 'debug', custom: { - log: console.log.bind(console), - debug: console.log.bind(console), - warn: function(msg) {warns.push(msg);} + log: function(){}, + warn: function(){}, + debug: function(msg) {debugs.push(msg);} }, colors: { warn: '' }, prefixTheme: 'abbreviated' @@ -262,19 +262,20 @@ describe('sails being lifted and lowered (e.g in a test framework)', function() }, function(err, _sailsApp) { if (err) { return done(err); } - sailsApp = _sailsApp; - // Assert that NODE_ENV is unchanged. - assert.equal('development', process.env.NODE_ENV); + // Assert that NODE_ENV is changed. + assert.equal(process.env.NODE_ENV, 'production'); // Assert that sails config is unchanged. assert.equal(sailsApp.config.environment, 'production'); - var foundWarning = false; - assert (_.any(warns, function(warn) { - return warn.indexOf('Detected Sails environment of `production`, but Node environment is `development`') > -1; - }), 'Did not log a warning about NODE_ENV not being set to production!'); + assert (_.any(debugs, function(debug) { + return debug.indexOf('Detected Sails environment is "production", but NODE_ENV is `undefined`.') > -1; + }), 'Did not log a warning about NODE_ENV being undefined while sails environment is `production`!'); + assert (_.any(debugs, function(debug) { + return debug.indexOf('Automatically setting the NODE_ENV environment variable to "production".') > -1; + }), 'Did not log a warning about NODE_ENV being set to `production`!'); return done(); @@ -284,6 +285,47 @@ describe('sails being lifted and lowered (e.g in a test framework)', function() }); + describe('with Sails environment set to `production`, and the Node environment is `development`', function() { + + var sailsApp; + var originalNodeEnv; + + before(function() { + originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = 'development'; + }); + + after(function(done) { + process.env.NODE_ENV = originalNodeEnv; + if (sailsApp) { + return sailsApp.lower(done); + } + else { + return done(); + } + }); + + it('should fail to lift sails', function(done) { + + // Load `app0` deep in the `'cenote'` + Sails().load({ + environment: 'production', + log: {level: 'silent'}, + globals: false, + hooks: { + grunt: false, + } + }, function(err, _sailsApp) { + if (!err) { return done(new Error('Sails should have failed to lift!')); } + assert.equal(err.code, 'E_INVALID_NODE_ENV'); + + return done(); + + });// + + }); + + }); });