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

[Discover] Update Version Filter for Data Sources #8785

Merged
merged 1 commit into from
Nov 2, 2024
Merged

[Discover] Update Version Filter for Data Sources #8785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 57 additions & 4 deletions src/plugins/data/public/query/query_string/dataset_service/lib/index_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,21 @@ import { indexTypeConfig } from './index_type';
import { SavedObjectsClientContract } from 'opensearch-dashboards/public';
import { DATA_STRUCTURE_META_TYPES, DataStructure, Dataset } from '../../../../../common';
import * as services from '../../../../services';
import { IDataPluginServices } from 'src/plugins/data/public';

jest.mock('../../../../services', () => ({
getSearchService: jest.fn(),
getIndexPatterns: jest.fn(),
}));
jest.mock('../../../../services', () => {
return {
getSearchService: jest.fn(),
getIndexPatterns: jest.fn(),
getQueryService: () => ({
queryString: {
getLanguageService: () => ({
getQueryEditorExtensionMap: jest.fn().mockReturnValue({}),
}),
},
}),
};
});

describe('indexTypeConfig', () => {
const mockSavedObjectsClient = {} as SavedObjectsClientContract;
Expand Down Expand Up @@ -77,4 +87,47 @@ describe('indexTypeConfig', () => {
const mockDataset: Dataset = { id: 'index1', title: 'Index 1', type: 'INDEX' };
expect(indexTypeConfig.supportedLanguages(mockDataset)).toEqual(['SQL', 'PPL']);
});

test('should fetch data sources for unknown type', async () => {
mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
savedObjects: [
{ id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '3.0' } },
],
});

const result = await indexTypeConfig.fetch(mockServices as IDataPluginServices, [
{ id: 'unknown', title: 'Unknown', type: 'Unknown' },
]);

expect(result.children).toHaveLength(1);
expect(result.children?.[0].title).toBe('DataSource 1');
expect(result.hasNext).toBe(true);
});

test('should filter out data sources with versions lower than 1.0.0', async () => {
mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
savedObjects: [
{ id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '1.0' } },
{
id: 'ds2',
attributes: { title: 'DataSource 2', dataSourceVersion: '' },
},
{ id: 'ds3', attributes: { title: 'DataSource 3', dataSourceVersion: '2.17.0' } },
{
id: 'ds4',
attributes: { title: 'DataSource 4', dataSourceVersion: '.0' },
},
],
});

const result = await indexTypeConfig.fetch(mockServices as IDataPluginServices, [
{ id: 'unknown', title: 'Unknown', type: 'UNKNOWN' },
]);

