Skip to content

Commit

Permalink
Merge pull request elastic#8376 from elastic/jasper/backport/8339/5.x
Browse files Browse the repository at this point in the history
[backport] PR elastic#8339 to 5.x - Set minimum aggregation 'Size' input value to 1, because ES will return an error if you provide a size of 0.
  • Loading branch information
cjcenizal committed Sep 20, 2016
2 parents 723b788 + 0ca20ac commit a4df947
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 4 deletions.
6 changes: 3 additions & 3 deletions src/ui/public/agg_types/controls/order_and_size.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
required
class="form-control"
type="number"
min="0"
>
min="1"
>
</div>
</div>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import sinon from 'auto-release-sinon';
import expect from 'expect.js';
import ElasticsearchError from '../elasticsearch_error';

describe('ElasticsearchError', () => {
function createError(rootCauses = []) {
// Elasticsearch errors are characterized by the resp.error.root_cause array.
return {
resp: {
error: {
root_cause: rootCauses.map(rootCause => ({
reason: rootCause,
})),
}
}
};
}

describe('interface', () => {
describe('constructor', () => {
it('throws an error if instantiated with a non-elasticsearch error', () => {
expect(() => new ElasticsearchError({})).to.throwError();
});
});

describe('getRootCauses', () => {
it(`returns the root_cause array's reason values`, () => {
const rootCauses = ['a', 'b'];
const error = createError(rootCauses);
const esError = new ElasticsearchError(error);
expect(esError.getRootCauses()).to.eql(rootCauses);
});
});

describe('hasRootCause', () => {
it(`returns true if the cause occurs in the root_cause array's reasons, insensitive to case`, () => {
const rootCauses = ['a very detailed error', 'a slightly more detailed error'];
const error = createError(rootCauses);
const esError = new ElasticsearchError(error);
expect(esError.hasRootCause('slightly MORE')).to.be(true);
});

it(`returns false if the cause doesn't occur in the root_cause array's reasons`, () => {
const rootCauses = ['a very detailed error', 'a slightly more detailed error'];
const error = createError(rootCauses);
const esError = new ElasticsearchError(error);
expect(esError.hasRootCause('nonexistent error')).to.be(false);
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import sinon from 'auto-release-sinon';
import expect from 'expect.js';
import isTermSizeZeroError from '../is_term_size_zero_error';

describe('isTermSizeZeroError', () => {
const identifyingString = 'size must be positive, got 0';

it('returns true if it contains the identifying string', () => {
const error = {
resp: {
error: {
root_cause: [{
reason: `Some crazy Java exception: ${identifyingString}`,
}],
}
}
};
expect(isTermSizeZeroError(error)).to.be(true);
});

it(`returns false if it doesn't contain the identifying string`, () => {
const error = {
resp: {
error: {
root_cause: [{
reason: `Some crazy Java exception`,
}],
}
}
};
expect(isTermSizeZeroError(error)).to.be(false);
});
});
32 changes: 32 additions & 0 deletions src/ui/public/elasticsearch_errors/elasticsearch_error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import _ from 'lodash';

export default class ElasticsearchError {
constructor(error) {
this.error = error;

this.getRootCauses = this.getRootCauses.bind(this);
this.hasRootCause = this.hasRootCause.bind(this);

if (!this.getRootCauses().length) {
throw new Error(
'ElasticsearchError must be instantiated with an elasticsearch error, i.e. it must have' +
`a resp.error.root_cause property. Instead got ${JSON.stringify(error)}`
);
}
}

getRootCauses() {
const rootCauses = _.get(this.error, 'resp.error.root_cause');
return _.pluck(rootCauses, 'reason');
}

hasRootCause(cause) {
const normalizedCause = cause.toLowerCase();
const rootCauses = this.getRootCauses();
const matchingCauses = rootCauses.filter(rootCause => {
const normalizedRootCause = rootCause.toLowerCase();
return normalizedRootCause.indexOf(normalizedCause) !== -1;
});
return matchingCauses.length !== 0;
}
}
7 changes: 7 additions & 0 deletions src/ui/public/elasticsearch_errors/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export {
default as ElasticsearchError,
} from './elasticsearch_error';

export {
default as isTermSizeZeroError,
} from './is_term_size_zero_error';
6 changes: 6 additions & 0 deletions src/ui/public/elasticsearch_errors/is_term_size_zero_error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import ElasticsearchError from './elasticsearch_error';

export default function isTermSizeZeroError(error) {
const esError = new ElasticsearchError(error);
return esError.hasRootCause('size must be positive, got 0');
}
16 changes: 15 additions & 1 deletion src/ui/public/visualize/visualize.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import uiModules from 'ui/modules';
import visualizeTemplate from 'ui/visualize/visualize.html';
import 'angular-sanitize';

import {
isTermSizeZeroError,
} from '../elasticsearch_errors';

uiModules
.get('kibana/directive', ['ngSanitize'])
.directive('visualize', function (Notifier, SavedVis, indexPatterns, Private, config, $timeout) {
Expand Down Expand Up @@ -155,7 +159,17 @@ uiModules
return searchSource.onResults().then(onResults);
}).catch(notify.fatal);

searchSource.onError(notify.error).catch(notify.fatal);
searchSource.onError(e => {
if (isTermSizeZeroError(e)) {
return notify.error(
`Your visualization ('${$scope.vis.title}') has an error: it has a term ` +
`aggregation with a size of 0. Please set it to a number greater than 0 to resolve ` +
`the error.`
);
}

notify.error(e);
}).catch(notify.fatal);
}));

$scope.$watch('esResp', prereq(function (resp, prevResp) {
Expand Down

0 comments on commit a4df947

Please sign in to comment.