-
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
[Security Solution] [Detections] amends useFetchIndex hook to accept data view id in addition to array of index names #160079
Conversation
… array of index names and updates useRuleIndexPattern hook to pass in data view id to useFetchIndex
…a into use-fetch-data-view-hook
// * @deprecated use DataViewSpec from @kbn/data-views-plugin/common | ||
// * or DataView if absolutely necessary | ||
// */ | ||
// export type DataViewBase = DataViewBase; |
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.
noticed commented code in few places. Can it be removed?
x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx
Outdated
Show resolved
Hide resolved
pick(key, schema), | ||
filterManager, | ||
license, | ||
dataViewInstance as DataViewBase |
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.
is this possible to avoid type castings?
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.
Kind of the same situation as above except this is due to the unified search team's component FilterBadgeGroup
needing a DataViewBase
type to be passed to it, which the dataViewInstance
is compatible with. Until that changes, we'll need the casting unfortunately.
const dataViewInstance = useMemo(() => { | ||
if (indexPattern != null) { | ||
const dv = new DataView({ spec: indexPattern, fieldFormats }); | ||
return { ...dv, fields: Object.values(indexPattern.fields ?? {}) } as DataView; |
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.
is this possible to avoid type castings?
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.
This is a great question - I kept these type castings because until we also update all the exceptions code to accept the DataViewSpec
type, we can't update the child component FieldComponent
, so once that work is completed we can remove the type castings all together which will be great. I'll add a todo to the code.
const dv = new DataView({ spec: threatIndexPatterns, fieldFormats }); | ||
return { ...dv, fields: Object.values(threatIndexPatterns.fields ?? {}) } as DataView; | ||
} | ||
return []; |
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.
nit: separate transform function can be added to DataView instance from index and fields formats. dataViewInstance and threatDataViewInstance look indentical
…rules/step_define_rule/index.tsx faster reduce Co-authored-by: Vitalii Dmyterko <[email protected]>
…a into use-fetch-data-view-hook
…-ref HEAD~1..HEAD --fix'
|
||
previousIndexesName.current = dv.getIndexPattern().split(','); | ||
const { browserFields } = getDataViewStateFromIndexFields(iNames, dataView.fields); | ||
previousIndexesNameOrId.current = dataView?.title?.split(','); |
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.
should not this previousIndexesNameOrId.current = dataView?.title?.split(',');
be within previous else
branch? it will always override line 136
acc.keywordFields.push({ label: field.name }); | ||
} else if (field.type === 'date') { | ||
acc.dateFields.push({ label: field.name }); | ||
} else if (field.type !== 'date') { |
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.
Can be just } else {
?
…a into use-fetch-data-view-hook
cypress tests were failing because of a missing dependency here: #160445 once that is merged I'll update this PR and hopefully that fixes a few of the failures. |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @dhurley14 |
} | ||
|
||
/** | ||
* Independent index fields hook/request | ||
* returns state directly, no redux | ||
*/ | ||
export const useFetchIndex = ( | ||
indexNames: string[], | ||
indexNamesOrDvId: string | string[], |
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.
If we're trying to move away from the indexNames
pattern, would it be better to just construct a new param structure and separate these two arguments instead of delineating on isArray
? I understand that would cause a lot of refactor since we use this hook everywhere, but just wondering what your thoughts were. Having multiple variables in this function logic with "or" in the name reads a bit confusing
@dhurley14 is this PR still being worked on or can it be closed? |
3 similar comments
@dhurley14 is this PR still being worked on or can it be closed? |
@dhurley14 is this PR still being worked on or can it be closed? |
@dhurley14 is this PR still being worked on or can it be closed? |
@PhilippeOberti Hi 👋 I will close this out. Hopefully myself or someone can pick this issue back up in the future. I think it is worth it as part of the cleanup work with sourcerer. |
Summary
Please read for more background on motivations for this PR -> #154968
updates useFetchIndex hook to accept a data view id in addition to an array of index names and updates useRuleIndexPattern hook to pass in data view id to useFetchIndex.