-
Notifications
You must be signed in to change notification settings - Fork 918
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-next] Add query editor extensions #7034
[Discover-next] Add query editor extensions #7034
Conversation
A search bar extension can display a UI component above the query bar. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of Query Enhancements. ```ts export interface SearchBarExtensionDependencies { /** * Currently selected index patterns. */ indexPatterns?: IIndexPattern[]; } export interface SearchBarExtensionConfig { /** * The id for the search bar extension. */ id: string; /** * Lower order indicates higher position on UI. */ order: number; /** * A function that determines if the search bar extension is enabled and should be rendered on UI. * @returns whether the extension is enabled. */ isEnabled: () => Promise<boolean>; /** * A function that returns the mount point for the search bar extension. * @param dependencies - The dependencies required for the extension. * @returns The mount point for the search bar extension. */ getComponent: (dependencies: SearchBarExtensionDependencies) => React.ReactElement; } export interface QueryEnhancement { ... searchBar?: { ... extensions?: SearchBarExtensionConfig[]; }; } Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit e748e81)
…arch-project#6895) it adds query assist specific logic in query enhancements plugin to show a UI above the PPL search bar if user has configured PPL agent. Issues Resolved: opensearch-project#6820 * add query assist to query enhancements Signed-off-by: Joshua Li <joshuali925@gmail.com> * align language to uppercase Signed-off-by: Joshua Li <joshuali925@gmail.com> * pick PR 6167 Signed-off-by: Joshua Li <joshuali925@gmail.com> * use useState hooks for query assist There is a bug in data explorer `AppContainer` where memorized `DiscoverCanvas` gets unmounted after `setQuery`. PR 6167 works around it by memorizing `AppContainer`. As query assist is no longer being unmounted, we can use proper hooks to persist state now. Signed-off-by: Joshua Li <joshuali925@gmail.com> * Revert "pick PR 6167" This reverts commit acb0d41. Wait for official 6167 to merge to avoid conflict Signed-off-by: Joshua Li <joshuali925@gmail.com> * address comments for PR 6894 Signed-off-by: Joshua Li <joshuali925@gmail.com> --------- Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit 016e0f2)
…assist (opensearch-project#6933) * pass dependencies to isEnabled func Signed-off-by: Joshua Li <joshuali925@gmail.com> * add lazy and memo to search bar extensions Signed-off-by: Joshua Li <joshuali925@gmail.com> * move ppl specific string out from query assist Signed-off-by: Joshua Li <joshuali925@gmail.com> * prevent setstate after hook unmounts Signed-off-by: Joshua Li <joshuali925@gmail.com> * add max-height to search bar extensions Signed-off-by: Joshua Li <joshuali925@gmail.com> * prevent setstate after component unmounts Signed-off-by: Joshua Li <joshuali925@gmail.com> * move ml-commons API to common/index.ts Signed-off-by: Joshua Li <joshuali925@gmail.com> * improve i18n and accessibility usages Signed-off-by: Joshua Li <joshuali925@gmail.com> * add hard-coded suggestions for sample data indices Signed-off-by: Joshua Li <joshuali925@gmail.com> --------- Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit 4aade0f)
…ject#6972) * disable query assist for non-default datasource Signed-off-by: Joshua Li <joshuali925@gmail.com> * disable query assist input when loading Signed-off-by: Joshua Li <joshuali925@gmail.com> * support MDS for query assist Signed-off-by: Joshua Li <joshuali925@gmail.com> * add unit tests for agents Signed-off-by: Joshua Li <joshuali925@gmail.com> * Revert "add unit tests for agents" This reverts commit 983514e. The test configs are not yet setup in query_enhancements plugins. Signed-off-by: Joshua Li <joshuali925@gmail.com> --------- Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit 328e08e)
SearchBar extensions depend on the currently selected dataSource, which depends on opensearch-project#6833 to be ported to main Signed-off-by: Joshua Li <joshuali925@gmail.com>
ℹ️ Manual Changeset Creation ReminderPlease ensure manual commit for changeset file 7034.yml under folder changelogs/fragments to complete this PR. If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link. For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool. |
Signed-off-by: Joshua Li <joshuali925@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7034 +/- ##
=======================================
Coverage 67.44% 67.45%
=======================================
Files 3444 3447 +3
Lines 67865 67905 +40
Branches 11027 11034 +7
=======================================
+ Hits 45772 45802 +30
- Misses 19429 19436 +7
- Partials 2664 2667 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
To support bannar callout for query assist in un-supported languages and notify user the feature, the search bar extensions need to be enabled for all languages. Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925 you just have to install the bot: #7034 (comment) to your github then within the Changelog section of the PR description:, you can add the information in the correct format there and the changelog will automatically be generated with no manually requirement which we then use for the release notes generation. More info: #5519 Plugins should be able to onboard this too. s/o @BigSamu 🎆 this has been such a time saver. |
src/plugins/data/public/ui/search_bar_extensions/search_bar_extension.tsx
Outdated
Show resolved
Hide resolved
move SearchBarExtensions to QueryEditorExtensions and change config settings from list to map. Signed-off-by: Joshua Li <joshuali925@gmail.com>
Signed-off-by: Joshua Li <joshuali925@gmail.com>
Signed-off-by: Joshua Li <joshuali925@gmail.com>
const { configMap, componentContainer, bannerContainer, ...dependencies } = props; | ||
|
||
const sortedConfigs = useMemo(() => { | ||
if (!configMap || !Object.keys(configMap)) 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.
Object.keys
will either return an array or throw. ![]
is false always. If this was meant to prevent wasting time on an empty object, it fails to do that.
if (!configMap || !Object.keys(configMap)) return []; | |
if ('[object Object]' !== Object.prototype.toString.call(configMap) || Object.keys(configMap).length === 0) return []; |
This will need linting though.
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.
thanks good point on .length === 0
, i missed that
is the object check necessary or conventional in OSD?
if ( | ||
!shouldRenderQueryEditorExtensions() || | ||
!queryEditorHeaderRef.current || | ||
!queryEditorBannerRef.current || | ||
!queryLanguage | ||
) |
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: perf
if ( | |
!shouldRenderQueryEditorExtensions() || | |
!queryEditorHeaderRef.current || | |
!queryEditorBannerRef.current || | |
!queryLanguage | |
) | |
if (!( | |
queryEditorHeaderRef.current && | |
queryEditorBannerRef.current && | |
queryLanguage && | |
shouldRenderQueryEditorExtensions() | |
)) |
@@ -282,6 +304,10 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) { | |||
); | |||
} | |||
|
|||
function shouldRenderQueryEditorExtensions(): boolean { | |||
return Boolean(queryEditorExtensionMap && Object.keys(queryEditorExtensionMap).length); |
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.
Do we trust that queryEditorExtensionMap
is going to be a plain object? I am not sure where it is coming from and hence asking: could it possibly be manipulated in the metadata store?
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.
private queryEditorExtensionMap: Record<string, QueryEditorExtensionConfig> = {}; |
OpenSearch-Dashboards/src/plugins/data/public/ui/ui_service.ts
Lines 48 to 51 in 7f0e39e
if (enhancements.queryEditorExtension) { | |
this.queryEditorExtensionMap[enhancements.queryEditorExtension.id] = | |
enhancements.queryEditorExtension; | |
} |
what is the metadata store?
@@ -43,6 +45,10 @@ export class UiService implements Plugin<IUiSetup, IUiStart> { | |||
if (enhancements.query && enhancements.query.language) { | |||
this.queryEnhancements.set(enhancements.query.language, enhancements.query); | |||
} | |||
if (enhancements.queryEditorExtension) { |
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 this maybe be?
if (enhancements.queryEditorExtension) { | |
if (enhancements.queryEditorExtension?.id) { |
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.
id is enforced on the type level, is type checking not enough and we prefer to also add runtime checking in osd?
Since this is disabled as default and @AMoo-Miki not blocking. Feel free to add to this issue with fast follows: #6957 (comment) or create a new issue than you can handle fast follows from here. |
### Description see #6894 This PR picks #6894, #6895, #6933, #6972 to main. Additionally, - separates extensions from query enhancements - adds banner support - partially revert #6972 as it's pending on the data source commit to main - renames search bar extension to query editor extension A query editor extension can display a UI component above the query editor and/or a banner above the language selector. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of UI Enhancements. ```ts export interface QueryEditorExtensionDependencies { /** * Currently selected index patterns. */ indexPatterns?: Array<IIndexPattern | string>; /** * Currently selected data source. */ dataSource?: DataSource; /** * Currently selected query language. */ language: string; } export interface QueryEditorExtensionConfig { /** * The id for the search bar extension. */ id: string; /** * Lower order indicates higher position on UI. */ order: number; /** * A function that determines if the search bar extension is enabled and should be rendered on UI. * @returns whether the extension is enabled. */ isEnabled: (dependencies: QueryEditorExtensionDependencies) => Promise<boolean>; /** * A function that returns the search bar extension component. The component * will be displayed on top of the query editor in the search bar. * @param dependencies - The dependencies required for the extension. * @returns The component the search bar extension. */ getComponent?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null; /** * A function that returns the search bar extension banner. The banner is a * component that will be displayed on top of the search bar. * @param dependencies - The dependencies required for the extension. * @returns The component the search bar extension. */ getBanner?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null; } export interface UiEnhancements { query?: QueryEnhancement; + queryEditorExtension?: QueryEditorExtensionConfig; } ``` Developers can utilize search bar extensions to add additional features to the search bar, such as query assist. Issues resolved: #6077 A search bar extension can display a UI component above the query bar. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of Query Enhancements. Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit 4f54049) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
if (isMounted.current) setIsEnabled(enabled); | ||
}); | ||
}, [props.dependencies, props.config]); | ||
|
||
if (!isEnabled) return null; |
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: use braces even for single line if statements
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 this would be a good linter rule. I personally prefer one line if-returns, but i agree using braces is a better practice
i'll create an issue to track this and enable eslint curly, without linter there might be missed ones or mistakes
bannerContainer: Element; | ||
} | ||
|
||
const QueryEditorExtensions: React.FC<QueryEditorExtensionsProps> = React.memo((props) => { |
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: is there a better name for this given its both a collection of extensions as well as specifying their destinations (and its easy to confuse components that are one character apart). perhaps its a manager as its responsibilities could include managing what gets rendered if we want more rules than rendering everything
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.
maybe QueryEditorExtensionManager
and QueryEditorExtension
? but I'm not very sure if a manager is something that should be rendered on the page, current impl needs QueryEditor
to render QueryEditorExtensions
. The one character naming existed in other places, like suggestion_component
and suggestions_component
, although i do find it to be confusing
} from './query_editor_extension'; | ||
|
||
interface QueryEditorExtensionsProps extends QueryEditorExtensionDependencies { | ||
configMap?: Record<string, QueryEditorExtensionConfig>; |
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.
q: is there a reason this has to be a map vs an array?
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.
consistency with existing code, see #7034 (comment)
### Description see #6894 This PR picks #6894, #6895, #6933, #6972 to main. Additionally, - separates extensions from query enhancements - adds banner support - partially revert #6972 as it's pending on the data source commit to main - renames search bar extension to query editor extension A query editor extension can display a UI component above the query editor and/or a banner above the language selector. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of UI Enhancements. ```ts export interface QueryEditorExtensionDependencies { /** * Currently selected index patterns. */ indexPatterns?: Array<IIndexPattern | string>; /** * Currently selected data source. */ dataSource?: DataSource; /** * Currently selected query language. */ language: string; } export interface QueryEditorExtensionConfig { /** * The id for the search bar extension. */ id: string; /** * Lower order indicates higher position on UI. */ order: number; /** * A function that determines if the search bar extension is enabled and should be rendered on UI. * @returns whether the extension is enabled. */ isEnabled: (dependencies: QueryEditorExtensionDependencies) => Promise<boolean>; /** * A function that returns the search bar extension component. The component * will be displayed on top of the query editor in the search bar. * @param dependencies - The dependencies required for the extension. * @returns The component the search bar extension. */ getComponent?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null; /** * A function that returns the search bar extension banner. The banner is a * component that will be displayed on top of the search bar. * @param dependencies - The dependencies required for the extension. * @returns The component the search bar extension. */ getBanner?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null; } export interface UiEnhancements { query?: QueryEnhancement; + queryEditorExtension?: QueryEditorExtensionConfig; } ``` Developers can utilize search bar extensions to add additional features to the search bar, such as query assist. Issues resolved: #6077 A search bar extension can display a UI component above the query bar. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of Query Enhancements. (cherry picked from commit 4f54049) Signed-off-by: Joshua Li <joshuali925@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
### Description see opensearch-project#6894 This PR picks opensearch-project#6894, opensearch-project#6895, opensearch-project#6933, opensearch-project#6972 to main. Additionally, - separates extensions from query enhancements - adds banner support - partially revert opensearch-project#6972 as it's pending on the data source commit to main - renames search bar extension to query editor extension A query editor extension can display a UI component above the query editor and/or a banner above the language selector. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of UI Enhancements. ```ts export interface QueryEditorExtensionDependencies { /** * Currently selected index patterns. */ indexPatterns?: Array<IIndexPattern | string>; /** * Currently selected data source. */ dataSource?: DataSource; /** * Currently selected query language. */ language: string; } export interface QueryEditorExtensionConfig { /** * The id for the search bar extension. */ id: string; /** * Lower order indicates higher position on UI. */ order: number; /** * A function that determines if the search bar extension is enabled and should be rendered on UI. * @returns whether the extension is enabled. */ isEnabled: (dependencies: QueryEditorExtensionDependencies) => Promise<boolean>; /** * A function that returns the search bar extension component. The component * will be displayed on top of the query editor in the search bar. * @param dependencies - The dependencies required for the extension. * @returns The component the search bar extension. */ getComponent?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null; /** * A function that returns the search bar extension banner. The banner is a * component that will be displayed on top of the search bar. * @param dependencies - The dependencies required for the extension. * @returns The component the search bar extension. */ getBanner?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null; } export interface UiEnhancements { query?: QueryEnhancement; + queryEditorExtension?: QueryEditorExtensionConfig; } ``` Developers can utilize search bar extensions to add additional features to the search bar, such as query assist. Issues resolved: opensearch-project#6077 A search bar extension can display a UI component above the query bar. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of Query Enhancements. Signed-off-by: Joshua Li <joshuali925@gmail.com>
#7077) Follow up for #7034, this PR - addresses #7034 (comment) - addresses #7034 (comment) - fixes render order - QueryEditorExtensions requires the container divs to be mounted first, but in the previous implementation, extensions will be mounted first and it relied on rerendering of queryEditorTopRow. Moving them into query editor fixes the render order and ensures refs are set. @AMoo-Miki I didn't use the object check `'[object Object]' !== Object.prototype.toString.call(configMap)`. I don't know what access user has, but if an attacker can arbitrarily alter `configMap`, the code will still break with something like ```tsx configMap={ new Proxy( {}, { ownKeys(target) { throw new Error('Accessing keys is not allowed.'); }, } ) } ``` Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/7f0e39eb9809c95b98069cc971611edc2cbbc62b/src/plugins/data/public/ui/ui_service.ts#L31 Signed-off-by: Joshua Li <joshuali925@gmail.com>
#7077) Follow up for #7034, this PR - addresses #7034 (comment) - addresses #7034 (comment) - fixes render order - QueryEditorExtensions requires the container divs to be mounted first, but in the previous implementation, extensions will be mounted first and it relied on rerendering of queryEditorTopRow. Moving them into query editor fixes the render order and ensures refs are set. @AMoo-Miki I didn't use the object check `'[object Object]' !== Object.prototype.toString.call(configMap)`. I don't know what access user has, but if an attacker can arbitrarily alter `configMap`, the code will still break with something like ```tsx configMap={ new Proxy( {}, { ownKeys(target) { throw new Error('Accessing keys is not allowed.'); }, } ) } ``` Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/7f0e39eb9809c95b98069cc971611edc2cbbc62b/src/plugins/data/public/ui/ui_service.ts#L31 Signed-off-by: Joshua Li <joshuali925@gmail.com> (cherry picked from commit b85e177) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
#7077) (#7140) Follow up for #7034, this PR - addresses #7034 (comment) - addresses #7034 (comment) - fixes render order - QueryEditorExtensions requires the container divs to be mounted first, but in the previous implementation, extensions will be mounted first and it relied on rerendering of queryEditorTopRow. Moving them into query editor fixes the render order and ensures refs are set. @AMoo-Miki I didn't use the object check `'[object Object]' !== Object.prototype.toString.call(configMap)`. I don't know what access user has, but if an attacker can arbitrarily alter `configMap`, the code will still break with something like ```tsx configMap={ new Proxy( {}, { ownKeys(target) { throw new Error('Accessing keys is not allowed.'); }, } ) } ``` Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/7f0e39eb9809c95b98069cc971611edc2cbbc62b/src/plugins/data/public/ui/ui_service.ts#L31 (cherry picked from commit b85e177) Signed-off-by: Joshua Li <joshuali925@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
see #6894
This PR picks #6894, #6895, #6933, #6972 to main. Additionally,
A query editor extension can display a UI component above the query editor and/or a banner above the language selector. The component has the ability to read and write discover search bar states to enhance the search experience for users. The configuration is part of UI Enhancements.
Issues Resolved
Developers can utilize search bar extensions to add additional features to the search bar, such as query assist.
Part of #6077
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration