Skip to content

[Data plugin] Wrong caching for Index pattern fields#87116

Merged
alexwizp merged 4 commits intoelastic:masterfrom
alexwizp:84666
Jan 5, 2021
Merged

[Data plugin] Wrong caching for Index pattern fields#87116
alexwizp merged 4 commits intoelastic:masterfrom
alexwizp:84666

Conversation

@alexwizp
Copy link
Copy Markdown
Contributor

@alexwizp alexwizp commented Jan 4, 2021 •
edited
Loading

Closes: #84666

Summary

[Data plugin] Wrong caching for Index pattern fields. Index pattern cache should be local instance, not global

alexwizp
alexwizp commented Jan 4, 2021
View reviewed changes
src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
import { findByTitle } from '../utils';
import { DuplicateIndexPatternError } from '../errors';

const indexPatternCache = createIndexPatternCache();
Copy link
Copy Markdown
Contributor Author

@alexwizp alexwizp Jan 4, 2021

Choose a reason for hiding this comment

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

@mattkime @ppisljar it looks like an issue while unifying client/server code.

This approach unacceptable for server side code. indexPatternCache variable should not declared as instance variable.

Copy link
Copy Markdown
Contributor Author

@alexwizp alexwizp Jan 4, 2021

Choose a reason for hiding this comment

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

also I think it should be backported into 7.10, 7.11 as a security issue.

@alexwizp alexwizp self-assigned this Jan 4, 2021
@alexwizp alexwizp added v7.12.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes Feature:Data Views Data Views code and UI - index patterns before 8.0 labels Jan 4, 2021
@alexwizp alexwizp marked this pull request as ready for review January 4, 2021 10:21
@alexwizp alexwizp requested a review from a team as a code owner January 4, 2021 10:21
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jan 4, 2021

Pinging @elastic/kibana-app (Team:KibanaApp)

alexwizp
alexwizp commented Jan 4, 2021
View reviewed changes
test/api_integration/apis/index_patterns/index_pattern_crud/update_index_pattern/main.ts
expect(response3.body.index_pattern.typeMeta).to.eql({ foo: 'baz' });
});

it('can update index_pattern fields', async () => {
Copy link
Copy Markdown
Contributor Author

@alexwizp alexwizp Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

have no idea how it worked before but if I'm not wrong users should not be able to change field name/type.

mattkime
mattkime approved these changes Jan 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Approved pending passing tests and more accurate description - 'Index pattern cache should be local instance, not global'

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Jan 4, 2021 •
edited
Loading

@mattkime could you please help me with understanding our indexPatterns API.
Please see my example with sending 2 queries.

# First query create a new indexpattern with 2 fields: foo, bar. 

const response = await supertest.post('/api/index_patterns/index_pattern').send({
        index_pattern: {
          title,
          fields: {
            foo: {
              name: 'foo',
              type: 'string',
            },
            bar: {
              name: 'bar',
              type: 'number',
              count: 123,
              script: '',
              esTypes: ['test-type'],
              scripted: true,
            },
          },
        },
      });

in response I see that index was successfully created with 2 fields.

Next query just a getting an indexPattern by id from step 1;

   const response1 = await supertest
        .get('/api/index_patterns/index_pattern/' + response.body.index_pattern.id)
        .send();

I expect to see 2 fields like in response from step 1. But I see only one:
image

What is the correct behavior? I honestly think these are just poorly written integration tests cause user should not be able somehow to modify fields (except scripted fields).

@mattkime
Copy link
Copy Markdown
Contributor

mattkime commented Jan 4, 2021

@alexwizp You are correct - the user can't add fields aside from scripted fields.

Its valid to ignore the non scripted fields.

stratoula
stratoula approved these changes Jan 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Nice catch @alexwizp, LGTM

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Jan 5, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Jan 5, 2021

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47252 48012 +760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 1003.4KB 1003.5KB +111.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alexwizp alexwizp merged commit 7ed089c into elastic:master Jan 5, 2021
@alexwizp alexwizp mentioned this pull request Jan 5, 2021
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jan 5, 2021
@alexwizp @kibanamachine
[Data plugin] Wrong caching for Index pattern fields (elastic#87116)
9531931 
* [Data plugin] Wrong caching for Index pattern fields

Closes: elastic#84666

* remove can update index_pattern fields test

* fix tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
@alexwizp alexwizp mentioned this pull request Jan 5, 2021
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jan 5, 2021
@alexwizp @kibanamachine
[Data plugin] Wrong caching for Index pattern fields (elastic#87116)
7ee4234 
* [Data plugin] Wrong caching for Index pattern fields

Closes: elastic#84666

* remove can update index_pattern fields test

* fix tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
alexwizp added a commit that referenced this pull request Jan 6, 2021
@alexwizp @kibanamachine
[Data plugin] Wrong caching for Index pattern fields (#87116) (#87376)
41aab66 
* [Data plugin] Wrong caching for Index pattern fields

Closes: #84666

* remove can update index_pattern fields test

* fix tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
alexwizp added a commit that referenced this pull request Jan 6, 2021
@alexwizp @kibanamachine
[Data plugin] Wrong caching for Index pattern fields (#87116) (#87378)
457a8e8 
* [Data plugin] Wrong caching for Index pattern fields

Closes: #84666

* remove can update index_pattern fields test

* fix tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/common/index_patterns/index_patterns/index_patterns.ts
@alexwizp alexwizp mentioned this pull request Apr 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattkime mattkime mattkime approved these changes

@stratoula stratoula stratoula approved these changes

@ppisljar ppisljar Awaiting requested review from ppisljar

Assignees

@alexwizp alexwizp

Labels

Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.12.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Data plugin] Wrong caching for Index pattern fields

5 participants

@alexwizp @elasticmachine @mattkime @kibanamachine @stratoula