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.

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

Add AggTypeFilters to filter out aggs in editor #19913

Merged
merged 12 commits into from
Jun 25, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,12 @@
"@kbn/eslint-plugin-license-header": "link:packages/kbn-eslint-plugin-license-header",
"@kbn/plugin-generator": "link:packages/kbn-plugin-generator",
"@kbn/test": "link:packages/kbn-test",
"@types/babel-core": "^6.25.5",
"@types/eslint": "^4.16.2",
"@types/execa": "^0.9.0",
"@types/getopts": "^2.0.0",
"@types/glob": "^5.0.35",
"@types/jest": "^22.2.3",
"@types/listr": "^0.13.0",
"@types/minimatch": "^2.0.29",
"@types/react": "^16.3.14",
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kibana/public/kibana.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import 'uiExports/visTypes';
import 'uiExports/visResponseHandlers';
import 'uiExports/visRequestHandlers';
import 'uiExports/visEditorTypes';
import 'uiExports/visualize';
import 'uiExports/savedObjectTypes';
import 'uiExports/spyModes';
import 'uiExports/fieldFormats';
Expand Down
1 change: 0 additions & 1 deletion src/core_plugins/kibana/public/visualize/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import _ from 'lodash';
import '../saved_visualizations/saved_visualizations';
import 'ui/vis/editors/default/sidebar';
import './agg_filter';
import 'ui/visualize';
import 'ui/collapsible_sidebar';
import 'ui/share';
Expand Down
1 change: 0 additions & 1 deletion src/core_plugins/kibana/public/visualize/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import './styles/main.less';
import './editor/editor';
import './wizard/wizard';
import './editor/agg_filter';
import 'ui/draggable/draggable_container';
import 'ui/draggable/draggable_item';
import 'ui/draggable/draggable_handle';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,12 @@ describe('editor', function () {
]
});

const $el = $('<vis-editor-agg-params agg="agg" group-name="groupName"></vis-editor-agg-params>');
const $el = $('<vis-editor-agg-params agg="agg" vis="vis" group-name="groupName"></vis-editor-agg-params>');
const $parentScope = $injector.get('$rootScope').$new();

agg = $parentScope.agg = vis.aggs.bySchemaName.segment[0];
$parentScope.groupName = 'buckets';
$parentScope.vis = vis;

$compile($el)($parentScope);
$scope = $el.scope();
Expand Down
20 changes: 20 additions & 0 deletions src/ui/public/agg_types/agg_type.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export type AggType = any;
1 change: 1 addition & 0 deletions src/ui/public/agg_types/controls/order_agg.html
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
</div>
<div ng-show="agg.params.orderAgg" class="vis-editor-agg-order-agg">
<vis-editor-agg-params
vis="vis"
agg="agg.params.orderAgg"
ng-if="agg.params.orderAgg"
group-name="'metrics'">
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/agg_types/controls/sub_agg.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<div ng-if="agg.params.metricAgg === 'custom'" class="vis-editor-agg-order-agg">
<ng-form name="customMetricForm">
<vis-editor-agg-params
vis="vis"
agg="agg.params.customMetric"
group-name="'metrics'">
</vis-editor-agg-params>
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/agg_types/controls/sub_metric.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<ng-form name="{{aggType}}Form">
<vis-editor-agg-params
agg="agg.params[aggType]"
vis="vis"
group-name="'{{aggGroup}}'">
</vis-editor-agg-params>
</ng-form>
Expand Down
76 changes: 76 additions & 0 deletions src/ui/public/agg_types/filter/agg_type_filters.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { first } from 'rxjs/operators';
import { AggTypeFilters } from './agg_type_filters';

describe('AggTypeFilters', () => {
let registry: AggTypeFilters;
const vis = {};
const aggConfig = {};

beforeEach(() => {
registry = new AggTypeFilters();
});

it('should filter nothing without registered filters', async () => {
const aggTypes = [{ name: 'count' }, { name: 'sum' }];
const observable = registry.filter$(aggTypes, vis, aggConfig);
const filtered = await observable.pipe(first()).toPromise();
expect(filtered).toEqual(aggTypes);
});

it('should emit a new filtered list when registering a new filter', async () => {
const aggTypes = [{ name: 'count' }, { name: 'sum' }];
const observable = registry.filter$(aggTypes, vis, aggConfig);
const spy = jest.fn();
observable.subscribe(spy);
expect(spy).toHaveBeenCalledTimes(1);
registry.addFilter(() => true);
expect(spy).toHaveBeenCalledTimes(2);
});

it('should pass all aggTypes to the registered filter', async () => {
const aggTypes = [{ name: 'count' }, { name: 'sum' }];
const filter = jest.fn();
registry.addFilter(filter);
await registry
.filter$(aggTypes, vis, aggConfig)
.pipe(first())
.toPromise();
expect(filter).toHaveBeenCalledWith(aggTypes[0], vis, aggConfig);
expect(filter).toHaveBeenCalledWith(aggTypes[1], vis, aggConfig);
});

it('should allow registered filters to filter out aggTypes', async () => {
const aggTypes = [{ name: 'count' }, { name: 'sum' }, { name: 'avg' }];
const observable = registry.filter$(aggTypes, vis, aggConfig);
let filtered = await observable.pipe(first()).toPromise();
expect(filtered).toEqual(aggTypes);

registry.addFilter(() => true);
registry.addFilter(aggType => aggType.name !== 'count');
filtered = await observable.pipe(first()).toPromise();
expect(filtered).toEqual([aggTypes[1], aggTypes[2]]);

registry.addFilter(aggType => aggType.name !== 'avg');
filtered = await observable.pipe(first()).toPromise();
expect(filtered).toEqual([aggTypes[1]]);
});
});
78 changes: 78 additions & 0 deletions src/ui/public/agg_types/filter/agg_type_filters.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import { BehaviorSubject } from 'rxjs';
import { map } from 'rxjs/operators';
import { AggType } from '..';
import { AggConfig, Vis } from '../../vis';

