Skip to content

Commit

Permalink
[Discover] Make _index and _id optional in EsHitRecord to suppo…
Browse files Browse the repository at this point in the history
…rt ES|QL records (#184975)

## Summary

This PR updates the `EsHitRecord` type to make `_index` and `_id`
optional in order to avoid dangerously casting ES|QL records (which may
not have `_index` or `_id`) to `EsHitRecord`.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
davismcphee authored Jun 11, 2024
1 parent 9298c6f commit dac2108
Show file tree
Hide file tree
Showing 11 changed files with 43 additions and 21 deletions.
6 changes: 4 additions & 2 deletions packages/kbn-data-service/src/search/tabify/tabify_docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ interface TabifyDocsOptions {

// This is an overwrite of the SearchHit type to add the ignored_field_values.
// Can be removed once the estypes.SearchHit knows about ignored_field_values
type Hit<T = unknown> = estypes.SearchHit<T> & { ignored_field_values?: Record<string, unknown[]> };
type Hit<T = unknown> = Partial<estypes.SearchHit<T>> & {
ignored_field_values?: Record<string, unknown[]>;
};

function flattenAccum(
flat: Record<string, any>,
Expand Down Expand Up @@ -137,7 +139,7 @@ export function flattenHit(hit: Hit, indexPattern?: DataView, params?: TabifyDoc
const isExcludedMetaField =
EXCLUDED_META_FIELDS.includes(fieldName) || fieldName.charAt(0) !== '_';
if (!isExcludedMetaField) {
flat[fieldName] = hit[fieldName as keyof estypes.SearchHit];
flat[fieldName] = hit[fieldName as keyof Hit];
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions packages/kbn-discover-utils/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import type { SearchHit } from '@elastic/elasticsearch/lib/api/typesWithBodyKey'

export type { IgnoredReason, ShouldShowFieldInTableHandler } from './utils';

export interface EsHitRecord extends Omit<SearchHit, '_source'> {
_source?: Record<string, unknown>;
type DiscoverSearchHit = SearchHit<Record<string, unknown>>;

export interface EsHitRecord extends Omit<DiscoverSearchHit, '_index' | '_id' | '_source'> {
_index?: DiscoverSearchHit['_index'];
_id?: DiscoverSearchHit['_id'];
_source?: DiscoverSearchHit['_source'];
}

/**
Expand Down
10 changes: 7 additions & 3 deletions packages/kbn-discover-utils/src/utils/format_hit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,19 @@
* Side Public License, v 1.
*/

import type { SearchHit } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import { i18n } from '@kbn/i18n';
import type { FieldFormatsStart } from '@kbn/field-formats-plugin/public';
import type { DataView } from '@kbn/data-views-plugin/public';
import type { DataTableRecord, ShouldShowFieldInTableHandler, FormattedHit } from '../types';
import type {
DataTableRecord,
ShouldShowFieldInTableHandler,
FormattedHit,
EsHitRecord,
} from '../types';
import { formatFieldValue } from './format_value';

const formattedHitCache = new WeakMap<
SearchHit,
EsHitRecord,
{ formattedHit: FormattedHit; maxEntries: number }
>();

Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-discover-utils/src/utils/format_value.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
* Side Public License, v 1.
*/

import type { SearchHit } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import type { FieldFormatsStart } from '@kbn/field-formats-plugin/public';
import { KBN_FIELD_TYPES } from '@kbn/field-types';
import type { DataView, DataViewField } from '@kbn/data-views-plugin/public';
Expand All @@ -15,6 +14,7 @@ import type {
HtmlContextTypeOptions,
TextContextTypeOptions,
} from '@kbn/field-formats-plugin/common/types';
import { EsHitRecord } from '../types';

/**
* Formats the value of a specific field using the appropriate field formatter if available
Expand All @@ -31,7 +31,7 @@ import type {
*/
export function formatFieldValue(
value: unknown,
hit: SearchHit,
hit: EsHitRecord,
fieldFormats: FieldFormatsStart,
dataView?: DataView,
field?: DataViewField,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ describe('context predecessors', function () {
fetchPredecessors = (timeValIso, timeValNr, tieBreakerField, tieBreakerValue, size = 10) => {
const anchor = buildDataTableRecord(
{
_id: 'test',
_source: {
[dataView.timeFieldName!]: timeValIso,
},
Expand Down Expand Up @@ -220,6 +221,7 @@ describe('context predecessors', function () {
fetchPredecessors = (timeValIso, timeValNr, tieBreakerField, tieBreakerValue, size = 10) => {
const anchor = buildDataTableRecord(
{
_id: 'test',
_source: {
[dataView.timeFieldName!]: timeValIso,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export async function fetchSurroundingDocs(
rows: DataTableRecord[];
interceptedWarnings: SearchResponseWarning[] | undefined;
}> {
if (typeof anchor !== 'object' || anchor === null || !size) {
if (typeof anchor !== 'object' || anchor === null || !anchor.raw._id || !size) {
return {
rows: [],
interceptedWarnings: undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import type { ExpressionsStart } from '@kbn/expressions-plugin/public';
import type { Datatable } from '@kbn/expressions-plugin/public';
import type { DataView } from '@kbn/data-views-plugin/common';
import { textBasedQueryStateToAstWithValidation } from '@kbn/data-plugin/common';
import type { DataTableRecord, EsHitRecord } from '@kbn/discover-utils';
import type { DataTableRecord } from '@kbn/discover-utils';
import type { RecordsFetchResponse } from '../../types';
import type { ProfilesManager } from '../../../context_awareness';

Expand Down Expand Up @@ -84,7 +84,7 @@ export function fetchEsql({
finalData = rows.map((row, idx) => {
const record: DataTableRecord = {
id: String(idx),
raw: row as EsHitRecord,
raw: row,
flattened: row,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { useNavigationProps } from '../../../hooks/use_navigation_props';
interface TableRowDetailsProps {
children: JSX.Element;
colLength: number;
rowIndex: string;
rowId: string;
rowIndex: string | undefined;
rowId: string | undefined;
columns: string[];
isTimeBased: boolean;
dataView: DataView;
Expand Down
16 changes: 11 additions & 5 deletions src/plugins/discover/public/hooks/use_navigation_props.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { useDiscoverServices } from './use_discover_services';

export interface UseNavigationProps {
dataView: DataView;
rowIndex: string;
rowId: string;
rowIndex: string | undefined;
rowId: string | undefined;
columns: string[];
savedSearchId?: string;
// provided by embeddable only
Expand Down Expand Up @@ -95,6 +95,9 @@ export const useNavigationProps = ({
);

useEffect(() => {
if (!rowIndex || !rowId) {
return;
}
const dataViewId = typeof index === 'object' ? index.id : index;
services.locator
.getUrl({ dataViewId, ...buildParams() })
Expand All @@ -113,6 +116,9 @@ export const useNavigationProps = ({
]);

useEffect(() => {
if (!rowIndex || !rowId) {
return;
}
const params = buildParams();
const dataViewId = typeof index === 'object' ? index.id : index;
services.locator
Expand All @@ -139,7 +145,7 @@ export const useNavigationProps = ({

const onOpenSingleDoc: MouseEventHandler = useCallback(
(event) => {
if (isModifiedEvent(event)) {
if (isModifiedEvent(event) || !rowIndex || !rowId) {
return;
}
event.preventDefault();
Expand All @@ -155,11 +161,11 @@ export const useNavigationProps = ({

const onOpenContextView: MouseEventHandler = useCallback(
(event) => {
const params = buildParams();
if (isModifiedEvent(event)) {
if (isModifiedEvent(event) || !rowId) {
return;
}
event.preventDefault();
const params = buildParams();
const dataViewId = typeof index === 'object' ? index.id : index;
services.locator.getUrl({ dataViewId, ...params }).then((referrer) =>
services.contextLocator.navigate({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { JSONCodeEditorCommonMemoized } from '../json_code_editor';

interface SourceViewerProps {
id: string;
index: string;
index: string | undefined;
dataView: DataView;
textBasedHits?: DataTableRecord[];
hasLineNumbers: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export interface EsDocSearchProps {
/**
* Index in ES to query
*/
index: string;
index: string | undefined;
/**
* DataView entity
*/
Expand Down Expand Up @@ -57,6 +57,10 @@ export function useEsDocSearch({
const useNewFieldsApi = useMemo(() => !uiSettings.get(SEARCH_FIELDS_FROM_SOURCE), [uiSettings]);

const requestData = useCallback(async () => {
if (!index) {
return;
}

const singleDocFetchingStartTime = window.performance.now();
try {
const result = await lastValueFrom(
Expand Down

0 comments on commit dac2108

Please sign in to comment.