handling references for kibana_context and get_index_pattern expression functions #95224

ppisljar · 2021-03-23T18:53:47Z

Summary

Handling reference injection and extraction for kibana_context and getIndexPattern expression functions

kibana_context function was refactored so it no longer depends on getSavedObject handler
getSavedObject handler was removed from expression handlers
kibana_context function now extracts and injects reference to the saved search from its savedSearchId parameter
getIndexPattern function now extracts and injects reference to the saved index pattern from its id parameter
expression executor was updated to correctly call extract and inject on functions

resolves #59960

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-03-23T18:53:49Z

Pinging @elastic/kibana-app-services (Team:AppServices)

dokmic · 2021-03-24T14:02:44Z

src/plugins/data/common/index_patterns/expressions/load_index_pattern.ts

+        id: state.id[0] as string,
+      },
+    ];
+    state.id[0] = refName;


Is that okay that the state object is mutable? Wouldn't it cause any side effects?

i'll update it to not mutate

src/plugins/data/common/index_patterns/expressions/load_index_pattern.ts

dokmic · 2021-03-24T14:12:51Z

src/plugins/data/common/search/expressions/kibana_context.ts

+
+    extract(state) {
+      const references: SavedObjectReference[] = [];
+      if (state.savedSearchId.length && typeof state.savedeSearchId[0] === 'string') {


The first clause is redundant because the typeof will return undefined for an unexisting item.

Suggested change

if (state.savedSearchId.length && typeof state.savedeSearchId[0] === 'string') {

if (typeof state.savedeSearchId[0] === 'string') {

dokmic · 2021-03-24T14:14:49Z

src/plugins/data/server/search/expressions/kibana_context.ts

+  getStartServices: StartServicesAccessor<DataPluginStartDependencies, DataPluginStart>;
+}) {
+  return getKibanaContextFn(async (getKibanaRequest) => {
+    if (!getKibanaRequest || !getKibanaRequest()) {


Suggested change

if (!getKibanaRequest || !getKibanaRequest()) {

if (!getKibanaRequest?.()) {

dokmic

I just left a few minor suggestions, but the code looks good to me.

streamich

Checked on Chrome/Mac, LGTM, except the savedSearchId property check, see below.

src/plugins/data/public/search/expressions/kibana_context.ts

src/plugins/data/common/search/expressions/kibana_context.ts

streamich · 2021-03-24T14:34:08Z

src/plugins/data/common/search/expressions/kibana_context.ts

-    const timeRange = args.timeRange || input?.timeRange;
-    let queries = mergeQueries(input?.query, args?.q || []);
-    let filters = [...(input?.filters || []), ...(args?.filters?.map(unboxExpressionValue) || [])];
+    async fn(input, args, { getKibanaRequest }) {


I'm a bit confused about what does getKibanaRequest resolve to, as KibanaRequest is the server-side type and makes sense only on the server.

Not specific to this PR:

Also, I'm skeptical about using KibanaRequest type in expressions plugin. Maybe there is a way to create our own wrapper around it, call it ExpressionUserContext.

The problem I see with it is that it assumes that you can authenticate only using KibanaRequest. Also, it exports KibanaRequest in ExecutionContext interface from expressions plugin, making it our public API, but when Core will want to change something in KibanaRequest, they might need to do changes to expressions plugin and it might be breaking change in expressions plugin. Just some thoughts.

getKibanaRequest on the client will be undefined, but the getStartServices function on the client doesn't expect it. this is just so we can keep the same code in common.

i agree the whole kibana request thing is a bit weird and we should probably do somethin about it in the future

src/plugins/data/server/search/expressions/kibana_context.ts

streamich · 2021-03-24T14:41:53Z

src/plugins/data/server/search/expressions/kibana_context.ts

+  return getKibanaContextFn(async (getKibanaRequest) => {
+    if (!getKibanaRequest || !getKibanaRequest()) {
+      throw new Error(
+        i18n.translate('data.search.kibana_context.error.kibanaRequest', {


This seems like an error message for Kibana developers, and this error should never happen, it looks like a check just in case a developer made a mistake. In that case, shall we skip translation here, especially considering that this code executes on the server.

If we still want to translate it, maybe better throw an error code, like

throw new Error('KIBANA_REQUEST_MISSING');

and then translate the error on the client-side.

src/plugins/data/server/search/expressions/kibana_context.ts

Co-authored-by: Vadim Dalecky <streamich@gmail.com>

Co-authored-by: Michael Dokolin <dokmic@gmail.com>

# Conflicts: # src/plugins/data/server/search/search_service.ts

kibanamachine · 2021-03-25T10:03:11Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	625	626	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	788.1KB	789.0KB	+851.0B
`expressions`	193.3KB	193.4KB	+57.0B
total			+908.0B

History

💚 Build #115041 succeeded 7adcad1
💔 Build #115015 failed e6afc6f
💚 Build #114857 succeeded b7b86cb
💔 Build #114697 failed 3e2d11fb3be20eb84ad95bf902b4f0d7b9abfd67

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…on functions (elastic#95224) # Conflicts: # src/plugins/data/server/search/search_service.ts

…on functions (#95224) (#95401) # Conflicts: # src/plugins/data/server/search/search_service.ts

…on functions (#95224) (#114447) # Conflicts: # src/plugins/data/server/search/search_service.ts Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

ppisljar added 3 commits March 23, 2021 18:34

kibana context extract/inject

b1914da

extractin index pattern id

0064999

updating executor

6c1fdbb

ppisljar added WIP Work in progress Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Mar 23, 2021

ppisljar requested a review from a team as a code owner March 23, 2021 18:53

ppisljar added 2 commits March 24, 2021 12:48

removing getSavedObject handler

362ade4

updating docs

b7b86cb

ppisljar force-pushed the expressions/extract_inject branch from 3e2d11f to b7b86cb Compare March 24, 2021 12:00

ppisljar removed the WIP Work in progress label Mar 24, 2021

dokmic reviewed Mar 24, 2021

View reviewed changes

src/plugins/data/common/index_patterns/expressions/load_index_pattern.ts Outdated Show resolved Hide resolved

dokmic reviewed Mar 24, 2021

View reviewed changes

dokmic approved these changes Mar 24, 2021

View reviewed changes

streamich assigned streamich and unassigned streamich Mar 24, 2021

streamich self-requested a review March 24, 2021 14:35

streamich approved these changes Mar 24, 2021

View reviewed changes

ppisljar and others added 5 commits March 24, 2021 19:39

Apply suggestions from code review

10ee5b7

Co-authored-by: Vadim Dalecky <streamich@gmail.com>

Apply suggestions from code review

f937922

Co-authored-by: Michael Dokolin <dokmic@gmail.com>

review feedback

e6afc6f

review feedback

7adcad1

Merge branch 'master' into expressions/extract_inject

9fb63dc

# Conflicts: # src/plugins/data/server/search/search_service.ts

ppisljar merged commit 1e87cef into elastic:master Mar 25, 2021

ppisljar added a commit to ppisljar/kibana that referenced this pull request Mar 25, 2021

handling references for kibana_context and get_index_pattern expressi…

c66b67d

…on functions (elastic#95224) # Conflicts: # src/plugins/data/server/search/search_service.ts

ppisljar mentioned this pull request Mar 25, 2021

[7.x] handling references for kibana_context and get_index_pattern expression functions (#95224) #95401

Merged

ppisljar added a commit that referenced this pull request Mar 25, 2021

handling references for kibana_context and get_index_pattern expressi…

8a89d6c

…on functions (#95224) (#95401) # Conflicts: # src/plugins/data/server/search/search_service.ts

ppisljar deleted the expressions/extract_inject branch March 29, 2021 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handling references for kibana_context and get_index_pattern expression functions #95224

handling references for kibana_context and get_index_pattern expression functions #95224

ppisljar commented Mar 23, 2021 •

edited

Loading

elasticmachine commented Mar 23, 2021

dokmic Mar 24, 2021

ppisljar Mar 24, 2021

dokmic Mar 24, 2021

dokmic Mar 24, 2021

dokmic left a comment

streamich left a comment

streamich Mar 24, 2021 •

edited

Loading

ppisljar Mar 24, 2021

streamich Mar 24, 2021

kibanamachine commented Mar 25, 2021

	if (state.savedSearchId.length && typeof state.savedeSearchId[0] === 'string') {
	if (typeof state.savedeSearchId[0] === 'string') {

	if (!getKibanaRequest \|\| !getKibanaRequest()) {
	if (!getKibanaRequest?.()) {

handling references for kibana_context and get_index_pattern expression functions #95224

handling references for kibana_context and get_index_pattern expression functions #95224

Conversation

ppisljar commented Mar 23, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Mar 23, 2021

dokmic Mar 24, 2021

Choose a reason for hiding this comment

ppisljar Mar 24, 2021

Choose a reason for hiding this comment

dokmic Mar 24, 2021

Choose a reason for hiding this comment

dokmic Mar 24, 2021

Choose a reason for hiding this comment

dokmic left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

streamich Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

ppisljar Mar 24, 2021

Choose a reason for hiding this comment

streamich Mar 24, 2021

Choose a reason for hiding this comment

kibanamachine commented Mar 25, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Page load bundle

History

ppisljar commented Mar 23, 2021 •

edited

Loading

streamich Mar 24, 2021 •

edited

Loading