Skip to content

Commit

Permalink
feat: improved domain error handling (opensearch-project#933)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nickofthyme
nickofthyme authored Dec 3, 2020
1 parent 196ee8d commit f480054
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 84 deletions.
73 changes: 41 additions & 32 deletions packages/osd-charts/src/chart_types/xy_chart/domains/x_domain.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,17 @@ import { ChartTypes } from '../..';
import { MockSeriesSpecs } from '../../../mocks/specs';
import { ScaleType } from '../../../scales/constants';
import { SpecTypes, Direction, BinAgg } from '../../../specs/constants';
import { Logger } from '../../../utils/logger';
import { getDataSeriesFromSpecs } from '../utils/series';
import { BasicSeriesSpec, SeriesTypes } from '../utils/specs';
import { convertXScaleTypes, findMinInterval, mergeXDomain } from './x_domain';

jest.mock('../../../utils/logger', () => ({
Logger: {
warn: jest.fn(),
},
}));

describe('X Domain', () => {
test('Should return null when missing specs or specs types', () => {
const seriesSpecs: BasicSeriesSpec[] = [];
Expand Down Expand Up @@ -738,17 +745,17 @@ describe('X Domain', () => {
expect(basicMergedDomain.domain).toEqual([0, 3]);

const arrayXDomain = [1, 2];
const attemptToMergeArrayDomain = () => {
mergeXDomain(specs, xValues, arrayXDomain);
};
const errorMessage = 'xDomain for continuous scale should be a DomainRange object, not an array';
expect(attemptToMergeArrayDomain).toThrowError(errorMessage);
let { domain } = mergeXDomain(specs, xValues, arrayXDomain);
expect(domain).toEqual([1, 5]);
const warnMessage = 'xDomain for continuous scale should be a DomainRange object, not an array';
expect(Logger.warn).toBeCalledWith(warnMessage);

(Logger.warn as jest.Mock).mockClear();

const invalidXDomain = { min: 10, max: 0 };
const attemptToMerge = () => {
mergeXDomain(specs, xValues, invalidXDomain);
};
expect(attemptToMerge).toThrowError('custom xDomain is invalid, min is greater than max');
domain = mergeXDomain(specs, xValues, invalidXDomain).domain;
expect(domain).toEqual([1, 5]);
expect(Logger.warn).toBeCalledWith('custom xDomain is invalid, min is greater than max. Custom domain is ignored.');
});

test('should account for custom domain when merging a linear domain: lower bounded domain', () => {
Expand All @@ -762,10 +769,11 @@ describe('X Domain', () => {
expect(mergedDomain.domain).toEqual([0, 5]);

const invalidXDomain = { min: 10 };
const attemptToMerge = () => {
mergeXDomain(specs, xValues, invalidXDomain);
};
expect(attemptToMerge).toThrowError('custom xDomain is invalid, custom min is greater than computed max');
const { domain } = mergeXDomain(specs, xValues, invalidXDomain);
expect(domain).toEqual([1, 5]);
expect(Logger.warn).toBeCalledWith(
'custom xDomain is invalid, custom min is greater than computed max. Custom domain is ignored.',
);
});

test('should account for custom domain when merging a linear domain: upper bounded domain', () => {
Expand All @@ -779,10 +787,11 @@ describe('X Domain', () => {
expect(mergedDomain.domain).toEqual([1, 3]);

const invalidXDomain = { max: -1 };
const attemptToMerge = () => {
mergeXDomain(specs, xValues, invalidXDomain);
};
expect(attemptToMerge).toThrowError('custom xDomain is invalid, computed min is greater than custom max');
const { domain } = mergeXDomain(specs, xValues, invalidXDomain);
expect(domain).toEqual([1, 5]);
expect(Logger.warn).toBeCalledWith(
'custom xDomain is invalid, computed min is greater than custom max. Custom domain is ignored.',
);
});

test('should account for custom domain when merging an ordinal domain', () => {
Expand All @@ -795,11 +804,11 @@ describe('X Domain', () => {
expect(basicMergedDomain.domain).toEqual(['a', 'b', 'c']);

const objectXDomain = { max: 10, min: 0 };
const attemptToMerge = () => {
mergeXDomain(specs, xValues, objectXDomain);
};
const errorMessage = 'xDomain for ordinal scale should be an array of values, not a DomainRange object';
expect(attemptToMerge).toThrowError(errorMessage);
const { domain } = mergeXDomain(specs, xValues, objectXDomain);
expect(domain).toEqual(['a', 'b', 'c', 'd']);
const warnMessage =
'xDomain for ordinal scale should be an array of values, not a DomainRange object. xDomain is ignored.';
expect(Logger.warn).toBeCalledWith(warnMessage);
});

describe('should account for custom minInterval', () => {
Expand All @@ -822,20 +831,20 @@ describe('X Domain', () => {

test('with invalid minInterval greater than computed minInterval for multi data set', () => {
const invalidXDomain = { minInterval: 10 };
const attemptToMerge = () => {
mergeXDomain(specs, xValues, invalidXDomain);
};
const expectedError = 'custom xDomain is invalid, custom minInterval is greater than computed minInterval';
expect(attemptToMerge).toThrowError(expectedError);
const { minInterval } = mergeXDomain(specs, xValues, invalidXDomain);
expect(minInterval).toEqual(1);
const expectedWarning =
'custom xDomain is invalid, custom minInterval is greater than computed minInterval. Using computed minInterval.';
expect(Logger.warn).toBeCalledWith(expectedWarning);
});

test('with invalid minInterval less than 0', () => {
const invalidXDomain = { minInterval: -1 };
const attemptToMerge = () => {
mergeXDomain(specs, xValues, invalidXDomain);
};
const expectedError = 'custom xDomain is invalid, custom minInterval is less than 0';
expect(attemptToMerge).toThrowError(expectedError);
const { minInterval } = mergeXDomain(specs, xValues, invalidXDomain);
expect(minInterval).toEqual(1);
const expectedWarning =
'custom xDomain is invalid, custom minInterval is less than 0. Using computed minInterval.';
expect(Logger.warn).toBeCalledWith(expectedWarning);
});
});

Expand Down
85 changes: 50 additions & 35 deletions packages/osd-charts/src/chart_types/xy_chart/domains/x_domain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ export function mergeXDomain(
1) Correct data to match ${mainXScaleType.scaleType} scale type (see previous warning)
2) Change xScaleType to ordinal and set xDomain to Domain array`);
} else {
throw new TypeError('xDomain for ordinal scale should be an array of values, not a DomainRange object.');
Logger.warn(
'xDomain for ordinal scale should be an array of values, not a DomainRange object. xDomain is ignored.',
);
}
}
}
Expand All @@ -77,44 +79,38 @@ export function mergeXDomain(

if (customXDomain) {
if (Array.isArray(customXDomain)) {
throw new TypeError('xDomain for continuous scale should be a DomainRange object, not an array');
}

customMinInterval = customXDomain.minInterval;
const [computedDomainMin, computedDomainMax] = seriesXComputedDomains;

if (isCompleteBound(customXDomain)) {
if (customXDomain.min > customXDomain.max) {
throw new Error('custom xDomain is invalid, min is greater than max');
}

seriesXComputedDomains = [customXDomain.min, customXDomain.max];
} else if (isLowerBound(customXDomain)) {
if (customXDomain.min > computedDomainMax) {
throw new Error('custom xDomain is invalid, custom min is greater than computed max');
}

seriesXComputedDomains = [customXDomain.min, computedDomainMax];
} else if (isUpperBound(customXDomain)) {
if (computedDomainMin > customXDomain.max) {
throw new Error('custom xDomain is invalid, computed min is greater than custom max');
Logger.warn('xDomain for continuous scale should be a DomainRange object, not an array');
} else {
customMinInterval = customXDomain.minInterval;
const [computedDomainMin, computedDomainMax] = seriesXComputedDomains;

if (isCompleteBound(customXDomain)) {
if (customXDomain.min > customXDomain.max) {
Logger.warn('custom xDomain is invalid, min is greater than max. Custom domain is ignored.');
} else {
seriesXComputedDomains = [customXDomain.min, customXDomain.max];
}
} else if (isLowerBound(customXDomain)) {
if (customXDomain.min > computedDomainMax) {
Logger.warn(
'custom xDomain is invalid, custom min is greater than computed max. Custom domain is ignored.',
);
} else {
seriesXComputedDomains = [customXDomain.min, computedDomainMax];
}
} else if (isUpperBound(customXDomain)) {
if (computedDomainMin > customXDomain.max) {
Logger.warn(
'custom xDomain is invalid, computed min is greater than custom max. Custom domain is ignored.',
);
} else {
seriesXComputedDomains = [computedDomainMin, customXDomain.max];
}
}

seriesXComputedDomains = [computedDomainMin, customXDomain.max];
}
}
const computedMinInterval = findMinInterval(values as number[]);
if (customMinInterval != null) {
// Allow greater custom min iff xValues has 1 member.
if (xValues.size > 1 && customMinInterval > computedMinInterval) {
throw new Error('custom xDomain is invalid, custom minInterval is greater than computed minInterval');
}
if (customMinInterval < 0) {
throw new Error('custom xDomain is invalid, custom minInterval is less than 0');
}
}

minInterval = customMinInterval || computedMinInterval;
minInterval = getMinInterval(computedMinInterval, xValues.size, customMinInterval);
}

return {
Expand All @@ -127,6 +123,25 @@ export function mergeXDomain(
};
}

function getMinInterval(computedMinInterval: number, size: number, customMinInterval?: number): number {
if (customMinInterval == null) {
return computedMinInterval;
}
// Allow greater custom min if xValues has 1 member.
if (size > 1 && customMinInterval > computedMinInterval) {
Logger.warn(
'custom xDomain is invalid, custom minInterval is greater than computed minInterval. Using computed minInterval.',
);
return computedMinInterval;
}
if (customMinInterval < 0) {
Logger.warn('custom xDomain is invalid, custom minInterval is less than 0. Using computed minInterval.');
return computedMinInterval;
}

return customMinInterval;
}

/**
* Find the minimum interval between xValues.
* Default to 0 if an empty array, 1 if one item array
Expand Down
29 changes: 19 additions & 10 deletions packages/osd-charts/src/chart_types/xy_chart/domains/y_domain.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,17 @@ import { ScaleType } from '../../../scales/constants';
import { SpecTypes } from '../../../specs/constants';
import { Position } from '../../../utils/commons';
import { BARCHART_1Y0G } from '../../../utils/data_samples/test_dataset';
import { Logger } from '../../../utils/logger';
import { computeSeriesDomainsSelector } from '../state/selectors/compute_series_domains';
import { BasicSeriesSpec, SeriesTypes, DEFAULT_GLOBAL_ID, StackMode } from '../utils/specs';
import { coerceYScaleTypes, groupSeriesByYGroup } from './y_domain';

jest.mock('../../../utils/logger', () => ({
Logger: {
warn: jest.fn(),
},
}));

const DEMO_AREA_SPEC_1 = {
id: 'a',
groupId: 'a',
Expand Down Expand Up @@ -448,12 +455,13 @@ describe('Y Domain', () => {
store,
);

const attemptToMerge = () => {
computeSeriesDomainsSelector(store.getState());
};
const {
yDomain: [{ domain }],
} = computeSeriesDomainsSelector(store.getState());
expect(domain).toEqual([20, 20]);

const errorMessage = 'custom yDomain for a is invalid, custom min is greater than computed max';
expect(attemptToMerge).toThrowError(errorMessage);
const warnMessage = 'custom yDomain for a is invalid, custom min is greater than computed max.';
expect(Logger.warn).toBeCalledWith(warnMessage);
});
test('Should merge Y domain accounting for custom domain limits: partial upper bounded domain', () => {
const store = MockStore.default();
Expand Down Expand Up @@ -496,12 +504,13 @@ describe('Y Domain', () => {
store,
);

const attemptToMerge = () => {
computeSeriesDomainsSelector(store.getState());
};
const {
yDomain: [{ domain }],
} = computeSeriesDomainsSelector(store.getState());
expect(domain).toEqual([-1, -1]);

const errorMessage = 'custom yDomain for a is invalid, computed min is greater than custom max';
expect(attemptToMerge).toThrowError(errorMessage);
const warnMessage = 'custom yDomain for a is invalid, custom max is less than computed max.';
expect(Logger.warn).toBeCalledWith(warnMessage);
});
test('Should merge Y domain with stacked as percentage', () => {
const store = MockStore.default();
Expand Down
15 changes: 8 additions & 7 deletions packages/osd-charts/src/chart_types/xy_chart/domains/y_domain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/

import { GracefulError } from '../../../components/error_boundary/errors';
import { ScaleContinuousType } from '../../../scales';
import { ScaleType } from '../../../scales/constants';
import { identity } from '../../../utils/commons';
Expand Down Expand Up @@ -101,16 +100,18 @@ function mergeYDomainForGroup(
domain = [newCustomDomain.min, newCustomDomain.max];
} else if (newCustomDomain && isLowerBound(newCustomDomain)) {
if (newCustomDomain.min > computedDomainMax) {
throw new GracefulError(`custom yDomain for ${groupId} is invalid, custom min is greater than computed max`);
Logger.warn(`custom yDomain for ${groupId} is invalid, custom min is greater than computed max.`);
domain = [newCustomDomain.min, newCustomDomain.min];
} else {
domain = [newCustomDomain.min, computedDomainMax];
}

domain = [newCustomDomain.min, computedDomainMax];
} else if (newCustomDomain && isUpperBound(newCustomDomain)) {
if (computedDomainMin > newCustomDomain.max) {
throw new Error(`custom yDomain for ${groupId} is invalid, computed min is greater than custom max`);
Logger.warn(`custom yDomain for ${groupId} is invalid, custom max is less than computed max.`);
domain = [newCustomDomain.max, newCustomDomain.max];
} else {
domain = [computedDomainMin, newCustomDomain.max];
}

domain = [computedDomainMin, newCustomDomain.max];
}
}
return {
Expand Down

0 comments on commit f480054

Please sign in to comment.