From 7d8d05b865ab7a7efbc81c9aa4021f2b32c14928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 16:11:39 +0200 Subject: [PATCH] Log errors and do not send 'X-Tiler-Errors' header --- lib/api/middlewares/error-middleware.js | 62 +-------- test/acceptance/error-middleware-test.js | 44 ------ test/unit/error-middleware-test.js | 167 ----------------------- 3 files changed, 6 insertions(+), 267 deletions(-) delete mode 100644 test/acceptance/error-middleware-test.js diff --git a/lib/api/middlewares/error-middleware.js b/lib/api/middlewares/error-middleware.js index 05d1bd49..0822c144 100644 --- a/lib/api/middlewares/error-middleware.js +++ b/lib/api/middlewares/error-middleware.js @@ -1,10 +1,10 @@ 'use strict'; -const _ = require('underscore'); const debug = require('debug')('windshaft:cartodb:error-middleware'); module.exports = function errorMiddleware (/* options */) { return function error (err, req, res, next) { + const { logger } = res.locals; var allErrors = Array.isArray(err) ? err : [err]; allErrors = populateLimitErrors(allErrors); @@ -15,14 +15,14 @@ module.exports = function errorMiddleware (/* options */) { var statusCode = findStatusCode(err); - setErrorHeader(allErrors, statusCode, res); - debug('[%s ERROR] -- %d: %s, %s', label, statusCode, err, err.stack); - // If a callback was requested, force status to 200 if (req.query && req.query.callback) { statusCode = 200; } + allErrors.forEach((err) => debug('[%s ERROR] -- %d: %s, %s', label, statusCode, err, err.stack)); + allErrors.forEach((err) => logger.error(err)); + var errorResponseBody = { errors: allErrors.map(errorMessage), errors_with_context: allErrors.map(errorMessageWithContext) @@ -135,7 +135,7 @@ function statusFromErrorMessage (errMsg) { function errorMessage (err) { // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 - var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + var message = (typeof err === 'string' ? err : err.message) || 'Unknown error'; return stripConnectionInfo(message); } @@ -165,7 +165,7 @@ function shouldBeExposed (prop) { function errorMessageWithContext (err) { // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 - var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + var message = (typeof err === 'string' ? err : err.message) || 'Unknown error'; var error = { type: err.type || 'unknown', @@ -181,53 +181,3 @@ function errorMessageWithContext (err) { return error; } - -function setErrorHeader (errors, statusCode, res) { - const errorsCopy = errors.slice(0); - const mainError = errorsCopy.shift(); - - const errorsLog = { - mainError: { - statusCode: statusCode || 200, - message: mainError.message, - name: mainError.name, - label: mainError.label, - type: mainError.type, - subtype: mainError.subtype - } - }; - - errorsLog.moreErrors = errorsCopy.map(error => { - return { - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }; - }); - - res.set('X-Tiler-Errors', stringifyForLogs(errorsLog)); -} - -/** - * Remove problematic nested characters - * from object for logs RegEx - * - * @param {Object} object - */ -function stringifyForLogs (object) { - Object.keys(object).map(key => { - if (typeof object[key] === 'string') { - object[key] = object[key].replace(/[^a-zA-Z0-9]/g, ' '); - } else if (typeof object[key] === 'object') { - stringifyForLogs(object[key]); - } else if (object[key] instanceof Array) { - for (const element of object[key]) { - stringifyForLogs(element); - } - } - }); - - return JSON.stringify(object); -} diff --git a/test/acceptance/error-middleware-test.js b/test/acceptance/error-middleware-test.js deleted file mode 100644 index 936c957f..00000000 --- a/test/acceptance/error-middleware-test.js +++ /dev/null @@ -1,44 +0,0 @@ -'use strict'; - -const assert = require('../support/assert'); -const TestClient = require('../support/test-client'); - -describe('error middleware', function () { - it('should returns a errors header', function (done) { - const mapConfig = { - version: '1.6.0', - layers: [{ - type: 'mapnik', - options: {} - }] - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: 'Missing cartocss for layer 0 options', - name: 'Error', - label: 'ANONYMOUS LAYERGROUP', - type: 'layer' - }, - moreErrors: [] - }; - - this.testClient = new TestClient(mapConfig, 1234); - - const params = { - response: { - status: 400, - headers: { - 'Content-Type': 'application/json; charset=utf-8', - 'X-Tiler-Errors': JSON.stringify(errorHeader) - } - } - }; - - this.testClient.getLayergroup(params, (err) => { - assert.ifError(err); - done(); - }); - }); -}); diff --git a/test/unit/error-middleware-test.js b/test/unit/error-middleware-test.js index b973d49a..8b845de5 100644 --- a/test/unit/error-middleware-test.js +++ b/test/unit/error-middleware-test.js @@ -20,171 +20,4 @@ describe('error-middleware', function () { 'Error status code for multiline/PSQL does not match' ); }); - - it('should return a header with errors', function (done) { - const error = new Error('error test'); - error.label = 'test label'; - error.type = 'test type'; - error.subtype = 'test subtype'; - - const errors = [error, error]; - - const req = {}; - const res = { - headers: {}, - set (key, value) { - this.headers[key] = value; - }, - statusCode: 0, - status (status) { - this.statusCode = status; - }, - json () {}, - send () {} - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }, - moreErrors: [{ - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }] - }; - - const errorFn = errorMiddleware(); - errorFn(errors, req, res, (err) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(res.headers, { - 'X-Tiler-Errors': JSON.stringify(errorHeader) - }); - - return done(); - }); - }); - - it('JSONP should return a header with error status code', function (done) { - const error = new Error('error test'); - error.label = 'test label'; - error.type = 'test type'; - error.subtype = 'test subtype'; - - const errors = [error, error]; - - const req = { - query: { callback: true } - }; - const res = { - headers: {}, - set (key, value) { - this.headers[key] = value; - }, - statusCode: 0, - status (status) { - this.statusCode = status; - }, - jsonp () {}, - send () {} - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }, - moreErrors: [{ - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }] - }; - - const errorFn = errorMiddleware(); - errorFn(errors, req, res, (err) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(res.headers, { - 'X-Tiler-Errors': JSON.stringify(errorHeader) - }); - - return done(); - }); - }); - - it('should escape chars that broke logs regex', function (done) { - const badString = 'error: ( ) = " \" \' * $ & |'; // eslint-disable-line no-useless-escape - const escapedString = 'error '; - - const error = new Error(badString); - error.label = badString; - error.type = badString; - error.subtype = badString; - - const errors = [error, error]; - - const req = {}; - const res = { - headers: {}, - set (key, value) { - this.headers[key] = value; - }, - statusCode: 0, - status (status) { - this.statusCode = status; - }, - json () {}, - send () {} - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: escapedString, - name: error.name, - label: escapedString, - type: escapedString, - subtype: escapedString - }, - moreErrors: [{ - message: escapedString, - name: error.name, - label: escapedString, - type: escapedString, - subtype: escapedString - }] - }; - - const errorFn = errorMiddleware(); - errorFn(errors, req, res, (err) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(res.headers, { - 'X-Tiler-Errors': JSON.stringify(errorHeader) - }); - - return done(); - }); - }); });