type AggTypeFilter = (
aggType: AggType,
vis: Vis,
aggConfig: AggConfig
) => boolean;

/**
* A registry to store {@link AggTypeFilter} which are used to filter down
* available aggregations for a specific visualization and {@link AggConfig}.
*/
class AggTypeFilters {
private filters = new Set<AggTypeFilter>();
private subject = new BehaviorSubject<Set<AggTypeFilter>>(this.filters);

/**
* Register a new {@link AggTypeFilter} with this registry.
* This will emit a new set of filtered aggTypes on every Observer returned
* by the {@link #filter$|filter method}.
*
* @param filter The filter to register.
*/
public addFilter(filter: AggTypeFilter): void {
this.filters.add(filter);
this.subject.next(this.filters);
}

/**
* Returns an Observable that will emit a filtered list of the passed {@link AggType|aggTypes}.
* A new filtered list will always be emitted when the {@link AggTypeFilter}
* registered with this registry will change.
*
* @param aggTypes A list of aggTypes that will be filtered down by this registry.
* @param vis The vis for which this list should be filtered down.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to indexPattern

* @param aggConfig The aggConfig for which the returning list will be used.
* @return A filtered list of the passed aggTypes.
*/
public filter$(aggTypes: AggType[], vis: Vis, aggConfig: AggConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I spent a lot of time reading this line by line to parse out what it was doing. I think if we changed some names and assigned some variables we can make the logic a little more accessible. Here's what I suggest:

  public allowedAggTypes$(aggTypes: AggType[], vis: Vis, aggConfig: AggConfig) {
    return this.subject.map(filters => {
      const aggTypeFilters = Array.from(filters);
      const allowedAggTypes = aggTypes.filter(aggType => {
        const isAggTypeAllowed = aggTypeFilters.every(aggTypeFilter => aggTypeFilter(aggType, vis, aggConfig));
        return isAggTypeAllowed;
      });
      return allowedAggTypes;
    });
  }

return this.subject.pipe(
map(filters => {
const allFilters = Array.from(filters);
const allowedAggTypes = aggTypes.filter(aggType => {
const isAggTypeAllowed = allFilters.every(filter =>
filter(aggType, vis, aggConfig)
);
return isAggTypeAllowed;
});
return allowedAggTypes;
})
);
}
}

const aggTypeFilters = new AggTypeFilters();

export { aggTypeFilters, AggTypeFilters };
20 changes: 20 additions & 0 deletions src/ui/public/agg_types/filter/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { aggTypeFilters } from './agg_type_filters';
20 changes: 20 additions & 0 deletions src/ui/public/agg_types/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { AggType } from './agg_type';
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@
* under the License.
*/

import { propFilter } from 'ui/filters/_prop_filter';
import { uiModules } from 'ui/modules';
type FilterFunc = <I>(item: I) => boolean;

uiModules
.get('kibana')
.filter('aggFilter', function () {
return propFilter('name');
});
export const propFilter: (
prop: string
) => <T>(list: T[], filters: string[] | string | FilterFunc) => T[];
20 changes: 20 additions & 0 deletions src/ui/public/vis/agg_config.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export type AggConfig = any;
3 changes: 2 additions & 1 deletion src/ui/public/vis/editors/default/__tests__/agg_params.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,11 @@ describe('Vis-Editor-Agg-Params plugin directive', function () {
});

$parentScope.agg = new AggConfig(vis, state);
$parentScope.vis = vis;

// make the element
$elem = angular.element(
`<vis-editor-agg-params agg="agg" group-name="groupName"></vis-editor-agg-params>`
`<vis-editor-agg-params vis="vis" agg="agg" group-name="groupName"></vis-editor-agg-params>`
);

