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.

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

Fix saving index pattern when there is a conflict #21947

Merged
merged 4 commits into from
Aug 15, 2018

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Aug 13, 2018

Fixes #21932

Changes in #20384 changed error handling to return kfetch's FetchError object. Saving index pattern is dependent on statusCode, which is not a top level property of FetchError. PR updates to look for res.status instead.

@@ -232,6 +232,16 @@ export class SavedObjectsClient {
return Promise.reject(new Error('body not permitted for GET requests'));
}

return kfetch({ method, pathname: path, query, body: JSON.stringify(body) });
return kfetch({ method, pathname: path, query, body: JSON.stringify(body) })
.catch(resp => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that you are not getting the REST resp from kfetch. kfetch is going to reject the promise with a FetchError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FetchError still has res and body properties:
https://github.com/elastic/kibana/blob/master/src/ui/public/kfetch/kfetch.ts#L29

I'll rename these variables so they reflect an instance of FetchError.

@nreese
Copy link
Contributor

nreese commented Aug 13, 2018

This is going to break places that have been updated to handle a FetchError. I would recommend just updating the consumers to directly handle FetchError.

https://github.com/elastic/kibana/blob/master/src/ui/public/error_auto_create_index/error_auto_create_index.js#L32

https://github.com/elastic/kibana/blob/master/src/ui/public/courier/saved_object/saved_object.js#L305

@jen-huang
Copy link
Contributor Author

@nreese Thanks for those references! I'm concerned about the places that haven't been updated to know about FetchError and are still attempting to read top level statusCode (the only problematic/missing property). Grepping on statusCode shows many results and it's hard to tell if those references are in saved objects context. I'd like to get your assessment on the likelihood of similar breakage in other areas.

I'll update this PR to modify the index pattern client instead in the meantime.

@jen-huang jen-huang changed the title Place back saved object client error formatting Fix saving index pattern when there is a conflict Aug 13, 2018
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review

@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 14, 2018

💔 Build Failed

23:57:29    │ proc  [ftr]        └-> "before all" hook
23:57:29    │ proc  [ftr]        └-> applies the correct CSS classes
23:57:29    │ proc  [ftr]          └-> "before each" hook: global before each
23:57:29    │ proc  [ftr]          │ debg  TestSubjects.find(grokDebugger acePatternInput codeEditorContainer)
23:57:29    │ proc  [ftr]          │ debg  findByCssSelector [data-test-subj~="grokDebugger"] [data-test-subj~="acePatternInput"] [data-test-subj~="codeEditorContainer"]
23:57:29    │ proc  [ftr]          │ debg  TestSubjects.find(grokDebugger acePatternInput codeEditorContainer)
23:57:29    │ proc  [ftr]          │ debg  findByCssSelector [data-test-subj~="grokDebugger"] [data-test-subj~="acePatternInput"] [data-test-subj~="codeEditorContainer"]
23:57:30    │ proc  [ftr]          │ info  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-x-pack/kibana/x-pack/test/functional/screenshots/failure/logstash grok debugger app syntax highlighting applies the correct CSS classes.png"
23:57:30    │ proc  [ftr]          │ info  Current URL is: http://localhost:5640/app/kibana#/dev_tools/grokdebugger?_g=()
23:57:30    │ proc  [ftr]          │ info  Saving page source to: /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-x-pack/kibana/x-pack/test/functional/failure_debug/html/logstash grok debugger app syntax highlighting applies the correct CSS classes.html
23:57:30    │ proc  [ftr]        └- ✖ fail: "logstash grok debugger app syntax highlighting applies the correct CSS classes"
23:57:30    │ proc  [ftr]        │        [GET http://localhost:9515/session/f6ea74747226692705633433d95ce268/element/0.23236615736280375-35/text] stale element reference: element is not attached to the page document

@jen-huang
Copy link
Contributor Author

Retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 14, 2018

💔 Build Failed

Same failure as above. Passes locally 😞Merging master to see if that helps.

@elasticmachine
Copy link
Contributor

elasticmachine commented Aug 14, 2018

💔 Build Failed

Weird ES-related failures. Aborted.

@jen-huang
Copy link
Contributor Author

Retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jen-huang
Copy link
Contributor Author

Retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jen-huang jen-huang merged commit 341d500 into elastic:master Aug 15, 2018
@jen-huang jen-huang deleted the fix/conflict-error branch August 15, 2018 05:21
jen-huang added a commit to jen-huang/kibana that referenced this pull request Aug 15, 2018
@LeeDr
Copy link
Contributor

LeeDr commented Aug 15, 2018

@jen-huang What will happen now in this same case? What will the user see?

In the issue this PR fixes, it could really be any Kibana user that does something that causes a change to the index pattern while you're trying to format a field or add a scripted field which causes the version conflict.
We don't want this user to clobber the previous version change. I'm thinking it should give an error that lets them know they need to refresh the page and try again?

@jen-huang
Copy link
Contributor Author

@LeeDr, the user will see an error that their version is out of date and they need to refresh. This was implemented in #18937, but broke due to some error formatting changes. This PR just adjusts for the updated format and restores the previous functionality 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants