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.

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

[Maps] support styles on aggregation fields with _of_ in name #51687

Closed
wants to merge 2 commits into from
Closed

[Maps] support styles on aggregation fields with _of_ in name #51687

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb3ed56
[Maps] support styles on aggregation fields with _of_ in name
nreese Nov 26, 2019
638b840
Merge branch 'master' of github.com:elastic/kibana into agg_field_split
nreese Dec 2, 2019
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
32 changes: 3 additions & 29 deletions x-pack/legacy/plugins/maps/public/layers/sources/es_agg_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { ESAggMetricField } from '../fields/es_agg_field';
import { ESDocField } from '../fields/es_doc_field';
import { METRIC_TYPE, COUNT_AGG_TYPE, COUNT_PROP_LABEL, COUNT_PROP_NAME, FIELD_ORIGIN } from '../../../common/constants';

const AGG_DELIMITER = '_of_';
export const AGG_DELIMITER = '_of_';

export class AbstractESAggSource extends AbstractESSource {

Expand Down Expand Up @@ -46,34 +46,8 @@ export class AbstractESAggSource extends AbstractESSource {
}) : [];
}

createField({ fieldName, label }) {

//if there is a corresponding field with a custom label, use that one.
if (!label) {
const matchField = this._metricFields.find(field => field.getName() === fieldName);
if (matchField) {
label = matchField.getLabel();
}
}

if (fieldName === COUNT_PROP_NAME) {
return new ESAggMetricField({
aggType: COUNT_AGG_TYPE,
label: label,
source: this,
origin: this.getOriginForField()
});
}
//this only works because aggType is a fixed set and does not include the `_of_` string
const [aggType, docField] = fieldName.split(AGG_DELIMITER);
const esDocField = new ESDocField({ fieldName: docField, source: this });
return new ESAggMetricField({
label: label,
esDocField,
aggType,
source: this,
origin: this.getOriginForField()
});
createField({ fieldName }) {
Copy link
Contributor

@thomasneirynck thomasneirynck Nov 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Great simplification! The main consequence now is that EsAggSource#createField is no longer a factory-function and creates inconsistencies with the other implementations of VectorSource#createField.

  1. By using the getMetricFieldForName, which only will return previously created objects, this continues to work as expected because all object-instances are guaranteed to be read-only, so reuse is not an issue. But that's implicit and never enforced (ie. redux requires persistence to always flow through the store, and we enforce this through code-review primarily).

  2. ESAggSource#createField can only construct fields that are actually available in the EsAggSource, which is inconsistent with all the other implementations. It is a fine limitation, but implementations should be consistent. e.g. consider EMS-sources disallowing creating fields which are not present in the manifest, ES-source disallowing creating fields which are not present in the index-pattern, etc...).

This was the reason we weren't reusing getMetricFieldForName here originally, instead doing a manual lookup for a custom label first.

  1. createField can no longer be used transparently, regardless of caller. e.g. sometimes it is used in the Source#constructor as a shorthand (which would require creation-of-fields from scratch). Other times, it's used from outside (like here, where the bug arises

    kibana/x-pack/legacy/plugins/maps/public/layers/styles/vector/vector_style.js

    Lines 455 to 457 in 05f765a

    return this._source.createField({
    fieldName: fieldDescriptor.name
    });
    ).

Maybe consider (?)

  • Introduce something like VectorSource#getFieldByName. EsAggSource can implement it just with getMetricFieldByName, the other sources can just delegate to use createField by default.
  • Modify

    kibana/x-pack/legacy/plugins/maps/public/layers/styles/vector/vector_style.js

    Lines 455 to 457 in 05f765a

    return this._source.createField({
    fieldName: fieldDescriptor.name
    });
    to return this._source.getFieldByName(descriptor.name)

return this.getMetricFieldForName(fieldName);
}

getMetricFieldForName(fieldName) {
Expand Down
8 changes: 4 additions & 4 deletions x-pack/legacy/plugins/maps/public/layers/sources/es_term_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import _ from 'lodash';
import { Schemas } from 'ui/vis/editors/default/schemas';
import { AggConfigs } from 'ui/agg_types';
import { i18n } from '@kbn/i18n';
import { ES_SIZE_LIMIT, FIELD_ORIGIN, METRIC_TYPE } from '../../../common/constants';
import { ES_SIZE_LIMIT, FIELD_ORIGIN, METRIC_TYPE, COUNT_PROP_LABEL } from '../../../common/constants';
import { ESDocField } from '../fields/es_doc_field';
import { AbstractESAggSource } from './es_agg_source';
import { AbstractESAggSource, AGG_DELIMITER } from './es_agg_source';

const TERMS_AGG_NAME = 'join';

Expand Down Expand Up @@ -82,12 +82,12 @@ export class ESTermSource extends AbstractESAggSource {
}

formatMetricKey(aggType, fieldName) {
const metricKey = aggType !== METRIC_TYPE.COUNT ? `${aggType}_of_${fieldName}` : aggType;
const metricKey = aggType !== METRIC_TYPE.COUNT ? `${aggType}${AGG_DELIMITER}${fieldName}` : aggType;
return `${FIELD_NAME_PREFIX}${metricKey}${GROUP_BY_DELIMITER}${this._descriptor.indexPatternTitle}.${this._termField.getName()}`;
}

formatMetricLabel(type, fieldName) {
const metricLabel = type !== METRIC_TYPE.COUNT ? `${type} ${fieldName}` : 'count';
const metricLabel = type !== METRIC_TYPE.COUNT ? `${type} ${fieldName}` : COUNT_PROP_LABEL;
return `${metricLabel} of ${this._descriptor.indexPatternTitle}:${this._termField.getName()}`;
}

Expand Down