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

[7.x] [Discover] Use fields API to retrieve fields (#83891) #88493

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

majagrubic
Copy link
Contributor

Backports the following commits to 7.x:

* Add search source to example plugin.

* Add uiSetting for fields API.

* Update SearchSource to support fields API.

* [PoC] reading from the fields API in Discover

* Add N fields as a default column

* Make fields column non-removeable

* Do not add 'fields' to state

* Remove fields from app state and read from source when needed

* Remove fields column if a new column is added

* Add search source to example plugin.

* Add uiSetting for fields API.

* Update SearchSource to support fields API.

* Improve error handling in search examples plugin.

* Add unit tests for legacy behavior.

* Remove uiSettings feature flag; add fieldsFromSource config.

* Rewrite flatten() based on final API design.

* Update example app based on final API design.

* Update maps app to use legacy fieldsFromSource.

* Update Discover to use legacy fieldsFromSource.

* Rename source filters to field filters.

* Address feedback.

* Update generated docs.

* Update maps functional test.

* Formatting fields column similar to _source

* Moving logic for using search API to updating search source

* Fix small merge error

* Move useSource switch to Discover section of advanced settings

* Do not use fields and source at the same time

* Remove unmapped fields switch

* Add basic support for grouping multifields

* Remove output.txt

* Fix some merge leftovers

* Fix some merge leftovers

* Fix merge errors

* Fix typescript errors and update nested fields logic

* Add a unit test

* Fixing field formats

* Fix multifield selection logic

* Request all fields from source

* Fix eslint

* Fix default columns when switching between _source and fields

* More unit tests

* Update API changes

* Add unit test for discover field details footer

* Remove unused file

* Remove fields formatting from index pattern

* Remove unnecessary check

* Addressing design comments

* Fixing fields column display and renaming it to Document

* Adding more unit tests

* Adding a missing check for useNewFieldsAPI; minor fixes

* Fixing typescript error

* Remove unnecessary console statement

* Add missing prop

* Fixing import order

* Adding functional test to test fields API

* [Functional test] Clean up in after

* Fixing context app

* Addressing PR comments

* Updating failed snapshot

* Addressing PR comments

* Fixing i18n translations, updating type

* Addressing PR comments

* Updating a functional test

* Add a separate functional test for fields API

* Read fields from source in a functional test

* Skip buggy test

* Use default behavior in functional tests

* Fixing remaining failing tests

* Fixing date-nanos test

* Updating FLS test

* Fixing yet another functional test

* Skipping non-relevant tests

* Fixing more tests

* Update stub import in test

* Fix import

* Fix invalid import

Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/test/functional/apps/security/doc_level_security_roles.js
#	x-pack/test/functional/apps/security/field_level_security.js
@majagrubic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 336 338 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 504.8KB 513.5KB +8.7KB

History

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

@majagrubic majagrubic requested a review from kertal January 18, 2021 08:50
await kibanaServer.uiSettings.replace({ defaultIndex: 'testlargestring' });
await kibanaServer.uiSettings.replace({
defaultIndex: 'testlargestring',
'discover:searchFieldsFromSource': false,
Copy link
Member

Choose a reason for hiding this comment

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

Think it would be cleaner not to enable the discover:searchFieldsFromSource for this single functional, since this is 'experimental'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had already changed the test to pass with the fields API 😂 This was an easier fix. But can revert it to the old behavior

defaultIndex: 'logstash-*',
'doc_table:legacy': false,
'discover:searchFieldsFromSource': false,
};
Copy link
Member

Choose a reason for hiding this comment

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

here I think setting searchFieldsFromSource to false is fine, since it's a test for a flagged feature

@@ -88,7 +88,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
'/app/discover?_t=1453775307251#' +
'/?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time' +
":(from:'2015-09-19T06:31:44.000Z',to:'2015-09" +
"-23T18:31:44.000Z'))&_a=(columns:!(),filters:!(),index:'logstash-" +
Copy link
Member

Choose a reason for hiding this comment

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

So a shared link now contains _source? I think it would be better not to change this expectURL, but to revert the change that leads to the adding of _source to the shared link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It contains _source right now. When switching to fields API, it won't include _source, as _source column doesn't exist anymore. Not sure if there's a proper fix for that. As for the test - I changed that test in master, this is basically reverting it to the current behavior.

@kertal kertal self-requested a review January 19, 2021 10:48
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Fine with going forward here, my remarks covered minor stuff, think it's better to merge to prevent back port conflicts.

@majagrubic majagrubic merged commit 5303669 into elastic:7.x Jan 19, 2021
@majagrubic majagrubic deleted the backport/7.x/pr-83891 branch January 19, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants