From b2f17e45327c5bb66d2678c6b0dcd6bc8af44d6a Mon Sep 17 00:00:00 2001 From: Ivan Dimitrov Date: Fri, 13 Jan 2017 10:53:15 +0000 Subject: [PATCH] 333 - Fixed the logic for setting the custom headers in case of POST and GET requests - for GET it will put the custom headers in the HTTP response headers - for POST it will put the custom headers in the response body - the setCustomHeaders was moved out of component.js to get-component.js under the name filterCustomHeaders. It's because in case of POST request the code path is different and goes to component(s).js instead of component.js. Now we do the filtering in one and the same place. - fixed a bug where we used to not take into account the custom headers in case of a unrendered component. For this reason the actual setting of response headers was moved from line 205 up to line 175. - fixed the registry acceptance tests. In their former variant when the GET tests passes they were used to leave the customHeadersToSkipOnWeakVersion settings so the POST tests were starting directly with this setting on. That's why new before/after sections were defined to reinitialize the registry before the tests and then recover it after them. --- src/registry/routes/component.js | 22 +- src/registry/routes/helpers/get-component.js | 21 +- test/acceptance/registry.js | 217 +++++++++++++------ test/unit/registry-routes-component.js | 7 +- 4 files changed, 177 insertions(+), 90 deletions(-) diff --git a/src/registry/routes/component.js b/src/registry/routes/component.js index 4474fb049..63944281c 100644 --- a/src/registry/routes/component.js +++ b/src/registry/routes/component.js @@ -7,20 +7,6 @@ module.exports = function(conf, repository){ var getComponent = new GetComponentHelper(conf, repository); - var setCustomHeaders = function(req, res, componentResponse) { - var headersToSet = componentResponse.headers; - - if (req.params.componentVersion !== componentResponse.version && - !_.isEmpty(res.conf.customHeadersToSkipOnWeakVersion)) - { - headersToSet = _.omit(headersToSet, res.conf.customHeadersToSkipOnWeakVersion); - } - - if (!_.isEmpty(headersToSet)) { - res.set(headersToSet); - } - }; - return function(req, res){ getComponent({ conf: res.conf, @@ -34,8 +20,12 @@ module.exports = function(conf, repository){ res.errorDetails = result.response.error; } - if (result.response.headers) { - setCustomHeaders(req, res, result.response); + if (!_.isEmpty(result.response.headers)) { + res.set(result.response.headers); + + if (req.method === 'GET') { + delete result.response.headers; + } } return res.json(result.status, result.response); diff --git a/src/registry/routes/helpers/get-component.js b/src/registry/routes/helpers/get-component.js index dd84e2a76..e683fbfba 100644 --- a/src/registry/routes/helpers/get-component.js +++ b/src/registry/routes/helpers/get-component.js @@ -118,6 +118,16 @@ module.exports = function(conf, repository){ }); } + var filterCustomHeaders = function(headers, requestedVersion, actualVersion) { + if (!_.isEmpty(headers) && + !_.isEmpty(conf.customHeadersToSkipOnWeakVersion) && + requestedVersion !== actualVersion) + { + headers = _.omit(headers, conf.customHeadersToSkipOnWeakVersion); + } + return headers; + }; + var returnComponent = function(err, data){ if(componentCallbackDone){ return; } @@ -162,6 +172,13 @@ module.exports = function(conf, repository){ renderMode: renderMode }); + if (responseHeaders) { + responseHeaders = filterCustomHeaders(responseHeaders, requestedComponent.version, component.version); + if (!_.isEmpty(responseHeaders)) { + response.headers = responseHeaders; + } + } + if (isUnrendered) { callback({ status: 200, @@ -202,10 +219,6 @@ module.exports = function(conf, repository){ }); } - if (responseHeaders) { - response.headers = responseHeaders; - } - callback({ status: 200, response: _.extend(response, { html: html }) diff --git a/test/acceptance/registry.js b/test/acceptance/registry.js index d5639c3b0..e83c58a81 100644 --- a/test/acceptance/registry.js +++ b/test/acceptance/registry.js @@ -4,6 +4,7 @@ var expect = require('chai').expect; var path = require('path'); var request = require('minimal-request'); var url = require('url'); +var _ = require('underscore'); describe('registry', function(){ @@ -15,16 +16,16 @@ describe('registry', function(){ conf; var next = function(done){ - return function(e, r, h){ + return function(e, r, d){ error = e; result = r; - headers = h; + headers = d.response.headers; done(); }; }; - before(function(done){ - conf = { + var getDefaultTestConfiguration = function() { + return { local: true, path: path.resolve('test/fixtures/components'), port: 3030, @@ -33,9 +34,15 @@ describe('registry', function(){ verbosity: 0, dependencies: ['underscore'] }; + }; + + var initializeRegistry = function(configuration, cb) { + registry = new oc.Registry(configuration); + registry.start(cb); + }; - registry = new oc.Registry(conf); - registry.start(done); + before(function(done){ + initializeRegistry(getDefaultTestConfiguration(), done); }); after(function(done){ registry.close(done); }); @@ -53,48 +60,9 @@ describe('registry', function(){ describe('GET /hello-world-custom-headers', function() { - var httpGetRequest = function(options, callback) { - var callbackDone = false; - var requestData = url.parse(options.url); - requestData.headers = options.headers || {}; - requestData.method = 'get'; - - var req = require('http').request(requestData).on('response', function(response) { - var body = []; - - response.on('data', function(chunk){ - body.push(chunk); - }).on('end', function(){ - body = Buffer.concat(body); - - if(!callbackDone){ - callbackDone = true; - var error = response.statusCode !== 200 ? response.statusCode : null; - - if (options.json) { - try { - callback(error, JSON.parse(body), response.headers); - } catch(e){ - return callback('json parsing error'); - } - } else { - callback(error, body, response.headers); - } - } - }); - }).on('error', function(e){ - if(!callbackDone){ - callbackDone = true; - callback(e); - } - }); - - req.end(); - }; - describe('with the default configuration (no customHeadersToSkipOnWeakVersion defined) and strong version 1.0.0', function() { before(function(done) { - httpGetRequest({ + request({ url: 'http://localhost:3030/hello-world-custom-headers/1.0.0', json: true }, next(done)); @@ -103,7 +71,7 @@ describe('registry', function(){ it('should return the component with custom headers', function() { expect(result.version).to.equal('1.0.0'); expect(result.name).to.equal('hello-world-custom-headers'); - expect(result.headers).to.eql({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + expect(result.headers).to.be.undefined; expect(headers).to.have.property('cache-control', 'public max-age=3600'); expect(headers).to.have.property('test-header', 'Test-Value'); }); @@ -111,7 +79,7 @@ describe('registry', function(){ describe('with the default configuration (no customHeadersToSkipOnWeakVersion defined) and weak version 1.x.x', function() { before(function(done) { - httpGetRequest({ + request({ url: 'http://localhost:3030/hello-world-custom-headers/1.x.x', json: true }, next(done)); @@ -120,7 +88,7 @@ describe('registry', function(){ it('should return the component with custom headers', function() { expect(result.version).to.equal('1.0.0'); expect(result.name).to.equal('hello-world-custom-headers'); - expect(result.headers).to.eql({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + expect(result.headers).to.be.undefined; expect(headers).to.have.property('cache-control', 'public max-age=3600'); expect(headers).to.have.property('test-header', 'Test-Value'); }); @@ -129,25 +97,18 @@ describe('registry', function(){ describe('with a custom configuration with customHeadersToSkipOnWeakVersion defined', function() { before(function(done) { registry.close(); + initializeRegistry(_.extend(getDefaultTestConfiguration(), {customHeadersToSkipOnWeakVersion: ['Cache-Control']}), done); + }); - conf = { - local: true, - path: path.resolve('test/fixtures/components'), - port: 3030, - baseUrl: 'http://localhost:3030/', - env: { name: 'local' }, - verbosity: 0, - dependencies: ['underscore'], - customHeadersToSkipOnWeakVersion: ['Cache-Control'] - }; - - registry = new oc.Registry(conf); - registry.start(done); + after(function(done){ + registry.close(function() { + initializeRegistry(getDefaultTestConfiguration(), done); + }); }); describe('when strong version is requested 1.0.0', function() { before(function(done) { - httpGetRequest({ + request({ url: 'http://localhost:3030/hello-world-custom-headers/1.0.0', json: true }, next(done)); @@ -156,7 +117,7 @@ describe('registry', function(){ it('should return the component with the custom headers', function() { expect(result.version).to.equal('1.0.0'); expect(result.name).to.equal('hello-world-custom-headers'); - expect(result.headers).to.eql({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + expect(result.headers).to.be.undefined; expect(headers).to.have.property('cache-control', 'public max-age=3600'); expect(headers).to.have.property('test-header', 'Test-Value'); }); @@ -164,7 +125,7 @@ describe('registry', function(){ describe('when weak version is requested 1.x.x', function() { before(function(done) { - httpGetRequest({ + request({ url: 'http://localhost:3030/hello-world-custom-headers/1.x.x', json: true }, next(done)); @@ -173,7 +134,7 @@ describe('registry', function(){ it('should skip Cache-Control header', function() { expect(result.version).to.equal('1.0.0'); expect(result.name).to.equal('hello-world-custom-headers'); - expect(result.headers).to.eql({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + expect(result.headers).to.be.undefined; expect(headers).to.not.have.property('cache-control'); expect(headers).to.have.property('test-header', 'Test-Value'); }); @@ -181,6 +142,130 @@ describe('registry', function(){ }); }); + describe('POST /hello-world-custom-headers', function() { + + describe('with the default configuration (no customHeadersToSkipOnWeakVersion defined) and strong version 1.0.0', function() { + before(function(done) { + request({ + url: 'http://localhost:3030', + json: true, + method: 'post', + body: { + components: [{ + name: 'hello-world-custom-headers', + version: '1.0.0' + }] + } + }, next(done)); + }); + + it('should not set HTTP custom headers', function() { + expect(headers).to.not.have.property('cache-control'); + expect(headers).to.not.have.property('test-header'); + }); + + it('should return the component with custom headers', function() { + expect(result[0].response.version).to.equal('1.0.0'); + expect(result[0].response.name).to.equal('hello-world-custom-headers'); + expect(result[0].response.headers).to.be.deep.equal({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + }); + }); + + describe('with the default configuration (no customHeadersToSkipOnWeakVersion defined) and weak version 1.x.x', function() { + before(function(done) { + request({ + url: 'http://localhost:3030', + json: true, + method: 'post', + body: { + components: [{ + name: 'hello-world-custom-headers', + version: '1.x.x' + }] + } + }, next(done)); + }); + + it('should not set HTTP custom headers', function() { + expect(headers).to.not.have.property('cache-control'); + expect(headers).to.not.have.property('test-header'); + }); + + it('should return the component with custom headers in the response body', function() { + expect(result[0].response.version).to.equal('1.0.0'); + expect(result[0].response.name).to.equal('hello-world-custom-headers'); + expect(result[0].response.headers).to.be.deep.equal({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + }); + }); + + describe('with a custom configuration with customHeadersToSkipOnWeakVersion defined', function() { + before(function(done) { + registry.close(); + initializeRegistry(_.extend(getDefaultTestConfiguration(), {customHeadersToSkipOnWeakVersion: ['Cache-Control']}), done); + }); + + after(function(done){ + registry.close(function() { + initializeRegistry(getDefaultTestConfiguration(), done); + }); + }); + + describe('when strong version is requested 1.0.0', function() { + before(function(done) { + request({ + url: 'http://localhost:3030', + json: true, + method: 'post', + body: { + components: [{ + name: 'hello-world-custom-headers', + version: '1.0.0' + }] + } + }, next(done)); + }); + + it('should not set HTTP custom headers', function() { + expect(headers).to.not.have.property('cache-control'); + expect(headers).to.not.have.property('test-header'); + }); + + it('should return the component with the custom headers', function() { + expect(result[0].response.version).to.equal('1.0.0'); + expect(result[0].response.name).to.equal('hello-world-custom-headers'); + expect(result[0].response.headers).to.be.deep.equal({'cache-control': 'public max-age=3600', 'test-header': 'Test-Value'}); + }); + }); + + describe('when weak version is requested 1.x.x', function() { + before(function(done) { + request({ + url: 'http://localhost:3030', + json: true, + method: 'post', + body: { + components: [{ + name: 'hello-world-custom-headers', + version: '1.x.x' + }] + } + }, next(done)); + }); + + it('should not set HTTP custom headers', function() { + expect(headers).to.not.have.property('cache-control'); + expect(headers).to.not.have.property('test-header'); + }); + + it('should skip Cache-Control header', function() { + expect(result[0].response.version).to.equal('1.0.0'); + expect(result[0].response.name).to.equal('hello-world-custom-headers'); + expect(result[0].response.headers).to.be.deep.equal({'test-header': 'Test-Value'}); + }); + }); + }); + }); + describe('GET /', function(){ before(function(done){ diff --git a/test/unit/registry-routes-component.js b/test/unit/registry-routes-component.js index b8a28b66a..c4c68f156 100644 --- a/test/unit/registry-routes-component.js +++ b/test/unit/registry-routes-component.js @@ -465,7 +465,7 @@ describe('registry : routes : component', function(){ }); }); - describe('when getting a component with server.js that sets custom headers with empty customHeadersToSkipOnWeakVersion', function() { + describe('when getting a component with server.js that sets custom headers with non-empty customHeadersToSkipOnWeakVersion', function() { var code, response, headers; before(function(done) { @@ -500,9 +500,8 @@ describe('registry : routes : component', function(){ expect(code).to.be.equal(200); }); - it('should not set the HTTP response test-headers', function() { - expect(response.headers).to.not.be.null; - expect(response.headers['test-header']).to.equal('test-value'); + it('should not set response headers', function() { + expect(response.headers).to.be.undefined; expect(headers).to.be.undefined; });