Skip to content

Commit

Permalink
Prevent rewrite of date_histogram interval (#91408) (#92351)
Browse files Browse the repository at this point in the history 
* Prevent rewrite of date_histogram interval

Closes:#91408

* fix PR comments

* fix PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
@alexwizp @kibanamachine
alexwizp and kibanamachine authored Feb 23, 2021
1 parent 1a45593 commit 05eb021
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 54 deletions.
8 changes: 7 additions & 1 deletion src/plugins/data/common/search/aggs/buckets/lib/time_buckets/calc_es_interval.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,18 @@ export interface EsInterval {
* @param {moment.duration} duration
* @return {object}
*/
export function convertDurationToNormalizedEsInterval(duration: moment.Duration): EsInterval {
export function convertDurationToNormalizedEsInterval(
duration: moment.Duration,
targetUnit?: Unit
): EsInterval {
for (let i = 0; i < unitsDesc.length; i++) {
const unit = unitsDesc[i];
const val = duration.as(unit);
// find a unit that rounds neatly
if (val >= 1 && Math.floor(val) === val) {
if (val === 1 && targetUnit && unit !== targetUnit) {
continue;
}
// if the unit is "large", like years, but
// isn't set to 1 ES will puke. So keep going until
// we get out of the "large" units
Expand Down
66 changes: 31 additions & 35 deletions src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { Assign } from 'utility-types';
import { isString, isObject as isObjectLodash, isPlainObject, sortBy } from 'lodash';
import moment, { Moment } from 'moment';

import { parseInterval } from '../../../utils';
import { Unit } from '@elastic/datemath';
import { parseInterval, splitStringInterval } from '../../../utils';
import { TimeRangeBounds } from '../../../../../query';
import { calcAutoIntervalLessThan, calcAutoIntervalNear } from './calc_auto_interval';
import {
Expand Down Expand Up @@ -38,6 +39,10 @@ function isValidMoment(m: any): boolean {
return m && 'isValid' in m && m.isValid();
}

function isDurationInterval(i: any): i is moment.Duration {
return moment.isDuration(i) && Boolean(+i);
}

export interface TimeBucketsConfig extends Record<string, any> {
'histogram:maxBars': number;
'histogram:barTarget': number;
Expand Down Expand Up @@ -172,38 +177,26 @@ export class TimeBuckets {
* @param {object|string|moment.duration} input - see desc
*/
setInterval(input: null | string | Record<string, any>) {
let interval = input;
this._originalInterval = null;
this._i = isObject(input) ? input.val : input;

// selection object -> val
if (isObject(input) && !moment.isDuration(input)) {
interval = input.val;
}

if (!interval || interval === autoInterval) {
if (!this._i || this._i === autoInterval) {
this._i = autoInterval;
return;
}

if (isString(interval)) {
input = interval;

// Preserve the original units because they're lost when the interval is converted to a
// moment duration object.
this._originalInterval = input;
if (isString(this._i)) {
const parsedInterval = parseInterval(this._i);

interval = parseInterval(interval);
if (interval === null || +interval === 0) {
interval = null;
if (isDurationInterval(parsedInterval)) {
this._originalInterval = this._i;
this._i = parsedInterval;
}
}

// if the value wasn't converted to a duration, and isn't
// already a duration, we have a problem
if (!moment.isDuration(interval)) {
throw new TypeError('"' + input + '" is not a valid interval.');
if (!isDurationInterval(this._i)) {
throw new TypeError('"' + this._i + '" is not a valid interval.');
}

this._i = interval;
}

/**
Expand Down Expand Up @@ -235,15 +228,9 @@ export class TimeBuckets {
*/
getInterval(useNormalizedEsInterval = true): TimeBucketsInterval {
const duration = this.getDuration();

// either pull the interval from state or calculate the auto-interval
const readInterval = () => {
const interval = this._i;
if (moment.isDuration(interval)) return interval;
return calcAutoIntervalNear(this._timeBucketConfig['histogram:barTarget'], Number(duration));
};

const parsedInterval = readInterval();
const parsedInterval = isDurationInterval(this._i)
? this._i
: calcAutoIntervalNear(this._timeBucketConfig['histogram:barTarget'], Number(duration));

// check to see if the interval should be scaled, and scale it if so
const maybeScaleInterval = (interval: moment.Duration) => {
Expand Down Expand Up @@ -271,10 +258,19 @@ export class TimeBuckets {
};

// append some TimeBuckets specific props to the interval
const decorateInterval = (interval: moment.Duration): TimeBucketsInterval => {
const decorateInterval = (
interval: Assign<moment.Duration, { scaled?: boolean }>
): TimeBucketsInterval => {
let originalUnit: Unit | undefined;

if (!interval.scaled && this._originalInterval) {
originalUnit = splitStringInterval(this._originalInterval!)?.unit;
}

const esInterval = useNormalizedEsInterval
? convertDurationToNormalizedEsInterval(interval)
? convertDurationToNormalizedEsInterval(interval, originalUnit)
: convertIntervalToEsInterval(String(this._originalInterval));

const prettyUnits = moment.normalizeUnits(esInterval.unit);

return Object.assign(interval, {
Expand Down
38 changes: 31 additions & 7 deletions src/plugins/data/common/search/aggs/utils/date_interval_utils/parse_interval.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,41 @@
*/

import { Duration, unitOfTime } from 'moment';
import { parseInterval } from './parse_interval';
import { parseInterval, splitStringInterval } from './parse_interval';

describe('splitStringInterval', () => {
test('should correctly split 1d interval', () => {
expect(splitStringInterval('1d')).toMatchInlineSnapshot(`
Object {
"unit": "d",
"value": 1,
}
`);
});

const validateDuration = (duration: Duration | null, unit: unitOfTime.Base, value: number) => {
expect(duration).toBeDefined();
test('should correctly split 34m interval', () => {
expect(splitStringInterval('1d')).toMatchInlineSnapshot(`
Object {
"unit": "d",
"value": 1,
}
`);
});

if (duration) {
expect(duration.as(unit)).toBe(value);
}
};
test('should return null if cannot parse interval', () => {
expect(splitStringInterval('wrong_value_1')).toMatchInlineSnapshot(`null`);
});
});

describe('parseInterval', () => {
const validateDuration = (duration: Duration | null, unit: unitOfTime.Base, value: number) => {
expect(duration).toBeDefined();

if (duration) {
expect(duration.as(unit)).toBe(value);
}
};

describe('integer', () => {
test('should correctly parse 1d interval', () => {
validateDuration(parseInterval('1d'), 'd', 1);
Expand Down
30 changes: 19 additions & 11 deletions src/plugins/data/common/search/aggs/utils/date_interval_utils/parse_interval.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,32 @@
*/

import { find } from 'lodash';
import moment, { unitOfTime } from 'moment';
import dateMath from '@elastic/datemath';
import moment from 'moment';
import dateMath, { Unit } from '@elastic/datemath';

// Assume interval is in the form (value)(unit), such as "1h"
const INTERVAL_STRING_RE = new RegExp('^([0-9\\.]*)\\s*(' + dateMath.units.join('|') + ')$');

export const splitStringInterval = (interval: string) => {
if (interval) {
const matches = interval.toString().trim().match(INTERVAL_STRING_RE);
if (matches) {
return {
value: parseFloat(matches[1]) || 1,
unit: matches[2] as Unit,
};
}
}
return null;
};

export function parseInterval(interval: string): moment.Duration | null {
const matches = String(interval).trim().match(INTERVAL_STRING_RE);
const parsedInterval = splitStringInterval(interval);

if (!matches) return null;
if (!parsedInterval) return null;

try {
const value = parseFloat(matches[1]) || 1;
const unit = matches[2] as unitOfTime.Base;

const { value, unit } = parsedInterval;
const duration = moment.duration(value, unit);

// There is an error with moment, where if you have a fractional interval between 0 and 1, then when you add that
Expand All @@ -31,10 +42,7 @@ export function parseInterval(interval: string): moment.Duration | null {
// adding 0.5 days until we hit the end date. However, since there is a bug in moment, when you add 0.5 days to
// the start date, you get the same exact date (instead of being ahead by 12 hours). So instead of returning
// a duration corresponding to 0.5 hours, we return a duration corresponding to 12 hours.
const selectedUnit = find(
dateMath.units,
(u) => Math.abs(duration.as(u)) >= 1
) as unitOfTime.Base;
const selectedUnit = find(dateMath.units, (u) => Math.abs(duration.as(u)) >= 1) as Unit;

// however if we do this fhe other way around it will also fail
// go from 500m to hours as this will result in infinite number (dividing 500/60 = 8.3*)
Expand Down

0 comments on commit 05eb021

Please sign in to comment.