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

[DataView] Fix multiple pattern matching #152552

Closed
wants to merge 17 commits into from
Closed

[DataView] Fix multiple pattern matching #152552

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
bcb9f8c
fix query
shahzad31 Mar 2, 2023
b246027
fix type
shahzad31 Mar 2, 2023
66ee68a
Merge branch 'main' into fix-data-view
mattkime Mar 2, 2023
3fad229
add integration test
mattkime Mar 2, 2023
8665168
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Mar 2, 2023
ce77cfe
fix failure to match use cases
mattkime Mar 2, 2023
7c67413
Merge branch 'fix-data-view' of github.com:shahzad31/kibana into fix-…
mattkime Mar 2, 2023
4fdaec1
Merge branch 'main' into fix-data-view
mattkime Mar 5, 2023
3ba5abb
fix type
shahzad31 Mar 6, 2023
94b301c
Merge branch 'main' into fix-data-view
shahzad31 Mar 6, 2023
9691763
Merge branch 'main' into fix-data-view
mattkime Mar 20, 2023
5ffe582
Update src/plugins/data_views/server/fetcher/index_patterns_fetcher.ts
mattkime Mar 20, 2023
00227b4
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Mar 20, 2023
35299c2
fix REST api
mattkime Mar 20, 2023
c98dc74
fix REST api
mattkime Mar 20, 2023
0abb1c5
Merge branch 'main' into fix-data-view
mattkime Mar 21, 2023
d6aa974
Merge branch 'main' into fix-data-view
mattkime Mar 24, 2023
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
3 changes: 3 additions & 0 deletions src/plugins/data_views/server/fetcher/index_patterns_fetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ export class IndexPatternsFetcher {
// if only one pattern, don't bother with validation. We let getFieldCapabilities fail if the single pattern is bad regardless
if (patternList.length > 1 && !allowNoIndices) {
patternListActive = await this.validatePatternListActive(patternList);
if (patternListActive.length === 0) {
return { fields: [], indices: [] };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO we should back-port this as empty fields/indices list to 8.7 but in 8.8.0 we should throw error and communicate it to handle the error in UI.

Copy link
Contributor

@mattkime mattkime Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@shahzad31 we recently decided against that behavior since it can often result in 'toast storms' and does not align with the getting started experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. So i think in this case, if someone needs to really determine they can check length of indices to see if it actually matched any thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shahzad31 Yes, exactly, and handle it well for the user instead of depending upon the toast.

}
}
const fieldCapsResponse = await getFieldCapabilities({
callCluster: this.elasticsearchClient,
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/data_views/server/routes/fields_for.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from '@kbn/core/server';
import { IndexPatternsFetcher } from '../fetcher';
import type { DataViewsServerPluginStart, DataViewsServerPluginStartDependencies } from '../types';
import { DataViewMissingIndices } from '../../common';

const parseMetaFields = (metaFields: string | string[]) => {
let parsedFields: string[] = [];
Expand Down Expand Up @@ -91,6 +92,10 @@ const handler: RequestHandler<{}, IQuery, IBody> = async (context, request, resp
fields: request.query.fields,
});

if (indices.length === 0 && allowNoIndex !== true) {
throw new DataViewMissingIndices(pattern);
}
Comment on lines +95 to +97
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? shouldn't it just return an empty array of fields + indices in this case?

Copy link
Member

Choose a reason for hiding this comment

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

@mattkime these changes should be removed, to make the PR green


return response.ok({
body: { fields, indices },
headers: {
Expand Down
8 changes: 8 additions & 0 deletions test/api_integration/apis/data_views/fields_for_wildcard_route/response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,14 @@ export default function ({ getService }) {
indices: ['basic_index'],
});
});

it('returns 404 when neither exists', async () => {
await supertest
.get('/api/index_patterns/_fields_for_wildcard')
.query({ pattern: 'bad_index,bad_index_2' })
.expect(404);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think you will need to update test as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime ^^

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.expect(404);
.expect(200);

since it's no longer an error condition, this, with the reverting the change for fields_for should make this PR ready to merge

});

it('returns 404 when no patterns exist', async () => {
await supertest
.get('/api/index_patterns/_fields_for_wildcard')
Expand Down