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

Select the correct language when a different one is chosen by the dataset selector #8671

Closed
wants to merge 2 commits into from
Closed

Select the correct language when a different one is chosen by the dataset selector #8671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddf822e
Select the correct language when a different one is chosen by the dat…
AMoo-Miki Oct 19, 2024
5be475d
Changeset file for PR #8671 created/updated
opensearch-changeset-bot[bot] Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/8671.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Select the correct language when a different one is chosen by the dataset selector ([#8671](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8671))
6 changes: 3 additions & 3 deletions src/plugins/data/public/query/query_string/query_string_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import { BehaviorSubject } from 'rxjs';
import { skip } from 'rxjs/operators';
import { CoreStart, NotificationsSetup } from 'opensearch-dashboards/public';
import { debounce, isEqual } from 'lodash';
import { isEqual } from 'lodash';
import { i18n } from '@osd/i18n';
import { Dataset, DataStorage, Query, TimeRange, UI_SETTINGS } from '../../../common';
import { createHistory, QueryHistory } from './query_history';
Expand Down Expand Up @@ -106,8 +106,8 @@
}
}

public getUpdates$ = () => {
return this.query$.asObservable().pipe(skip(1));
public getUpdates$ = (defaultSkip = 1) => {
return this.query$.asObservable().pipe(skip(defaultSkip));

Check warning on line 110 in src/plugins/data/public/query/query_string/query_string_manager.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/query_string_manager.ts#L110 

Added line #L110 was not covered by tests
};

public getQuery = (): Query => {
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/ui/query_editor/language_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const QueryLanguageSelector = (props: QueryLanguageSelectorProps) => {
: undefined;

useEffect(() => {
const subscription = queryString.getUpdates$().subscribe((query: Query) => {
const subscription = queryString.getUpdates$(0).subscribe((query: Query) => {
Copy link
Member

Choose a reason for hiding this comment

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

is it potentially just a race condition or something?

could we verify if the props.query.language is the current query always. and this component gets re-rendered. i need to verify since last checking these things out. or if you have the insight already.

because what im thinking is that potentially the dataset is checking which languages are supported and adding to a list while this is listening to changes and before the language is added this setter is called but set to the first language it found.

there's a couple of places i think were we need loading spinners. shooting in the dark perhaps we just need to check if everything is done loading before setting the current language?

Copy link
Member

Choose a reason for hiding this comment

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

or opt for just disabling unsupported languages and setting the current value vs removing them from view

if (query.language !== currentLanguage) {
setCurrentLanguage(query.language);
}
Expand Down
7 changes: 3 additions & 4 deletions src/plugins/data/public/ui/query_editor/query_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,11 +240,10 @@ export default class QueryEditorUI extends Component<Props, State> {

if (!isEqual(this.props.query.dataset, prevQuery.dataset)) {
if (this.inputRef) {
const newQuery = this.queryString.getInitialQuery();
const newQueryString = newQuery.query;
if (this.inputRef.getValue() !== newQueryString) {
const newQueryString = this.props.query.query;
Copy link
Member

Choose a reason for hiding this comment

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

i think now when i select the dataset for the new language the query string is not updated with the updated dataset.

Copy link
Member

Choose a reason for hiding this comment

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

recreation steps:

  • have an index pattern and navigate to discover. load results
  • select an index from a cluster.
  • notice how the query (if for sql and ppl) still references the old dataset

if (this.inputRef.getValue() !== newQueryString && typeof newQueryString === 'string') {
this.inputRef.setValue(newQueryString);
this.onSubmit(newQuery);
this.onSubmit(this.props.query);
}
}
}
Expand Down
Loading