// compile the html
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/vis/editors/default/agg.html
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
agg="agg"
group-name="groupName"
ng-show="editorOpen"
vis="vis"
class="vis-editor-agg-editor">
</vis-editor-agg-params>

Expand Down
11 changes: 10 additions & 1 deletion src/ui/public/vis/editors/default/agg_params.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { aggTypes } from '../../../agg_types';
import { uiModules } from '../../../modules';
import { documentationLinks } from '../../../documentation_links/documentation_links';
import aggParamsTemplate from './agg_params.html';
import { aggTypeFilters } from '../../../agg_types/filter';

uiModules
.get('app/visualize')
Expand All @@ -39,8 +40,16 @@ uiModules
link: function ($scope, $el, attr) {
$scope.$bind('agg', attr.agg);
$scope.$bind('groupName', attr.groupName);
$scope.$bind('vis', attr.vis);

const aggTypeSubscription = aggTypeFilters
Copy link
Contributor

@cjcenizal cjcenizal Jun 15, 2018

Choose a reason for hiding this comment

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

How important/valuable is it that we're exposing observables through the interface here? For example, I can also picture this functionality being supported by an interface that looked like this (while still using observables internally):

const aggTypeSubscription = new AggTypeSubscription(aggTypes.byType[$scope.groupName], $scope.vis, $scope.agg);
aggTypeSubscription.subscribe(aggTypes => $scope.aggTypeOptions = aggTypes);

This would encapsulate the complexity of observables and present me with an interface which appears and behaves like an event emitter.

Copy link
Contributor Author

@timroes timroes Jun 15, 2018

Choose a reason for hiding this comment

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

If we are anyway working with observables I think we are actually also want to expose this for a couple of reasons:

  1. Otherwise we also need to build deregistration logic in that interface.
  2. Observables are super powerful and allow tons of operators, so if the actual consumer of that API need to postprocess the answer in anyway or want to debounce it, or just listen for the first changes, etc. - there is already an Observable operator for that. I don't see a good reason why we would want to deliberately kill that flexibility and possibly force the user to wrap everything in an observable again to be able to achieve that operations.

Since Observables and RxJS tend to become a common pattern (and tend to become a standard JavaScript pattern) I would rather only hide Observables in places we use them, if there is a really good reason for doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a couple thoughts in response, but keep in mind I'm coming to this with zero experience in observables, so I'm mostly asking questions to learn and not with any strongly held opinion. I would not consider my comments a blocker on this PR in any way.

Observables are super powerful and allow tons of operators

100% agree, from what I've learned about them so far their power is apparent. But this is why I'm trying to learn more about why this power is necessary, since you don't want to use a tank when all you need is a bicycle. In this particular case, it doesn't seem like we're taking advantage of the observable. Are there concrete use cases for the aggTypeFilters module in which this power is necessary on the consumer's side, or examples of similar modules elsewhere that make the advantages more obvious?

I don't see a good reason why we would want to deliberately kill that flexibility

Just to explain my thought process, I look for the abstractions we're creating in our code and try to figure out what kind of model can be inferred from them. So I poke and probe at abstractions that are complex and try to understand if that complexity is necessary. An example of where unnecessary complexity in abstractions created a confusing model would be our courier code. I was thinking that if it turns out that the observable is really just an implementation detail, then it might make sense to reduce complexity for the consumer by hiding it.

Since Observables and RxJS tend to become a common pattern

No argument here. 😄 I agree that observables can be really useful and I'm sure there are situations where an abstraction that deals in observables makes sense. I'm just trying to understand why they're a core part of this abstraction's interface. I have very little experience with observables, so I'm really just trying to learn more about the situations in which they're most useful and understand how this is one of those situations.

.filter$(aggTypes.byType[$scope.groupName], $scope.vis, $scope.agg)
.subscribe(aggTypes => $scope.aggTypeOptions = aggTypes);

$scope.$on('$destroy', () => {
aggTypeSubscription.unsubscribe();
});

$scope.aggTypeOptions = aggTypes.byType[$scope.groupName];
$scope.advancedToggled = false;

// We set up this watch prior to adding the controls below, because when the controls are added,
Expand Down
1 change: 0 additions & 1 deletion src/ui/public/vis/editors/default/agg_select.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
</ui-select-match>
<ui-select-choices
repeat="agg in aggTypeOptions
| aggFilter:agg.schema.aggFilter
| filter: { title: $select.search }
| orderBy:'title'
| sortPrefixFirst:$select.search:'title'"
Expand Down
1 change: 1 addition & 0 deletions src/ui/public/vis/editors/default/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import './sidebar';
import './vis_options';
import './vis_editor_resizer';
import './vis_type_agg_filter';
import $ from 'jquery';

import _ from 'lodash';
Expand Down
Loading