Skip to content

Commit

Permalink
Fix for Custom cache headers of one component can appear to another.
Browse files Browse the repository at this point in the history
The problem was introduced with opencomponents#326 -
In get-component.js the declaration of responseHeaders var happens in a more
global scope than it should be. For this reason every invocation of renderer
function sees the changes in the headers made by previously requested
components.
The fix is to just move the declaration one scope inner.

- Added a unit test to validate the fix - it tries to load two components
subsequently. The first one does provide custom headers while the second
one - don't. Then we check and expect to see there are no custom headers
in the result for the second one.
  • Loading branch information
i-b-dimitrov committed Dec 9, 2016
1 parent 0dac67e commit b685780
Show file tree
Hide file tree
Showing 5 changed files with 160 additions and 5 deletions.
6 changes: 3 additions & 3 deletions src/registry/routes/helpers/get-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ module.exports = function(conf, repository){
cache = new Cache({
verbose: !!conf.verbosity,
refreshInterval: conf.refreshInterval
}),
responseHeaders;
});

var renderer = function(options, cb){

var nestedRenderer = new NestedRenderer(renderer, options.conf),
retrievingInfo = new GetComponentRetrievingInfo(options);
retrievingInfo = new GetComponentRetrievingInfo(options),
responseHeaders;

var getLanguage = function(){
var paramOverride = !!options.parameters && options.parameters['__ocAcceptLanguage'];
Expand Down
27 changes: 27 additions & 0 deletions test/fixtures/mocked-components/another-response-headers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

module.exports = {
package: {
name: 'response-headers-2-component',
version: '1.0.0',
oc: {
container: false,
renderInfo: false,
files: {
template: {
type: 'jade',
hashKey: '8c1fbd954f2b0d8cd5cf11c885fed4805225749f',
src: 'template.js'
},
dataProvider: {
type: 'node.js',
hashKey: '123456',
src: 'server.js'
}
}
}
},
data: '"use strict";module.exports.data = function(ctx, cb){ctx.setHeader("another-test-header","another-test-value"); cb(null, {done:true});};',
view: 'var oc=oc||{};oc.components=oc.components||{},oc.components["8c1fbd954f2b0d8cd5cf11c885fed4805225749f"]' +
'=function(){var o=[];return o.push("<div>hello</div>"),o.join("")};'
};
4 changes: 3 additions & 1 deletion test/fixtures/mocked-components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ module.exports = {
'plugin-component': require('./plugin'),
'timeout-component': require('./timeout'),
'undefined-component': require('./undefined'),
'response-headers-component': require('./response-headers')
'simple-component': require('./simple'),
'response-headers-component': require('./response-headers'),
'another-response-headers-component': require('./another-response-headers')
};
27 changes: 27 additions & 0 deletions test/fixtures/mocked-components/simple.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

module.exports = {
package: {
name: 'simple-component',
version: '1.0.0',
oc: {
container: false,
renderInfo: false,
files: {
template: {
type: 'jade',
hashKey: '8c1fbd954f2b0d8cd5cf11c885fed4805225749f',
src: 'template.js'
},
dataProvider: {
type: 'node.js',
hashKey: '123457',
src: 'server.js'
}
}
}
},
data: '"use strict";module.exports.data = function(ctx, cb){cb(null, {done:true});};',
view: 'var oc=oc||{};oc.components=oc.components||{},oc.components["8c1fbd954f2b0d8cd5cf11c885fed4805225749f"]' +
'=function(){var o=[];return o.push("<div>hello</div>"),o.join("")};'
};
101 changes: 100 additions & 1 deletion test/unit/registry-routes-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,6 @@ describe('registry : routes : component', function(){
});

describe('when getting a component with server.js that sets custom headers', function() {

var code, response, headers;

before(function(done) {
Expand Down Expand Up @@ -466,4 +465,104 @@ describe('registry : routes : component', function(){
});
});

describe('when getting a simple component with server.js after headers component no custom headers should be set', function() {
var code={},
response={},
headers={};

before(function(done) {
var headersComponent = mockedComponents['another-response-headers-component'];
var simpleComponent = mockedComponents['simple-component'];

mockedRepository = {
getCompiledView: sinon.stub(),
getComponent: sinon.stub(),
getDataProvider: sinon.stub(),
getStaticFilePath: sinon.stub().returns('//my-cdn.com/files/')
};

//Custom repository initialization to give us two components when invoked twice...
//...the firts one with custom headers and the second - without.
mockedRepository.getCompiledView.onCall(0).yields(null, headersComponent.view);
mockedRepository.getCompiledView.onCall(1).yields(null, simpleComponent.view);

mockedRepository.getComponent.onCall(0).yields(null, headersComponent.package);
mockedRepository.getComponent.onCall(1).yields(null, simpleComponent.package);

mockedRepository.getDataProvider.onCall(0).yields(null, headersComponent.data);
mockedRepository.getDataProvider.onCall(1).yields(null, simpleComponent.data);

componentRoute = new ComponentRoute({}, mockedRepository);
resJsonStub = sinon.stub();

var resJson = function(index, callback) {
return function(calledCode, calledResponse) {
code[index] = calledCode;
response[index] = calledResponse;
callback && callback();
};
};

var resSet = function(index) {
return function(calledHeaders) {
headers[index] = calledHeaders;
};
};

var requestComponent = function(componentName, resultIndex) {
return new Promise(function(resolve, reject) {
componentRoute({
headers: {},
params: { componentName: componentName, componentVersion: '1.X.X' }
}, {
conf: {
baseUrl: 'http://component.com/',
executionTimeout: 0.1
},
json: resJson(resultIndex, function() { resolve(); }),
set: resSet(resultIndex)
});
});
};

requestComponent('another-response-headers-component', 0)
.then(requestComponent('simple-component', 1))
.then(function() { done(); });
});

//The first part of the test - checking that another-response-headers-component
//should return a response with custom headers
it('should return 200 status code for the first component', function() {
expect(code[0]).to.be.equal(200);
});

it('should return "response-headers-component" name for the first component\'s name and request version', function() {
expect(response[0].name).to.equal('another-response-headers-component');
expect(response[0].requestVersion).to.equal('1.X.X');
});

it('should set response headers for the first component', function() {
expect(response[0].headers).to.not.be.null;
expect(response[0].headers['another-test-header']).to.equal('another-test-value');
expect(headers[0]).to.not.be.null;
expect(headers[0]['another-test-header']).to.equal('another-test-value');
});

//The second part of the test - validating that simple-component should not return
//the custom headers previously set by another-headers-component.
it('should return 200 status code for the first component', function() {
expect(code[1]).to.be.equal(200);
});

it('should return "simple-component" name for the first component\'s name and request version', function() {
expect(response[1].name).to.equal('simple-component');
expect(response[1].requestVersion).to.equal('1.X.X');
});

it('should not set custom response', function() {
expect(response[1].headers).to.be.undefined;
expect(headers[1]).to.be.undefined;
});
});

});

0 comments on commit b685780

Please sign in to comment.