expect(result.children).toHaveLength(2);
expect(result.children?.[0].title).toBe('DataSource 1');
expect(result.children?.[1].title).toBe('DataSource 3');
expect(result.children?.some((child) => child.title === 'DataSource 2')).toBe(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: not sure how valuable this is

expect(result.hasNext).toBe(true);
});
});
10 changes: 5 additions & 5 deletions src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { SavedObjectsClientContract } from 'opensearch-dashboards/public';
import { map } from 'rxjs/operators';
import { i18n } from '@osd/i18n';
import semver from 'semver';
import {
DEFAULT_DATA,
DataStructure,
Expand Down Expand Up @@ -120,11 +121,10 @@
perPage: 10000,
});
const dataSources: DataStructure[] = response.savedObjects
.filter(
(savedObject) =>
typeof savedObject.attributes?.dataSourceEngineType === 'string' &&
!savedObject.attributes?.dataSourceEngineType?.includes('OpenSearch Serverless')
)
.filter((savedObject) => {
const coercedVersion = semver.coerce(savedObject.attributes.dataSourceVersion);

Check warning on line 125 in src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts#L125 

Added line #L125 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

This attribute always exists?

Copy link
Member

@kavilla kavilla Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah @Flyingliuhub @ZilongX confirming this is the correct attribute?

We should get cypress tests once i merge the workflow in #8703

@virajsanghvi should do a scan on the usage of this property? I see a number of references to dataSourceEngineType in main right now

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes dataSourceVersion would always exist, it may carry empty value for invalid datasource but the filed itself would be there

return coercedVersion ? semver.satisfies(coercedVersion, '>=1.0.0') : false;
Copy link
Member

Choose a reason for hiding this comment

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

ah i see this what we meant with putting version. if this is the case would we be able to go with the original suggestion of adding an optional property of the dataset type ?

})
.map((savedObject) => ({
id: savedObject.id,
title: savedObject.attributes.title,
Expand Down
18 changes: 11 additions & 7 deletions src/plugins/query_enhancements/public/datasets/s3_type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,36 +142,40 @@ describe('s3TypeConfig', () => {
it('should fetch data sources for unknown type', async () => {
mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
savedObjects: [
{ id: 'ds1', attributes: { title: 'DataSource 1', dataSourceEngineType: 'OpenSearch' } },
{ id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '3.0' } },
],
});

const result = await s3TypeConfig.fetch(mockServices as IDataPluginServices, [
{ id: 'unknown', title: 'Unknown', type: 'UNKNOWN' },
]);

expect(result.children).toHaveLength(1); // Including DEFAULT_DATA.STRUCTURES.LOCAL_DATASOURCE
expect(result.children).toHaveLength(1);
expect(result.children?.[0].title).toBe('DataSource 1');
expect(result.hasNext).toBe(true);
});

it('should filter out OpenSearch Serverless data sources', async () => {
it('should filter out data sources with versions lower than 1.0.0', async () => {
mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
savedObjects: [
{ id: 'ds1', attributes: { title: 'DataSource 1', dataSourceEngineType: 'OpenSearch' } },
{ id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '1.0' } },
{
id: 'ds2',
attributes: { title: 'DataSource 2', dataSourceEngineType: 'OpenSearch Serverless' },
attributes: { title: 'DataSource 2', dataSourceVersion: '' },
},
{ id: 'ds3', attributes: { title: 'DataSource 3', dataSourceVersion: '2.17.0' } },
{
id: 'ds4',
attributes: { title: 'DataSource 4', dataSourceVersion: '.0' },
},
Comment on lines +158 to 170
Copy link
Collaborator

@Hailong-am Hailong-am Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can we add some test to check invalid semver version like a.b.c and semver version with snapshot/alpha (3.0.0-snapshot) etc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens on an invalid version? I assume coerce is just not touching it and it fails satisfies? Data source 2 is an example of an invalid version, but the truthy check on coercedVersion handles that case (I assume).

Docs looks like coerce should handle second case so don't think that's a big issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Confirmed an invalid version returns null locally, so lgtm

{ id: 'ds3', attributes: { title: 'DataSource 3', dataSourceEngineType: 'OpenSearch' } },
],
});

const result = await s3TypeConfig.fetch(mockServices as IDataPluginServices, [
{ id: 'unknown', title: 'Unknown', type: 'UNKNOWN' },
]);

expect(result.children).toHaveLength(2); // Including DEFAULT_DATA.STRUCTURES.LOCAL_DATASOURCE
expect(result.children).toHaveLength(2);
expect(result.children?.[0].title).toBe('DataSource 1');
expect(result.children?.[1].title).toBe('DataSource 3');
expect(result.children?.some((child) => child.title === 'DataSource 2')).toBe(false);
Expand Down
10 changes: 5 additions & 5 deletions src/plugins/query_enhancements/public/datasets/s3_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { i18n } from '@osd/i18n';
import { trimEnd } from 'lodash';
import { HttpSetup, SavedObjectsClientContract } from 'opensearch-dashboards/public';
import semver from 'semver';
import {
DATA_STRUCTURE_META_TYPES,
DEFAULT_DATA,
Expand Down Expand Up @@ -197,11 +198,10 @@ const fetchDataSources = async (client: SavedObjectsClientContract): Promise<Dat
perPage: 10000,
});
const dataSources: DataStructure[] = resp.savedObjects
.filter(
(savedObject) =>
typeof savedObject.attributes?.dataSourceEngineType === 'string' &&
!savedObject.attributes?.dataSourceEngineType?.includes('OpenSearch Serverless')
)
.filter((savedObject) => {
const coercedVersion = semver.coerce(savedObject.attributes.dataSourceVersion);
return coercedVersion ? semver.satisfies(coercedVersion, '>=1.0.0') : false;
})
Comment on lines +201 to +204
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use semver.vaild(savedObject.attributes.dataSourceVersion) to check datsource version is valid first and then do satisfies check?

Copy link
Member

Choose a reason for hiding this comment

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

+1, If semver.coerce returns null, this will lead to semver.satisfies(null, '>=x.x.x'), could cause an issue? You may want to add a null check before using semver.satisfies, as semver.satisfies doesn't handle the null

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't the truthy check before satisfies handle your concern?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. truthy check should handle the concerns about null and undefined, should we validate the datasource version is valid? Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

It's my understanding that if the dataSourceVersion is not valid in our situation, then semver.coerce() should return null.

Only text which lacks digits will fail coercion

dataSourceVersion is "invalid" when it is an empty string and gets caught by the truthy check.

.map((savedObject) => ({
id: savedObject.id,
title: savedObject.attributes.title,
Expand Down
Loading