Skip to content

Hides advanced json for count metric #74636

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Aug 31, 2020
Merged

Hides advanced json for count metric #74636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6a32f32
remove advanced json for count agg
stratoula Aug 10, 2020
3bb8102
Remove only advanced json from count agg
stratoula Aug 10, 2020
e6e12f1
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 12, 2020
370d624
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 14, 2020
2e92418
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 17, 2020
dc495c3
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 19, 2020
49c4bb3
use Constant from data plugin
stratoula Aug 19, 2020
bf0f6fa
add the logic to data plugin
stratoula Aug 20, 2020
d3a52a9
remove json arg from function definition
stratoula Aug 21, 2020
4424f0d
Merge branch 'master' into count-agg-remove-advanced-json
stratoula Aug 21, 2020
54ac39e
remove unecessary translations
stratoula Aug 21, 2020
73747e3
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 21, 2020
31bfadc
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 23, 2020
250e208
Merge branch 'master' into count-agg-remove-advanced-json
elasticmachine Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/plugins/data/common/search/aggs/agg_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ describe('AggType Class', () => {
expect(aggType.params[1].name).toBe('customLabel');
});

test('disables json param', () => {
const aggType = new AggType({
name: 'name',
title: 'title',
json: false,
});

expect(aggType.params.length).toBe(1);
expect(aggType.params[0].name).toBe('customLabel');
});

