Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client errors #291

Merged
merged 10 commits into from
Sep 20, 2016
44 changes: 29 additions & 15 deletions client/src/get-components-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,6 @@ var _ = require('./utils/helpers');

module.exports = function(config){

var makePostRequest = function(components, options, cb){
request({
url: config.registries.serverRendering,
method: 'post',
headers: options.headers,
timeout: options.timeout,
json: true,
body: { components: components }
}, cb);
};

return function(toDo, options, cb){

var serverRenderingFail = settings.serverSideRenderingFail,
Expand Down Expand Up @@ -48,14 +37,26 @@ module.exports = function(config){
return cb(serverRenderingFail);
}

makePostRequest(serverRendering.components, options, function(error, responses){
var requestDetails = {
url: config.registries.serverRendering,
method: 'post',
headers: options.headers,
timeout: options.timeout,
json: true,
body: {
components: serverRendering.components
}
};

request(requestDetails, function(error, responses){
if(!!error || !responses || _.isEmpty(responses)){
responses = [];
var errorDetails = !!error ? error.toString() : settings.emptyResponse;
_.each(serverRendering.components, function(){
responses.push({
response: { error: format(settings.connectionError, errorDetails, config.registries.serverRendering) },
status: 500
response: {
error: format(settings.connectionError, JSON.stringify(requestDetails), errorDetails)
}
});
});
}
Expand All @@ -65,7 +66,20 @@ module.exports = function(config){

if(action.render === 'server'){
if(response.status !== 200){
var errorDetails = format('{0} ({1})', (response.response && response.response.error) || '', response.status);

var errorDetails;
if(!response.status && response.response.error){
errorDetails = response.response.error;
} else {

var errorDescription = (response.response && response.response.error);
if(errorDescription && response.response.details && response.response.details.originalError){
errorDescription += response.response.details.originalError;
}

errorDetails = format('{0} ({1})', errorDescription || '', response.status);
}

action.result.error = new Error(format(serverRenderingFail, errorDetails));
if(!!options.disableFailoverRendering){
action.result.html = '';
Expand Down
2 changes: 1 addition & 1 deletion client/src/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module.exports = {
clientSideRenderingFail: 'Client-side rendering failed',
configurationNotValid: 'Configuration is not valid: ',
connectionError: '{0} when connecting to {1}',
connectionError: 'request {0} failed ({1})',
emptyResponse: 'Empty response',
genericError: 'Internal client error',
legacyComponent: 'The component can\'t be rendered because it was published with an older OC version',
Expand Down
133 changes: 126 additions & 7 deletions test/acceptance/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
var cheerio = require('cheerio');
var expect = require('chai').expect;
var path = require('path');
var _ = require('underscore');

describe('The node.js OC client', function(){

Expand Down Expand Up @@ -34,6 +35,12 @@ describe('The node.js OC client', function(){
};
};

var getRegExpFromJson = function(x){
return JSON.stringify(x)
.replace(/\+/g, '\\+')
.replace(/\[/g, '\\[');
};

describe('when initialised providing registries properties', function(){

before(function(done){
Expand Down Expand Up @@ -151,22 +158,32 @@ describe('The node.js OC client', function(){
});
});

describe('when there\'s error in one of them', function(){
describe('when there are errors in some of them', function(){
var $errs;
before(function(done){
client.renderComponents([{
name: 'hello-world-i-dont-exist'
}, {
name: 'no-containers'
}], { container: false, renderInfo: false }, function(err, html){
}, {
name: 'errors-component',
parameters: {
errorType: '500'
}
}], {
container: false,
renderInfo: false,
disableFailoverRendering: true
}, function(err, html){
$errs = err;
done();
});
});

it('should return an error for the component with error', function(){
it('should return an error for each component with error', function(){
expect($errs[0].toString()).to.be.equal('Error: Server-side rendering failed: Component "hello-world-i-dont-exist" not found on local repository (404)');
expect($errs[1]).to.be.null;
expect($errs[2].toString()).to.be.equal('Error: Server-side rendering failed: Component execution error: An error happened (500)');
});
});
});
Expand Down Expand Up @@ -216,6 +233,23 @@ describe('The node.js OC client', function(){

describe('when server-side rendering an existing component linked to a non responsive registry', function(){

var expectedRequest = {
url: 'http://localhost:1234',
method: 'post',
headers: {
accept: 'application/vnd.oc.unrendered+json',
'user-agent': 'oc-client-(.*?)'
},
timeout: 5,
json: true,
body: {
components: [{
name: 'hello-world',
version: '~1.0.0'
}]
}
};

describe('when client-side failover rendering disabled', function(){

var error;
Expand All @@ -235,7 +269,11 @@ describe('The node.js OC client', function(){
});

it('should contain the error details', function(){
expect(error.toString()).to.match(/Error\: Server-side rendering failed: Error: (.*?) when connecting to http\:\/\/localhost\:1234 \(500\)/g);

var exp = getRegExpFromJson(expectedRequest),
expected = new RegExp('Error: Server-side rendering failed: request ' + exp + ' failed \\(Error: connect ECONNREFUSED(.*?)\\)');

expect(error.toString()).to.match(expected);
});
});

Expand Down Expand Up @@ -268,7 +306,11 @@ describe('The node.js OC client', function(){
});

it('should contain the error details', function(){
expect(error.toString()).to.match(/Error\: Server-side rendering failed: Error: (.*?) when connecting to http\:\/\/localhost\:1234 \(500\)/g);

var exp = getRegExpFromJson(expectedRequest),
expected = new RegExp('Error: Server-side rendering failed: request ' + exp + ' failed \\(Error: connect ECONNREFUSED(.*?)\\)');

expect(error.toString()).to.match(expected);
});
});

Expand Down Expand Up @@ -307,7 +349,32 @@ describe('The node.js OC client', function(){
});

it('should contain the error details', function(){
expect(error.toString()).to.match(/Error\: Server-side rendering failed: Error: (.*?) when connecting to http\:\/\/localhost\:1234 \(500\)/g);

var expectedRequestWithExtraParams = {
url: 'http://localhost:1234',
method: 'post',
headers: {
'accept-language': 'da, en-gb;q=0.8, en;q=0.7',
accept: 'application/vnd.oc.unrendered+json',
'user-agent': 'oc-client-(.*?)'
},
timeout: 5,
json: true,
body: {
components: [{
name: 'hello-world',
version: '~1.0.0',
parameters: {
hi: 'john'
}
}]
}
};

var exp = getRegExpFromJson(expectedRequestWithExtraParams),
expected = new RegExp('Error: Server-side rendering failed: request ' + exp + ' failed \\(Error: connect ECONNREFUSED(.*?)\\)');

expect(error.toString()).to.match(expected);
});
});

Expand Down Expand Up @@ -341,13 +408,65 @@ describe('The node.js OC client', function(){
});

it('should contain the error details', function(){
expect(error.toString()).to.match(/Error\: Server-side rendering failed: Error: (.*?) when connecting to http\:\/\/localhost\:1234 \(500\)/g);
var exp = getRegExpFromJson(expectedRequest),
expected = new RegExp('Error: Server-side rendering failed: request ' + exp + ' failed \\(Error: connect ECONNREFUSED(.*?)\\)');

expect(error.toString()).to.match(expected);
});
});
});

describe('when server-side rendering an existing component linked to a responsive registry', function(){

describe('when the component times-out', function(){

var error, result;

var expectedRequest = {
url: 'http://localhost:3030',
method: 'post',
headers: {
accept: 'application/vnd.oc.unrendered+json',
'user-agent': 'oc-client-(.*?)'
},
timeout: 0.01,
json: true,
body: {
components: [{
name: 'errors-component',
parameters: {
errorType: 'timeout',
timeout: 1000
}
}]
}
};

before(function(done){
client.renderComponent('errors-component', {
parameters: { errorType: 'timeout', timeout: 1000 },
timeout: 0.01,
disableFailoverRendering: true
}, function(err, html){
error = err;
result = html;
done();
});
});

it('should contain a blank html response', function(){
expect(result).to.eql('');
});

it('should contain the error details', function(){

var exp = getRegExpFromJson(expectedRequest),
expected = new RegExp('Error: Server-side rendering failed: request ' + exp + ' failed \\(timeout\\)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matteofigus is it worth including actual timeout value in the error message as well (for diagnostic purposes)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is there. The client has a default overridable timeout value that is always passed through the request call. Now we log all the arguments that we pass to that function call ( a JSON.stringify of expectedRequest - a couple of lines up) - which contains timeout, headers, body payload, everything.

An example of an error (I censored some private data stuff):

[Error: Server-side rendering failed: request {"url":"http://xxx","method":"post","headers":{"accept-language":"fr-CA, fr, en, *","ot-requestid":"xxx","ot-referringhost":"127.0.0.1","ot-referringservice":"xxx","accept":"application/vnd.oc.unrendered+json","user-agent":"oc-client-1.10.0/v4.4.4-darwin-x64"},"timeout":5,"json":true,"body":{"components":[{"name":"xxx","parameters":{xxx},"version":"1.2.0"}]}} failed (timeout)] ]


expect(error.toString()).to.match(expected);
});
});

describe('when container option = true', function(){
var error;
before(function(done){
Expand Down
1 change: 1 addition & 0 deletions test/acceptance/registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ describe('registry', function(){
expect(result.components).to.eql([
'http://localhost:3030/container-with-multiple-nested',
'http://localhost:3030/container-with-nested',
'http://localhost:3030/errors-component',
'http://localhost:3030/handlebars3-component',
'http://localhost:3030/hello-world',
'http://localhost:3030/language',
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/components/errors-component/.npmignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
_package
package.tar.gz
node_modules
26 changes: 26 additions & 0 deletions test/fixtures/components/errors-component/_package/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"name": "errors-component",
"description": "",
"version": "1.0.0",
"repository": "",
"oc": {
"container": false,
"renderInfo": false,
"files": {
"template": {
"type": "jade",
"hashKey": "884851aea70e65c9aed7f2f4f40e91566a24404c",
"src": "template.js"
},
"dataProvider": {
"type": "node.js",
"hashKey": "74d5a719e6e0d15ceb53a65ddb32284afc125c56",
"src": "server.js"
},
"static": []
},
"version": "0.33.6",
"packaged": true,
"date": 1474297320045
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions test/fixtures/components/errors-component/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"name": "errors-component",
"description": "",
"version": "1.0.0",
"repository": "",
"oc": {
"container": false,
"renderInfo": false,
"files": {
"data": "server.js",
"template": {
"src": "template.jade",
"type": "jade"
}
}
}
}
20 changes: 20 additions & 0 deletions test/fixtures/components/errors-component/server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

module.exports.data = function(context, callback){

if(context.params.errorType === 'timeout'){
setTimeout(function() {
callback(null, {
error: true,
timeout: true
});
}, context.params.timeout || 2000);
} else if(context.params.errorType === '500'){
callback('An error happened');
} else {

callback(null, {
error: false
});
}
};
1 change: 1 addition & 0 deletions test/fixtures/components/errors-component/template.jade
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
div Any problems? #{error ? 'Yep' : 'Nope'}.
1 change: 1 addition & 0 deletions test/unit/registry-domain-repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ describe('registry : domain : repository', function(){
expect(response.result).to.eql([
'container-with-multiple-nested',
'container-with-nested',
'errors-component',
'handlebars3-component',
'hello-world',
'language',
Expand Down