Refactor logging to provide common logger from LoggerAdapter (#2478)

* Refactor logging to provide common logger from LoggerAdapter

Move logger logic de WinstonLoggerAdapter

Further improvements in configuration

Use logger instead of getLogger

- Removes PLog module

Reverts name changes

nits

* Adds additional logging levels as requirements

* Adds tests for logging configuration

* removes flaky test

* investigate...

* further investigation

* Adds silent option to disable console output

* Restores logs with VERBOSE in tests

* Expose controller instead of adapter, reduces method requirements for adapter

* Shuffles initializations around

* Fix doc

* Load cloudCode last to make sure the logger is available

* Adds test to make sure we can load an adapter from npm module

* extract defaults

* Adds defaultMongoURI to defaults

* fix defaults values

* Proper error for PG failures

* Disable flaky test

Author: flovilmart

spec/AdapterLoader.spec.js

@@ -45,6 +45,19 @@ describe("AdapterLoader", ()=>{
     done();
   });
 
+  it("should instantiate an adapter from npm module", (done) => {
+    var adapter = loadAdapter({
+      module: 'parse-server-fs-adapter'
+    });
+
+    expect(typeof adapter).toBe('object');
+    expect(typeof adapter.createFile).toBe('function');
+    expect(typeof adapter.deleteFile).toBe('function');
+    expect(typeof adapter.getFileData).toBe('function');
+    expect(typeof adapter.getFileLocation).toBe('function');
+    done();
+  });
+
   it("should instantiate an adapter from function/Class", (done) => {
     var adapter = loadAdapter({
       adapter: FilesAdapter

spec/InstallationsRouter.spec.js

@@ -5,7 +5,7 @@ var InstallationsRouter = require('../src/Routers/InstallationsRouter').Installa
 
 var config = new Config('test');
 
-describe('InstallationsRouter', () => {
+describe_only_db(['mongo'])('InstallationsRouter', () => {
   it('uses find condition from request.body', (done) => {
     var androidDeviceRequest = {
       'installationId': '12345678-abcd-abcd-abcd-123456789abc',
@@ -71,6 +71,9 @@ describe('InstallationsRouter', () => {
       var results = res.response.results;
       expect(results.length).toEqual(1);
       done();
+    }).catch((err) => {
+      fail(JSON.stringify(err));
+      done();
     });
   });
 
@@ -172,6 +175,9 @@ describe('InstallationsRouter', () => {
       expect(response.results.length).toEqual(0);
       expect(response.count).toEqual(2);
       done();
+    }).catch((err) => {
+      fail(JSON.stringify(err));
+      done();
     });
   });
 });

spec/Logger.spec.js

@@ -1,4 +1,4 @@
-var logger = require('../src/logger');
+var logging = require('../src/Adapters/Logger/WinstonLogger');
 var winston = require('winston');
 
 class TestTransport extends winston.Transport {
@@ -9,10 +9,55 @@ class TestTransport extends winston.Transport {
 
 describe('Logger', () => {
   it('should add transport', () => {
-    const testTransport = new (TestTransport)({});
+    const testTransport = new (TestTransport)({
+      name: 'test'
+    });
     spyOn(testTransport, 'log');
-    logger.addTransport(testTransport);
-    logger.logger.info('hi');
+    logging.addTransport(testTransport);
+    expect(Object.keys(logging.logger.transports).length).toBe(4);
+    logging.logger.info('hi');
     expect(testTransport.log).toHaveBeenCalled();
+    logging.removeTransport(testTransport);
+    expect(Object.keys(logging.logger.transports).length).toBe(3);
+  });
+
+  it('should have files transports', (done) => {
+    reconfigureServer().then(() => {
+      let transports = logging.logger.transports;
+      let transportKeys = Object.keys(transports);
+      expect(transportKeys.length).toBe(3);
+      done();
+    });
+  });
+
+  it('should disable files logs', (done) => {
+    reconfigureServer({
+      logsFolder: null
+    }).then(() => {
+      let transports = logging.logger.transports;
+      let transportKeys = Object.keys(transports);
+      expect(transportKeys.length).toBe(1);
+      done();
+    });
+  });
+
+  it('should enable JSON logs', (done) => {
+    // Force console transport
+    reconfigureServer({
+      logsFolder: null,
+      jsonLogs: true,
+      silent: false
+    }).then(() => {
+      let spy = spyOn(process.stdout, 'write');
+      logging.logger.info('hi', {key: 'value'});
+      expect(process.stdout.write).toHaveBeenCalled();
+      var firstLog = process.stdout.write.calls.first().args[0];
+      expect(firstLog).toEqual(JSON.stringify({key: 'value', level: 'info', message: 'hi' })+'\n');
+      return reconfigureServer({
+        jsonLogs: false
+      });
+    }).then(() => {
+      done();
+    });
   });
 });

spec/Subscription.spec.js

@@ -1,10 +1,10 @@
 var Subscription = require('../src/LiveQuery/Subscription').Subscription;
-
+let logger;
 describe('Subscription', function() {
 
   beforeEach(function() {
-    var mockError = jasmine.createSpy('error');
-    jasmine.mockLibrary('../src/LiveQuery/PLog', 'error', mockError);
+    logger = require('../src/logger').logger;
+    spyOn(logger, 'error').and.callThrough();
   });
 
   it('can be initialized', function() {
@@ -62,17 +62,15 @@ describe('Subscription', function() {
     var subscription = new Subscription('className', { key : 'value' }, 'hash');
     subscription.deleteClientSubscription(1, 1);
 
-    var PLog =require('../src/LiveQuery/PLog');
-    expect(PLog.error).toHaveBeenCalled();
+    expect(logger.error).toHaveBeenCalled();
   });
 
   it('can delete nonexistent request for one client', function() {
     var subscription = new Subscription('className', { key : 'value' }, 'hash');
     subscription.addClientSubscription(1, 1);
     subscription.deleteClientSubscription(1, 2);
 
-    var PLog =require('../src/LiveQuery/PLog');
-    expect(PLog.error).toHaveBeenCalled();
+    expect(logger.error).toHaveBeenCalled();
     expect(subscription.clientRequestIds.size).toBe(1);
     expect(subscription.clientRequestIds.get(1)).toEqual([1]);
   });
@@ -83,8 +81,7 @@ describe('Subscription', function() {
     subscription.addClientSubscription(1, 2);
     subscription.deleteClientSubscription(1, 2);
 
-    var PLog =require('../src/LiveQuery/PLog');
-    expect(PLog.error).not.toHaveBeenCalled();
+    expect(logger.error).not.toHaveBeenCalled();
     expect(subscription.clientRequestIds.size).toBe(1);
     expect(subscription.clientRequestIds.get(1)).toEqual([1]);
   });
@@ -96,8 +93,7 @@ describe('Subscription', function() {
     subscription.deleteClientSubscription(1, 1);
     subscription.deleteClientSubscription(1, 2);
 
-    var PLog =require('../src/LiveQuery/PLog');
-    expect(PLog.error).not.toHaveBeenCalled();
+    expect(logger.error).not.toHaveBeenCalled();
     expect(subscription.clientRequestIds.size).toBe(0);
   });
 
@@ -111,13 +107,8 @@ describe('Subscription', function() {
     subscription.deleteClientSubscription(2, 1);
     subscription.deleteClientSubscription(2, 2);
 
-    var PLog =require('../src/LiveQuery/PLog');
-    expect(PLog.error).not.toHaveBeenCalled();
+    expect(logger.error).not.toHaveBeenCalled();
     expect(subscription.clientRequestIds.size).toBe(1);
     expect(subscription.clientRequestIds.get(1)).toEqual([1]);
   });
-
-  afterEach(function(){
-    jasmine.restoreLibrary('../src/LiveQuery/PLog', 'error');
-  });
 });

spec/WinstonLoggerAdapter.spec.js

@@ -8,7 +8,7 @@ describe('info logs', () => {
 
   it("Verify INFO logs", (done) => {
       var winstonLoggerAdapter = new WinstonLoggerAdapter();
-      winstonLoggerAdapter.info('testing info logs', () => {
+      winstonLoggerAdapter.log('info', 'testing info logs', () => {
         winstonLoggerAdapter.query({
           from: new Date(Date.now() - 500),
           size: 100,
@@ -29,7 +29,7 @@ describe('info logs', () => {
 describe('error logs', () => {
   it("Verify ERROR logs", (done) => {
     var winstonLoggerAdapter = new WinstonLoggerAdapter();
-    winstonLoggerAdapter.error('testing error logs', () => {
+    winstonLoggerAdapter.log('error', 'testing error logs', () => {
       winstonLoggerAdapter.query({
         from: new Date(Date.now() - 500),
         size: 100,

spec/helper.js

@@ -45,6 +45,7 @@ var defaultConfiguration = {
   webhookKey: 'hook',
   masterKey: 'test',
   fileKey: 'test',
+  silent: !process.env.VERBOSE,
   push: {
     'ios': {
       cert: 'prodCert.pem',
@@ -352,8 +353,6 @@ global.describe_only_db = db => {
   }
 }
 
-// LiveQuery test setting
-require('../src/LiveQuery/PLog').logLevel = 'NONE';
 var libraryCache = {};
 jasmine.mockLibrary = function(library, name, mock) {
   var original = require(library)[name];

src/Adapters/Files/GridStoreAdapter.js

@@ -8,14 +8,13 @@
 
 import { MongoClient, GridStore, Db} from 'mongodb';
 import { FilesAdapter }              from './FilesAdapter';
-
-const DefaultMongoURI = 'mongodb://localhost:27017/parse';
+import defaults                      from '../../defaults';
 
 export class GridStoreAdapter extends FilesAdapter {
   _databaseURI: string;
   _connectionPromise: Promise<Db>;
 
-  constructor(mongoDatabaseURI = DefaultMongoURI) {
+  constructor(mongoDatabaseURI = defaults.DefaultMongoURI) {
     super();
     this._databaseURI = mongoDatabaseURI;
     this._connect();

src/Adapters/Logger/LoggerAdapter.js

@@ -3,15 +3,13 @@
 // Allows you to change the logger mechanism
 //
 // Adapter classes must implement the following functions:
-// * info(obj1 [, obj2, .., objN])
-// * error(obj1 [, obj2, .., objN])
-// * query(options, callback)
+// * log() {}
+// * query(options, callback) /* optional */
 // Default is WinstonLoggerAdapter.js
 
 export class LoggerAdapter {
-  info() {}
-  error() {}
-  query(options, callback) {}
+  constructor(options) {}
+  log(level, message, /* meta */) {}
 }
 
 export default LoggerAdapter;

src/Adapters/Logger/WinstonLogger.js

@@ -0,0 +1,100 @@
+import winston from 'winston';
+import fs from 'fs';
+import path from 'path';
+import DailyRotateFile from 'winston-daily-rotate-file';
+import _ from 'lodash';
+import defaults  from '../../defaults';
+
+const logger = new winston.Logger();
+const additionalTransports = [];
+
+function updateTransports(options) {
+  let transports = Object.assign({}, logger.transports);
+  if (options) {
+    let silent = options.silent;
+    delete options.silent;
+    if (_.isNull(options.dirname)) {
+      delete transports['parse-server'];
+      delete transports['parse-server-error'];
+    } else if (!_.isUndefined(options.dirname)) {
+      transports['parse-server'] = new (DailyRotateFile)(
+        Object.assign({
+          filename: 'parse-server.info',
+          name: 'parse-server',
+        }, options));
+      transports['parse-server-error'] = new (DailyRotateFile)(
+        Object.assign({
+          filename: 'parse-server.err',
+          name: 'parse-server-error',
+          level: 'error'
+        }, options));
+    }
+
+    transports.console = new (winston.transports.Console)(
+      Object.assign({
+        colorize: true,
+        name: 'console',
+        silent
+      }, options));
+  }
+  // Mount the additional transports
+  additionalTransports.forEach((transport) => {
+    transports[transport.name] = transport;
+  });
+  logger.configure({
+    transports: _.values(transports)
+  });
+}
+
+export function configureLogger({
+  logsFolder = defaults.logsFolder,
+  jsonLogs = defaults.jsonLogs,
+  logLevel = winston.level,
+  verbose = defaults.verbose,
+  silent = defaults.silent } = {}) {
+
+  if (verbose) {
+    logLevel = 'verbose';
+  }
+
+  winston.level = logLevel;
+  const options = {};
+
+  if (logsFolder) {
+    if (!path.isAbsolute(logsFolder)) {
+      logsFolder = path.resolve(process.cwd(), logsFolder);
+    }
+    try {
+      fs.mkdirSync(logsFolder);
+    } catch (exception) {}
+  }
+  options.dirname = logsFolder;
+  options.level = logLevel;
+  options.silent = silent;
+
+  if (jsonLogs) {
+    options.json = true;
+    options.stringify = true;
+  }
+  updateTransports(options);
+}
+
+export function addTransport(transport) {
+  additionalTransports.push(transport);
+  updateTransports();
+}
+
+export function removeTransport(transport) {
+  let transportName = typeof transport == 'string' ? transport : transport.name;
+  let transports = Object.assign({}, logger.transports);
+  delete transports[transportName];
+  logger.configure({
+    transports: _.values(transports)
+  });
+  _.remove(additionalTransports, (transport) => {
+    return transport.name === transportName;
+  });
+}
+
+export { logger, addTransport, configureLogger, removeTransport };
+export default logger;