[DataView] Fix multiple pattern matching #152552
Conversation
shahzad31
added
v8.7.0
v8.8.0
release_note:skip
…-ref HEAD~1..HEAD --fix'
| if (patternListActive.length === 0) { | ||
| throw new DataViewMissingIndices(patternList.join(',')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since Matt is on PTO and also has been working on this issue, could you provide some context about this change, what is being fixed, and how it can be tested? Many thx!
BTW: some recent change in this area, to raise awareness #151788
There was a problem hiding this comment.
Hello, so the integration Test added by matt speaks for itself. when multiple heartbeat-8*,heartbeat-7*,synthetics-* like this was provided and if none of them contained any index, request was matching to all the indices.
src/plugins/data_views/server/fetcher/index_patterns_fetcher.ts
Outdated
Show resolved
Hide resolved
kertal
added
the
Feature:Data Views
mattkime
added
backport:prev-minor
Co-authored-by: Matthias Wilhelm <[email protected]>
…-ref HEAD~1..HEAD --fix'
| await supertest | ||
| .get('/api/index_patterns/_fields_for_wildcard') | ||
| .query({ pattern: 'bad_index,bad_index_2' }) | ||
| .expect(404); |
There was a problem hiding this comment.
i think you will need to update test as well.
| @@ -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: [] }; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@shahzad31 we recently decided against that behavior since it can often result in 'toast storms' and does not align with the getting started experience.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@shahzad31 Yes, exactly, and handle it well for the user instead of depending upon the toast.
|
@shahzad31 does other work depend upon this PR? It seems that addressing tests will be more work than 'fixing' the code. |
| if (indices.length === 0 && allowNoIndex !== true) { | ||
| throw new DataViewMissingIndices(pattern); | ||
| } |
There was a problem hiding this comment.
Why is this needed? shouldn't it just return an empty array of fields + indices in this case?
There was a problem hiding this comment.
@mattkime these changes should be removed, to make the PR green
|
This PR looks easier and removes unneeded code - #153350 |
| await supertest | ||
| .get('/api/index_patterns/_fields_for_wildcard') | ||
| .query({ pattern: 'bad_index,bad_index_2' }) | ||
| .expect(404); |
There was a problem hiding this comment.
| .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
|
Closing in favor of #153350 - will bring over the api integration test. |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
If multiple patterns are provided and it matches no indices. it should return 404.
Example
heartbeat-8*,heartbeat-7*,synthetics-*