-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Replace RxJS firstValueFrom with RxJS lastValueFrom to prevent problem with partial results #134682
Merged
kertal
merged 7 commits into
elastic:main
from
kertal:replace-observable-firstValueFrom
Jun 20, 2022
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4957571
Replace firstValueFrom with lastValueFrom
kertal 58fb5e6
Merge remote-tracking branch 'upstream/main' into replace-observable-…
kertal e387b44
Add test
kertal 09f704b
Add test for search source expression
kertal f7beca1
Add test for es query DSL expression
kertal 412e279
Merge branch 'main' into replace-observable-firstValueFrom
kertal 0e8985a
Merge branch 'main' into replace-observable-firstValueFrom
kertal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
|
||
import { useCallback, useEffect, useMemo, useState } from 'react'; | ||
import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; | ||
import { firstValueFrom } from 'rxjs'; | ||
import { lastValueFrom } from 'rxjs'; | ||
import { DataView } from '@kbn/data-views-plugin/public'; | ||
import { DocProps } from '../application/doc/components/doc'; | ||
import { ElasticRequestState } from '../application/doc/types'; | ||
|
@@ -18,47 +18,6 @@ import { useDiscoverServices } from './use_discover_services'; | |
|
||
type RequestBody = Pick<estypes.SearchRequest, 'body'>; | ||
|
||
/** | ||
* helper function to build a query body for Elasticsearch | ||
* https://www.elastic.co/guide/en/elasticsearch/reference/current//query-dsl-ids-query.html | ||
*/ | ||
export function buildSearchBody( | ||
id: string, | ||
indexPattern: DataView, | ||
useNewFieldsApi: boolean, | ||
requestAllFields?: boolean | ||
): RequestBody | undefined { | ||
const computedFields = indexPattern.getComputedFields(); | ||
const runtimeFields = computedFields.runtimeFields as estypes.MappingRuntimeFields; | ||
const request: RequestBody = { | ||
body: { | ||
query: { | ||
ids: { | ||
values: [id], | ||
}, | ||
}, | ||
stored_fields: computedFields.storedFields, | ||
script_fields: computedFields.scriptFields, | ||
version: true, | ||
}, | ||
}; | ||
if (!request.body) { | ||
return undefined; | ||
} | ||
if (useNewFieldsApi) { | ||
// @ts-expect-error | ||
request.body.fields = [{ field: '*', include_unmapped: 'true' }]; | ||
request.body.runtime_mappings = runtimeFields ? runtimeFields : {}; | ||
if (requestAllFields) { | ||
request.body._source = true; | ||
} | ||
} else { | ||
request.body._source = true; | ||
} | ||
request.body.fields = [...(request.body?.fields || []), ...(computedFields.docvalueFields || [])]; | ||
return request; | ||
} | ||
|
||
/** | ||
* Custom react hook for querying a single doc in ElasticSearch | ||
*/ | ||
|
@@ -75,14 +34,15 @@ export function useEsDocSearch({ | |
|
||
const requestData = useCallback(async () => { | ||
try { | ||
const { rawResponse } = await firstValueFrom( | ||
const result = await lastValueFrom( | ||
data.search.search({ | ||
params: { | ||
index, | ||
body: buildSearchBody(id, indexPattern, useNewFieldsApi, requestSource)?.body, | ||
}, | ||
}) | ||
); | ||
const rawResponse = result.rawResponse; | ||
|
||
const hits = rawResponse.hits; | ||
|
||
|
@@ -109,3 +69,44 @@ export function useEsDocSearch({ | |
|
||
return [status, hit, requestData]; | ||
} | ||
|
||
/** | ||
* helper function to build a query body for Elasticsearch | ||
* https://www.elastic.co/guide/en/elasticsearch/reference/current//query-dsl-ids-query.html | ||
*/ | ||
export function buildSearchBody( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a little code style change, moving the helper function to the bottom, so when scrolling the doc, the important function is displayed first |
||
id: string, | ||
indexPattern: DataView, | ||
useNewFieldsApi: boolean, | ||
requestAllFields?: boolean | ||
): RequestBody | undefined { | ||
const computedFields = indexPattern.getComputedFields(); | ||
const runtimeFields = computedFields.runtimeFields as estypes.MappingRuntimeFields; | ||
const request: RequestBody = { | ||
body: { | ||
query: { | ||
ids: { | ||
values: [id], | ||
}, | ||
}, | ||
stored_fields: computedFields.storedFields, | ||
script_fields: computedFields.scriptFields, | ||
version: true, | ||
}, | ||
}; | ||
if (!request.body) { | ||
return undefined; | ||
} | ||
if (useNewFieldsApi) { | ||
// @ts-expect-error | ||
request.body.fields = [{ field: '*', include_unmapped: 'true' }]; | ||
request.body.runtime_mappings = runtimeFields ? runtimeFields : {}; | ||
if (requestAllFields) { | ||
request.body._source = true; | ||
} | ||
} else { | ||
request.body._source = true; | ||
} | ||
request.body.fields = [...(request.body?.fields || []), ...(computedFields.docvalueFields || [])]; | ||
return request; | ||
} |
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dear @Dosant , if you have minute, could you have a look at the unit tests, they work, but I would love to know, if I could make them better ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will do it 👍 should capture the flow well.
Theoretically it is possible to make them behave more real-world by adding delay+fake timers between emits. Maybe even use rxjs-marbles for it.
But I think that's already good enough for the current use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx @Dosant , for confirming 👍