test('can disable customLabel', () => {
const aggType = new AggType({
name: 'smart agg',
Expand Down
17 changes: 11 additions & 6 deletions src/plugins/data/common/search/aggs/agg_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export interface AggTypeConfig<
getRequestAggs?: ((aggConfig: TAggConfig) => TAggConfig[]) | (() => TAggConfig[] | void);
getResponseAggs?: ((aggConfig: TAggConfig) => TAggConfig[]) | (() => TAggConfig[] | void);
customLabels?: boolean;
json?: boolean;
decorateAggConfig?: () => any;
postFlightRequest?: (
resp: any,
Expand Down Expand Up @@ -235,13 +236,17 @@ export class AggType<
if (config.params && config.params.length && config.params[0] instanceof BaseParamType) {
this.params = config.params as TParam[];
} else {
// always append the raw JSON param
// always append the raw JSON param unless it is configured to false
const params: any[] = config.params ? [...config.params] : [];
params.push({
name: 'json',
type: 'json',
advanced: true,
});

if (config.json !== false) {
params.push({
name: 'json',
type: 'json',
advanced: true,
});
}

// always append custom label

if (config.customLabels !== false) {
Expand Down
1 change: 1 addition & 0 deletions src/plugins/data/common/search/aggs/metrics/count.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const getCountMetricAgg = () =>
defaultMessage: 'Count',
}),
hasNoDsl: true,
json: false,
makeLabel() {
return i18n.translate('data.search.aggs.metrics.countLabel', {
defaultMessage: 'Count',
Expand Down
14 changes: 0 additions & 14 deletions src/plugins/data/common/search/aggs/metrics/count_fn.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,12 @@ describe('agg_expression_functions', () => {
"id": undefined,
"params": Object {
"customLabel": undefined,
"json": undefined,
},
"schema": undefined,
"type": "count",
},
}
`);
});

test('correctly parses json string argument', () => {
const actual = fn({
json: '{ "foo": true }',
});

expect(actual.value.params.json).toEqual({ foo: true });
expect(() => {
fn({
json: '/// intentionally malformed json ///',
});
}).toThrowErrorMatchingInlineSnapshot(`"Unable to parse json argument string"`);
});
});
});
12 changes: 1 addition & 11 deletions src/plugins/data/common/search/aggs/metrics/count_fn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import { i18n } from '@kbn/i18n';
import { ExpressionFunctionDefinition } from 'src/plugins/expressions/common';
import { AggExpressionType, AggExpressionFunctionArgs, METRIC_TYPES } from '../';
import { getParsedValue } from '../utils/get_parsed_value';

const fnName = 'aggCount';

Expand Down Expand Up @@ -55,12 +54,6 @@ export const aggCount = (): FunctionDefinition => ({
defaultMessage: 'Schema to use for this aggregation',
}),
},
json: {
types: ['string'],
help: i18n.translate('data.search.aggs.metrics.count.json.help', {
defaultMessage: 'Advanced json to include when the agg is sent to Elasticsearch',
}),
},
customLabel: {
types: ['string'],
help: i18n.translate('data.search.aggs.metrics.count.customLabel.help', {
Expand All @@ -78,10 +71,7 @@ export const aggCount = (): FunctionDefinition => ({
enabled,
schema,
type: METRIC_TYPES.COUNT,
params: {
...rest,
json: getParsedValue(args, 'json'),
},
params: rest,
Copy link
Member

@lukeelmers lukeelmers Aug 20, 2020

Choose a reason for hiding this comment

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

We should remove the json argument from the function definition above as well, since it's no longer being used.

Note that this might cause a TS error because the function definition is using AggExpressionFunctionArgs which is pulling the relevant type from the AggParamsMapping. For count, the value used is BaseAggParams which includes json:

kibana/src/plugins/data/common/search/aggs/types.ts

Line 138 in bf0f6fa

[METRIC_TYPES.COUNT]: BaseAggParams;

If TS gives you trouble, you may need to define an AggParamsMetricCount in the count.ts file without extending BaseAggParams as others agg types do.

Copy link
Contributor Author

@stratoula stratoula Aug 21, 2020

Choose a reason for hiding this comment

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

Thanx @lukeelmers , I missed that, TS looks happy btw :)

},
};
},
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/vis_default_editor/public/components/agg_params_helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
IAggType,
IndexPattern,
IndexPatternField,
} from 'src/plugins/data/public';
} from '../../../data/public';
import { filterAggTypes, filterAggTypeFields } from '../agg_filters';
import { groupAndSortBy, ComboBoxGroupedOptions } from '../utils';
import { AggTypeState, AggParamsState } from './agg_params_state';
Expand Down
5 changes: 5 additions & 0 deletions test/functional/apps/visualize/_point_series_options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export default function ({ getService, getPageObjects }) {
const retry = getService('retry');
const kibanaServer = getService('kibanaServer');
const browser = getService('browser');
const testSubjects = getService('testSubjects');
const PageObjects = getPageObjects([
'visualize',
'header',
Expand Down Expand Up @@ -148,6 +149,10 @@ export default function ({ getService, getPageObjects }) {
});
});

it('should not show advanced json for count agg', async function () {
await testSubjects.missingOrFail('advancedParams-1');
});

it('should put secondary axis on the right', async function () {
const length = await PageObjects.visChart.getRightValueAxes();
expect(length).to.be(1);
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/ja-JP.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1115,7 +1115,6 @@
"data.search.aggs.metrics.count.customLabel.help": "このアグリゲーションのカスタムラベルを表します",
"data.search.aggs.metrics.count.enabled.help": "このアグリゲーションが有効かどうかを指定します",
"data.search.aggs.metrics.count.id.help": "このアグリゲーションのID",
"data.search.aggs.metrics.count.json.help": "アグリゲーションがElasticsearchに送信されるときに含める高度なJSON",
"data.search.aggs.metrics.count.schema.help": "このアグリゲーションで使用するスキーマ",
"data.search.aggs.metrics.countLabel": "カウント",
"data.search.aggs.metrics.countTitle": "カウント",
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/translations/translations/zh-CN.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,7 +1116,6 @@
"data.search.aggs.metrics.count.customLabel.help": "表示此聚合的定制标签",
"data.search.aggs.metrics.count.enabled.help": "指定是否启用此聚合",
"data.search.aggs.metrics.count.id.help": "此聚合的 ID",
"data.search.aggs.metrics.count.json.help": "聚合发送至 Elasticsearch 时要包括的高级 json",
"data.search.aggs.metrics.count.schema.help": "要用于此聚合的方案",
"data.search.aggs.metrics.countLabel": "计数",
"data.search.aggs.metrics.countTitle": "计数",
Expand Down