From 4024cab7cd9cb6d4f76d067779f7baa7d9ce0f80 Mon Sep 17 00:00:00 2001 From: Pablo Palacios Date: Fri, 22 Nov 2024 19:41:40 +0100 Subject: [PATCH] fix: prevent fetchr to throw a 500 whenever a resource does not implement an operation (#546) --- libs/fetcher.js | 4 +- tests/functional/app.js | 4 +- tests/functional/fetchr.test.js | 28 ++++ tests/unit/libs/fetcher.client.js | 153 +++++++++++++++------- tests/unit/libs/fetcher.js | 12 +- tests/util/testCrud.js | 204 +++++++++++++++++------------- 6 files changed, 271 insertions(+), 134 deletions(-) diff --git a/libs/fetcher.js b/libs/fetcher.js index cb8b37d..9072e9f 100644 --- a/libs/fetcher.js +++ b/libs/fetcher.js @@ -252,8 +252,10 @@ class Request { if (!handler) { const err = new FetchrError( - `operation: ${this.operation} is undefined on service: ${this.resource}`, + `Operation "${this.operation}" is not allowed for resource "${sanitizeResourceName(this.resource)}"`, ); + err.statusCode = 405; + return { err }; } diff --git a/tests/functional/app.js b/tests/functional/app.js index 755ada3..b8a0dab 100644 --- a/tests/functional/app.js +++ b/tests/functional/app.js @@ -19,7 +19,9 @@ app.use(express.json()); app.use('/static', express.static(path.join(__dirname, 'static'))); -app.use('/api', Fetchr.middleware()); +app.use('/api', Fetchr.middleware(), (err, req, res, next) => { + res.status(err.statusCode || 500).json({ message: err.message }); +}); app.get('/', (req, res) => { res.send(` diff --git a/tests/functional/fetchr.test.js b/tests/functional/fetchr.test.js index 8e99625..0e611db 100644 --- a/tests/functional/fetchr.test.js +++ b/tests/functional/fetchr.test.js @@ -595,6 +595,34 @@ describe('client/server integration', () => { }); }); }); + + describe('client errors', () => { + it('handles unknown resources', async () => { + const response = await page.evaluate(() => { + const fetcher = new Fetchr(); + return fetcher + .create('unknown-resource') + .catch((err) => err); + }); + + expect(response.statusCode).to.equal(400); + expect(response.message).to.equal( + 'Resource "unknown*resource" is not registered', + ); + }); + + it('handles not implemented operations', async () => { + const response = await page.evaluate(() => { + const fetcher = new Fetchr(); + return fetcher.delete('error').catch((err) => err); + }); + + expect(response.statusCode).to.equal(405); + expect(JSON.parse(response.message).output.message).to.equal( + 'Operation "delete" is not allowed for resource "error"', + ); + }); + }); }); describe('headers', () => { diff --git a/tests/unit/libs/fetcher.client.js b/tests/unit/libs/fetcher.client.js index faaa953..1c20fd1 100644 --- a/tests/unit/libs/fetcher.client.js +++ b/tests/unit/libs/fetcher.client.js @@ -121,10 +121,20 @@ describe('Client Fetcher', function () { } }; }); + beforeEach(function () { stats = null; }); - testCrud(params, body, config, callbackWithStats, resolve, reject); + + testCrud({ + params, + body, + config, + callback: callbackWithStats, + reject, + resolve, + }); + after(function () { validateRequest = null; }); @@ -147,15 +157,17 @@ describe('Client Fetcher', function () { corsPath: corsPath, }); }); + testCrud({ - params: params, - body: body, + params, + body, config: { cors: true }, + callback, + reject, + resolve, disableNoConfigTests: true, - callback: callback, - resolve: resolve, - reject: reject, }); + after(function () { validateRequest = null; }); @@ -222,7 +234,16 @@ describe('Client Fetcher', function () { xhrTimeout: 4000, }); }); - testCrud(params, body, config, callback, resolve, reject); + + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -245,17 +266,17 @@ describe('Client Fetcher', function () { xhrTimeout: 4000, }); }); + testCrud({ - params: params, - body: body, - config: { - timeout: 5000, - }, - disableNoConfigTests: true, + params, + body, + config: { timeout: 5000 }, callback: callback, resolve: resolve, reject: reject, + disableNoConfigTests: true, }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -277,7 +298,16 @@ describe('Client Fetcher', function () { context: context, }); }); - testCrud(params, body, config, callback, resolve, reject); + + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -325,7 +355,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); }); describe('Property Name', function () { @@ -338,7 +375,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); }); describe('Property Names', function () { @@ -351,7 +395,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); }); }); @@ -396,7 +447,16 @@ describe('Client Fetcher', function () { }, }); }); - testCrud(params, body, config, callback, resolve, reject); + + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -418,20 +478,17 @@ describe('Client Fetcher', function () { context: context, }); }); - var customConfig = { - headers: { - 'X-APP-VERSION': VERSION, - }, - }; + testCrud({ + params, + body, + config: { headers: { 'X-APP-VERSION': VERSION } }, + callback, + resolve, + reject, disableNoConfigTests: true, - params: params, - body: body, - config: customConfig, - callback: callback, - resolve: resolve, - reject: reject, }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); @@ -679,7 +736,14 @@ describe('Client Fetcher', function () { }); }); - testCrud(params, body, config, callback, resolve, reject); + testCrud({ + params, + body, + config, + callback, + resolve, + reject, + }); after(function () { mockery.deregisterMock('./util/httpRequest'); @@ -705,23 +769,24 @@ describe('Client Fetcher', function () { Fetcher = require('../../../libs/fetcher.client'); this.fetcher = new Fetcher({}); }); - var customConfig = { - retry: { - interval: 350, - maxRetries: 2, - statusCodes: [0, 502, 504], - }, - unsafeAllowRetry: true, - }; + testCrud({ + params, + body, + config: { + retry: { + interval: 350, + maxRetries: 2, + statusCodes: [0, 502, 504], + }, + unsafeAllowRetry: true, + }, + callback, + resolve, + reject, disableNoConfigTests: true, - params: params, - body: body, - config: customConfig, - callback: callback, - resolve: resolve, - reject: reject, }); + after(function () { mockery.deregisterMock('./util/httpRequest'); mockery.disable(); diff --git a/tests/unit/libs/fetcher.js b/tests/unit/libs/fetcher.js index cd36330..ac705a3 100644 --- a/tests/unit/libs/fetcher.js +++ b/tests/unit/libs/fetcher.js @@ -1293,8 +1293,16 @@ describe('Server Fetcher', function () { beforeEach(function () { stats = null; }); - // CRUD - testCrud(params, body, config, callbackWithStats, resolve, reject); + + testCrud({ + params, + body, + config, + callback: callbackWithStats, + resolve, + reject, + isServer: true, + }); }); }); diff --git a/tests/util/testCrud.js b/tests/util/testCrud.js index 66fefaa..2af2f64 100644 --- a/tests/util/testCrud.js +++ b/tests/util/testCrud.js @@ -1,61 +1,57 @@ -var expect = require('chai').expect; -var defaultOptions = require('./defaultOptions'); -var resource = defaultOptions.resource; -var invalidResource = 'invalid_resource'; -var mockErrorService = require('../mock/MockErrorService'); -var mockNoopService = require('../mock/MockNoopService'); - -module.exports = function testCrud( - params, - body, - config, - callback, - resolve, - reject, -) { - var options = {}; - if (arguments.length === 1) { - options = params; - params = options.params || defaultOptions.params; - body = options.body || defaultOptions.body; - config = options.config || defaultOptions.config; - callback = options.callback || defaultOptions.callback; - resolve = options.resolve || defaultOptions.resolve; - reject = options.reject || defaultOptions.reject; - } +const expect = require('chai').expect; +const defaultOptions = require('./defaultOptions'); +const resource = defaultOptions.resource; +const invalidResource = 'invalid_resource'; +const mockErrorService = require('../mock/MockErrorService'); +const mockNoopService = require('../mock/MockNoopService'); + +module.exports = function testCrud({ + params = defaultOptions.params, + body = defaultOptions.body, + config = defaultOptions.config, + callback = defaultOptions.callback, + resolve = defaultOptions.resolve, + reject = defaultOptions.reject, + disableNoConfigTests = false, + isServer = false, +}) { describe('CRUD Interface', function () { describe('should work superagent style', function () { describe('with callbacks', function () { it('should handle CREATE', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation](resource) .params(params) .body(body) .clientConfig(config) .end(callback(operation, done)); }); + it('should handle READ', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation](resource) .params(params) .clientConfig(config) .end(callback(operation, done)); }); + it('should handle UPDATE', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation](resource) .params(params) .body(body) .clientConfig(config) .end(callback(operation, done)); }); + it('should handle DELETE', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation](resource) .params(params) .clientConfig(config) .end(callback(operation, done)); }); + it('should throw if no resource is given', function () { expect(this.fetcher.read.bind(this.fetcher)).to.throw( 'Resource is required for a fetcher request', @@ -64,8 +60,22 @@ module.exports = function testCrud( }); describe('with Promises', function () { + function denySuccess(done) { + return function () { + done(new Error('This operation should have failed')); + }; + } + + function allowFailure(done) { + return function (err) { + expect(err.name).to.equal('FetchrError'); + expect(err.message).to.exist; + done(); + }; + } + it('should handle CREATE', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation](resource) .params(params) .body(body) @@ -75,8 +85,9 @@ module.exports = function testCrud( reject(operation, done), ); }); + it('should handle READ', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation](resource) .params(params) .clientConfig(config) @@ -85,8 +96,9 @@ module.exports = function testCrud( reject(operation, done), ); }); + it('should handle UPDATE', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation](resource) .params(params) .body(body) @@ -96,8 +108,9 @@ module.exports = function testCrud( reject(operation, done), ); }); + it('should handle DELETE', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation](resource) .params(params) .clientConfig(config) @@ -106,48 +119,41 @@ module.exports = function testCrud( reject(operation, done), ); }); - var denySuccess = function (done) { - return function () { - done(new Error('This operation should have failed')); - }; - }; - var allowFailure = function (done) { - return function (err) { - expect(err.name).to.equal('FetchrError'); - expect(err.message).to.exist; - done(); - }; - }; + it('should reject a CREATE promise on invalid resource', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation](invalidResource) .params(params) .body(body) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should reject a READ promise on invalid resource', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation](invalidResource) .params(params) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should reject a UPDATE promise on invalid resource', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation](invalidResource) .params(params) .body(body) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should reject a DELETE promise on invalid resource', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation](invalidResource) .params(params) .clientConfig(config) .then(denySuccess(done), allowFailure(done)); }); + it('should throw if no resource is given', function () { expect(this.fetcher.read.bind(this.fetcher)).to.throw( 'Resource is required for a fetcher request', @@ -155,10 +161,23 @@ module.exports = function testCrud( }); }); }); + describe('should be backwards compatible', function () { + function denySuccess(done) { + return function (err) { + if (!err) { + done(new Error('This operation should have failed')); + } else { + expect(err.name).to.equal('FetchrError'); + expect(err.message).to.exist; + done(); + } + }; + } + // with config it('should handle CREATE', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation]( resource, params, @@ -167,8 +186,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle READ', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation]( resource, params, @@ -176,8 +196,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle UPDATE', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation]( resource, params, @@ -186,8 +207,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle DELETE', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation]( resource, params, @@ -195,19 +217,9 @@ module.exports = function testCrud( callback(operation, done), ); }); - var denySuccess = function (done) { - return function (err) { - if (!err) { - done(new Error('This operation should have failed')); - } else { - expect(err.name).to.equal('FetchrError'); - expect(err.message).to.exist; - done(); - } - }; - }; + it('should throw catchable error on CREATE with invalid resource', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation]( invalidResource, params, @@ -216,8 +228,9 @@ module.exports = function testCrud( denySuccess(done), ); }); + it('should throw catchable error on READ with invalid resource', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation]( invalidResource, params, @@ -225,8 +238,9 @@ module.exports = function testCrud( denySuccess(done), ); }); + it('should throw catchable error on UPDATE with invalid resource', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation]( invalidResource, params, @@ -235,8 +249,9 @@ module.exports = function testCrud( denySuccess(done), ); }); + it('should throw catchable error on DELETE with invalid resource', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation]( invalidResource, params, @@ -244,12 +259,13 @@ module.exports = function testCrud( denySuccess(done), ); }); - if (!options.disableNoConfigTests) { + + if (!disableNoConfigTests) { // without config // we have a feature flag to disable these tests because // it doesn't make sense to test a feature like CORS without being able to pass in a config it('should handle CREATE w/ no config', function (done) { - var operation = 'create'; + const operation = 'create'; this.fetcher[operation]( resource, params, @@ -257,16 +273,18 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle READ w/ no config', function (done) { - var operation = 'read'; + const operation = 'read'; this.fetcher[operation]( resource, params, callback(operation, done), ); }); + it('should handle UPDATE w/ no config', function (done) { - var operation = 'update'; + const operation = 'update'; this.fetcher[operation]( resource, params, @@ -274,8 +292,9 @@ module.exports = function testCrud( callback(operation, done), ); }); + it('should handle DELETE w/ no config', function (done) { - var operation = 'delete'; + const operation = 'delete'; this.fetcher[operation]( resource, params, @@ -284,8 +303,9 @@ module.exports = function testCrud( }); } }); + it('should keep track of metadata in getServiceMeta', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher._serviceMeta.length = 0; // reset serviceMeta to empty array fetcher .read(resource) @@ -315,7 +335,7 @@ module.exports = function testCrud( expect(meta).to.include.keys('headers'); expect(meta.headers).to.include.keys('x-bar'); expect(meta.headers['x-bar']).to.equal('bar'); - var serviceMeta = fetcher.getServiceMeta(); + const serviceMeta = fetcher.getServiceMeta(); expect(serviceMeta).to.have.length(2); expect(serviceMeta[0].headers).to.include.keys( 'x-foo', @@ -333,9 +353,10 @@ module.exports = function testCrud( }); }); }); + describe('should have serviceMeta data on error', function () { it('with callbacks', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher._serviceMeta.length = 0; // reset serviceMeta to empty array fetcher .read(mockErrorService.resource) @@ -346,7 +367,7 @@ module.exports = function testCrud( .clientConfig(config) .end(function (err) { if (err) { - var serviceMeta = fetcher.getServiceMeta(); + const serviceMeta = fetcher.getServiceMeta(); expect(serviceMeta).to.have.length(1); expect(serviceMeta[0]).to.include.keys('headers'); expect(serviceMeta[0].headers).to.include.keys( @@ -359,8 +380,9 @@ module.exports = function testCrud( } }); }); + it('with Promises', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher._serviceMeta.length = 0; // reset serviceMeta to empty array fetcher .read(mockErrorService.resource) @@ -371,7 +393,7 @@ module.exports = function testCrud( .clientConfig(config) .catch(function (err) { if (err) { - var serviceMeta = fetcher.getServiceMeta(); + const serviceMeta = fetcher.getServiceMeta(); expect(serviceMeta).to.have.length(1); expect(serviceMeta[0]).to.include.keys('headers'); expect(serviceMeta[0].headers).to.include.keys( @@ -386,29 +408,39 @@ module.exports = function testCrud( }); }); }); - describe('should reject no operation service', function () { + + describe('should reject when resource does not implement operation', function () { + function getErrorMessage(err) { + return isServer + ? err.message + : JSON.parse(err.message).output.message; + } + it('with callback', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher .read(mockNoopService.resource) .clientConfig(config) .end(function (err) { + const message = getErrorMessage(err); expect(err.name).to.equal('FetchrError'); - expect(err.message).to.contain( - 'operation: read is undefined on service: mock_noop_service', + expect(message).to.equal( + 'Operation "read" is not allowed for resource "mock_noop_service"', ); done(); }); }); + it('with Promise', function (done) { - var fetcher = this.fetcher; + const fetcher = this.fetcher; fetcher .read(mockNoopService.resource) .clientConfig(config) .catch(function (err) { + const message = getErrorMessage(err); expect(err.name).to.equal('FetchrError'); - expect(err.message).to.contain( - 'operation: read is undefined on service: mock_noop_service', + expect(message).to.equal( + 'Operation "read" is not allowed for resource "mock_noop_service"', ); done(); });