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

Jump to bottom

[Search][BUG] Call wrong search strategy recursively in async search #69116

Merged
merged 5 commits into from
Jun 15, 2020
Merged

[Search][BUG] Call wrong search strategy recursively in async search #69116

merged 5 commits into from
Jun 15, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Jun 14, 2020 •
edited
Loading

Summary

Bug was introduced in #60342
This PR fixes calling wrong strategy recursively and adds a test improvement to prevent this in the future.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom added bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 14, 2020
@lizozom lizozom requested a review from a team as a code owner June 14, 2020 10:17
@lizozom lizozom self-assigned this Jun 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lizozom lizozom requested a review from lukasolson June 14, 2020 10:18
@lizozom lizozom changed the title [BUG] Call wrong search strategy recursively in async search [Search][BUG] Call wrong search strategy recursively in async search Jun 14, 2020
lukasolson
lukasolson approved these changes Jun 14, 2020
View reviewed changes
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM, added a minor comment below.

x-pack/plugins/data_enhanced/public/search/async_search_strategy.test.ts Outdated
@@ -24,7 +24,11 @@ describe('Async search strategy', () => {
beforeEach(() => {
mockCoreSetup = coreMock.createSetup();
mockDataStart = dataPluginMock.createStartContract();
(mockDataStart.search.getSearchStrategy as jest.Mock).mockReturnValue({ search: mockSearch });

const getStrategy = mockDataStart.search.getSearchStrategy as jest.Mock;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need to cast here. But since it's already there and not specific to this PR, I'm fine with it.

x-pack/plugins/data_enhanced/public/search/async_search_strategy.test.ts Outdated Show resolved Hide resolved
@lizozom lizozom requested a review from a team June 14, 2020 20:36
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load asset size

beta
id value diff baseline
/bundles/plugin/dataEnhanced/dataEnhanced.plugin.js 503.0KB +11.0B 503.0KB

History

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

flash1293
flash1293 approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, code review only. Just a changed mock

@lizozom lizozom merged commit efe6ba4 into elastic:master Jun 15, 2020
@lizozom lizozom mentioned this pull request Jun 15, 2020
Merged
lizozom added a commit to lizozom/kibana that referenced this pull request Jun 15, 2020
@lizozom
[Search][BUG] Call wrong search strategy recursively in async search (e…
7857ace 
…lastic#69116)

* Call sync search recursively

* Fix test

* Fix search mock to avoid resetting it

* delete empty line

* fix tests
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 15, 2020
@gmmorris
Merge branch 'master' into actions/execute-immediate
896b9a5 
* master: (91 commits)
  [Search][BUG] Call wrong search strategy recursively in async search (elastic#69116)
  [Observability] Create context container to enable Observability plugin registry function (elastic#68642)
  Rename space id for disabled index pattern test (elastic#68990)
  skip flaky suite (elastic#63339)
  Resolver Light Theme And Kibana Integration (elastic#67859)
  [kbn/dev-utils] expose public tooling_log module (elastic#68868)
  index pattern(s) take dependencies as object (elastic#69055)
  include ci-stats metrics in pr comment (elastic#68563)
  Bump webpack packages (elastic#68716)
  [Uptime] Fixed metric query broken because of missing mapping (elastic#68999)
  Added cloud as an optional dependency (elastic#69050)
  Fixed all external links (elastic#68614)
  [DOCS] Reorganizes doc nav to match new Kibana nav (elastic#69069)
  [Endpoint] Using the stats provided by the backend for resolver UI (elastic#68577)
  [DOCS] Removees 8.0 from Upgrade Assistant docs (elastic#69067)
  [ML] Fix cloud deployment ID check (elastic#68695)
  [DOCS] Move metrics app content to metrics monitoring guide (elastic#69033)
  Add ingest manager topic to docs (elastic#68980)
  [SECURITY SOLUTION] EMT-401: add policy data to metadata and fix tests (elastic#68582)
  [DOCS] Fixes POST request for saved objects (elastic#69036)
  ...
lizozom added a commit that referenced this pull request Jun 15, 2020
@lizozom
[Search][BUG] Call wrong search strategy recursively in async search (#…
1e7e8f8 
…69116) (#69128)

* Call sync search recursively

* Fix test

* Fix search mock to avoid resetting it

* delete empty line

* fix tests
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 15, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
f34d031 
* master: (60 commits)
  Re-enable mistakenly skipped tests. (elastic#69123)
  [Search][BUG] Call wrong search strategy recursively in async search (elastic#69116)
  [Observability] Create context container to enable Observability plugin registry function (elastic#68642)
  Rename space id for disabled index pattern test (elastic#68990)
  skip flaky suite (elastic#63339)
  Resolver Light Theme And Kibana Integration (elastic#67859)
  [kbn/dev-utils] expose public tooling_log module (elastic#68868)
  index pattern(s) take dependencies as object (elastic#69055)
  include ci-stats metrics in pr comment (elastic#68563)
  Bump webpack packages (elastic#68716)
  [Uptime] Fixed metric query broken because of missing mapping (elastic#68999)
  Added cloud as an optional dependency (elastic#69050)
  Fixed all external links (elastic#68614)
  [DOCS] Reorganizes doc nav to match new Kibana nav (elastic#69069)
  [Endpoint] Using the stats provided by the backend for resolver UI (elastic#68577)
  [DOCS] Removees 8.0 from Upgrade Assistant docs (elastic#69067)
  [ML] Fix cloud deployment ID check (elastic#68695)
  [DOCS] Move metrics app content to metrics monitoring guide (elastic#69033)
  Add ingest manager topic to docs (elastic#68980)
  [SECURITY SOLUTION] EMT-401: add policy data to metadata and fix tests (elastic#68582)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flash1293 flash1293 flash1293 approved these changes

@lukasolson lukasolson lukasolson approved these changes

Assignees

@lizozom lizozom

Labels
bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lizozom @elasticmachine @kibanamachine @lukasolson @flash1293