[SavedObjects] Add aggregations support #96292
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Rudolf Meijering <[email protected]>
…aved-objects-aggs
pgayvallet
commented
Apr 8, 2021
Comment on lines
+46
to
+51
| filter: s.object({ | ||
| term: s.recordOf(s.string(), s.oneOf([s.string(), s.boolean(), s.number()])), | ||
| }), | ||
| histogram: s.object({ | ||
| field: s.maybe(s.string()), | ||
| interval: s.maybe(s.number()), |
There was a problem hiding this comment.
Some fields / shapes could easily be factorized (e.g extended_bounds and hard_bounds are the same TS type in AggregationContainer), but I'd rather do that later when we'll get some feedback on the usability of the API.
Comment on lines
+112
to
+120
| } else if (ast.type === 'literal' && ast.value && typeof ast.value === 'string') { | ||
| const key = ast.value.replace('.attributes', ''); | ||
| const mappingKey = 'properties.' + key.split('.').join('.properties.'); | ||
| const field = get(indexMapping, mappingKey); | ||
| if (field != null && field.type === 'nested') { | ||
| localNestedKeys = ast.value; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Quite out of the scope of the main goal of the PR, but the initial PR was also fixing #81009 via this change, so I kept it.
Tell me if we want to properly extract this change to its own PR instead.
There was a problem hiding this comment.
it is just a little bundle!
Comment on lines
+60
to
+67
| export const isObjectTypeAttribute = ( | ||
| attributePath: string, | ||
| indexMapping: IndexMapping, | ||
| allowedTypes: string[] | ||
| ): boolean => { | ||
| const error = hasFilterKeyError(attributePath, allowedTypes, indexMapping); | ||
| return error == null; | ||
| }; |
There was a problem hiding this comment.
The logic behind that function is quite complex, and was already implemented for KQL filters. However it did not want to directly use hasFilterKeyError in the higher-level validation, so I wrapped it and added specific unit tests, in case we need to change the underlying implementation.
Same goes with isRootLevelAttribute and the call to fieldDefined
Comment on lines
+176
to
+177
| export interface SavedObjectsFindResponse<T = unknown, A = unknown> { | ||
| aggregations?: A; |
There was a problem hiding this comment.
We can't (at least easily) change SavedObjectsFindResponse to have the aggregations property only defined when the aggs input parameter was provided.
The initial PR defined it as an optional property. I wonder if having the property always present and populating it with {} when no aggs input is provided wouldn't be a better developer experience, as atm any usage of response.aggregations requires to check for existence or use !. OTOH removing the optional nature of the property may force to adapt a lot of test code returning 'mock' responses. wdyt?
There was a problem hiding this comment.
My 2cents: I would aim for a better DX. But there's already a lot going on in this PR. We can do follow-ups to improve the typing logic.
Comment on lines
+137
to
+138
| * | ||
| * @alpha |
There was a problem hiding this comment.
Original PR flagged the option with @alpha. I don't think we're using that elsewhere in core, especially on an option instead of an API. Should we keep it, or should we just document the aggs option as experimental?
There was a problem hiding this comment.
I dont mind the @alpha tag but IMO we should document the option as experimental regardless
Comment on lines
+297
to
+308
| it('should return 200 with valid response for a valid aggregation', async () => | ||
| await supertest | ||
| .get( | ||
| `/api/saved_objects/_find?type=visualization&per_page=0&aggs=${encodeURIComponent( | ||
| JSON.stringify({ | ||
| type_count: { max: { field: 'visualization.attributes.version' } }, | ||
| }) | ||
| )}` | ||
| ) | ||
| .expect(200) | ||
| .then((resp) => { | ||
| expect(resp.body).to.eql({ |
There was a problem hiding this comment.
Wasn't sure if it made sense to add too much api integration tests, as the feature is already correctly unit tested and as there is a thin line between testing the feature and testing the ES aggregation API. Tell me if you see tests I should be adding here.
Comment on lines
+38
to
+43
| export const validateAndConvertAggregations = ( | ||
| allowedTypes: string[], | ||
| aggs: Record<string, estypes.AggregationContainer>, | ||
| indexMapping: IndexMapping | ||
| ): Record<string, estypes.AggregationContainer> => { | ||
| return validateAggregations(aggs, { |
There was a problem hiding this comment.
Please have as much fun reviewing this file as I did writing it. (and no, I did NOT use a pure visitor pattern here, the aggrRecord / aggrStructure two stages validation is way easier to both understand and maintain)
|
Pinging @elastic/kibana-core (Team:Core) |
azasypkin
approved these changes
Apr 8, 2021
afharo
reviewed
Apr 9, 2021
src/core/server/saved_objects/service/lib/aggregations/validation.ts
Outdated
Show resolved
Hide resolved
src/core/server/saved_objects/service/lib/aggregations/validation_utils.ts
Show resolved
Hide resolved
Comment on lines
+176
to
+177
| export interface SavedObjectsFindResponse<T = unknown, A = unknown> { | ||
| aggregations?: A; |
There was a problem hiding this comment.
My 2cents: I would aim for a better DX. But there's already a lot going on in this PR. We can do follow-ups to improve the typing logic.
pgayvallet
added a commit
to pgayvallet/kibana
that referenced
this pull request
Apr 16, 2021
* step 1 to add aggs in the find function of saved object * setp 2 - add specific unit test to aggs + fix bug found during integrations * step 3 - add security api_integration arounds aggs * fix types * unit test added for aggs_utils * add documentation * fix docs * review I * doc * try to fix test * add the new property to the saved object globaltype * fix types * delete old files * fix types + test api integration * type fix + test * Update src/core/server/saved_objects/types.ts Co-authored-by: Rudolf Meijering <[email protected]> * review I * change our validation to match discussion with Pierre and Rudolph * Validate multiple items nested filter query through KueryNode * remove unused import * review + put back test * migrate added tests to new TS file * fix documentation * fix license header * move stuff * duplicating test mappings * rename some stuff * move ALL the things * cast to aggregation container * update generated doc * add deep nested validation * rewrite the whole validation mechanism * some cleanup * minor cleanup * update generated doc * adapt telemetry client * fix API integ tests * fix doc * TOTO-less * remove xpack tests * list supported / unsupported aggregations * typo fix * extract some validation function * fix indent * add some unit tests * adapt FTR assertions * update doc * fix doc * doc again * cleanup test names * improve tsdoc on validation functions * perf nit Co-authored-by: Xavier Mouligneau <[email protected]> Co-authored-by: Rudolf Meijering <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
pgayvallet
added a commit
that referenced
this pull request
Apr 16, 2021
* step 1 to add aggs in the find function of saved object * setp 2 - add specific unit test to aggs + fix bug found during integrations * step 3 - add security api_integration arounds aggs * fix types * unit test added for aggs_utils * add documentation * fix docs * review I * doc * try to fix test * add the new property to the saved object globaltype * fix types * delete old files * fix types + test api integration * type fix + test * Update src/core/server/saved_objects/types.ts Co-authored-by: Rudolf Meijering <[email protected]> * review I * change our validation to match discussion with Pierre and Rudolph * Validate multiple items nested filter query through KueryNode * remove unused import * review + put back test * migrate added tests to new TS file * fix documentation * fix license header * move stuff * duplicating test mappings * rename some stuff * move ALL the things * cast to aggregation container * update generated doc * add deep nested validation * rewrite the whole validation mechanism * some cleanup * minor cleanup * update generated doc * adapt telemetry client * fix API integ tests * fix doc * TOTO-less * remove xpack tests * list supported / unsupported aggregations * typo fix * extract some validation function * fix indent * add some unit tests * adapt FTR assertions * update doc * fix doc * doc again * cleanup test names * improve tsdoc on validation functions * perf nit Co-authored-by: Xavier Mouligneau <[email protected]> Co-authored-by: Rudolf Meijering <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Xavier Mouligneau <[email protected]> Co-authored-by: Rudolf Meijering <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
rudolf
reviewed
Apr 16, 2021
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/feature_controls·ts.apis SecuritySolution Endpoints feature controls APIs can't be accessed by user with dashboard "all" privilegesStandard OutStack TraceKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/feature_controls·ts.apis SecuritySolution Endpoints feature controls APIs can't be accessed by user with dashboard "all" privilegesStandard OutStack TraceMetrics [docs]Page load bundle
Unknown metric groupsAPI count
API count missing comments
History
To update your PR or re-run it, just comment with: |
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Apr 19, 2021
…te-legacy-es-client * 'master' of github.com:elastic/kibana: (102 commits) [Exploratory view] integerate page views to exploratory view (elastic#97258) Fix typo in license_api_guard README name and import http server mocks from public interface (elastic#97334) Avoid mutating KQL query when validating it (elastic#97081) Add description as title on tag badge (elastic#97109) Remove legacy ES client usages in `home` and `xpack_legacy` (elastic#97359) [Fleet] Finer-grained error information from install/upgrade API (elastic#95649) Rule registry bundle size (elastic#97251) [Partial Results] Move other bucket into Search Source (elastic#96384) [Dashboard] Makes lens default editor for creating new panels (elastic#96181) skip flaky suite (elastic#97387) [Asset Management] Agent picker follow up (elastic#97357) skip flaky suite (elastic#97382) [Security Solutions] Fixes flake with cypress tests (elastic#97329) skip flaky suite (elastic#97355) Skip test to try and stabilize master minimize number of so fild asserted in tests. it creates flakines when implementation details change (elastic#97374) [Search Sessions] Client side search cache (elastic#92439) [SavedObjects] Add aggregations support (elastic#96292) [Reporting] Remove legacy elasticsearch client usage from the reporting plugin (elastic#97184) [kbnClient] fix basePath handling and export reponse type (elastic#97277) ... # Conflicts: # x-pack/plugins/watcher/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts # x-pack/plugins/watcher/server/plugin.ts # x-pack/plugins/watcher/server/routes/api/indices/register_get_route.ts # x-pack/plugins/watcher/server/routes/api/license/register_refresh_route.ts # x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts # x-pack/plugins/watcher/server/routes/api/register_load_history_route.ts # x-pack/plugins/watcher/server/routes/api/settings/register_load_route.ts # x-pack/plugins/watcher/server/routes/api/watch/action/register_acknowledge_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_activate_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_deactivate_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_delete_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_execute_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_history_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_load_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_save_route.ts # x-pack/plugins/watcher/server/routes/api/watch/register_visualize_route.ts # x-pack/plugins/watcher/server/routes/api/watches/register_delete_route.ts # x-pack/plugins/watcher/server/routes/api/watches/register_list_route.ts # x-pack/plugins/watcher/server/shared_imports.ts # x-pack/plugins/watcher/server/types.ts
This was referenced Nov 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Taking over @XavierM's amazing work in #64002.
This PR adds an initial ES aggregation support to the savedObjects
findAPI.Closes #81009
Technical design and aggregation validation
Implementing aggs for saved objects comes with two main technical challenges:
Using the SO abstraction schema
We don't want to leak the actual underlying ES schema when performing aggs on saved objects, so we have to validate and rewrite the attribute paths inside the aggregation terms, similarly to what we did when rewriting KQL queries provided via the
filteroption of thefindAPI.{type}.{rootField}syntax must be used.e.g
dashboard.updated_at{type}.attributes.{attributePath}syntax must be used.e.g
alert.attributes.actions.groupAvoiding security risks
Aggregations are a very powerful ES feature, but exposing the whole API would come with some security risks that we need to avoid. For example, we want to forbid the usage of the
scriptoption available for some aggregations to block this potential attack vector that consumers of the SO api could use.After discussions with the Core and Security teams, the consensus was to use an allowlist for the attributes. The main argument being that using a blocklist instead could potentially lead to forget to add 'dangerous' fields to the blocklist when implementing new aggregations.
In practice, that means we are validating the structure of all implemented aggregations to ensure that only the allowed subset of the aggregations properties are used. This is done using self-maintained schemas for each implemented aggregation, and explains why all aggregations are not directly usable with this initial PR.
Supported aggregations
The initial set of supported aggregations is:
Adding new aggregations is quite straightforward and only requires implementing the associated validation schema.
Usage examples
Aggregating on type attributes
Similar to the
filtersyntax, querying on attribute fields must be done using the{type}.attributes.{attrPath}syntax.Aggregating on root attributes
Similar to the
filtersyntax, querying on attribute fields must be done using the{type}.{attr}syntax.Nested aggregations
Apart from the special field syntax and the ban of some aggregation options, the whole aggs API is available, such as nested aggregations.
Type support
Input
The
SavedObjectFindOptions.aggsis leveraging theestypes.AggregationContainertype coming from the ES client library (@elastic/elasticsearch), meaning that there is full typescript support on the shape of the aggregations.NOTE ON BANNED FIELDS: We're directly using the es library types, meaning that for typescript, banned fields such as
scriptare valid. Using such fields will only cause failures during runtime. This is an acceptable compromise as we just don't want to expose our own custom types for aggregations.Output
There isn't currently any type inference between the input and output of an aggregation (as the ES lib doesn't support it), meaning that
response.aggregationhas no default shape. a second generic parameter has been added toSavedObjectClient.findto let consumers specify the expected shape of the aggregation response.Checklist