-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Validate timerange before submitting query to ES #69363
[Discover] Validate timerange before submitting query to ES #69363
Conversation
…-17-discover-validate-timerange
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't investigated, but it seems like there is a case where this still crashes at least parts of Discover:
Going to this URL:
app/discover#/?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:nafdsdsfow))&_a=(columns:!(_source),filters:!(),index:'90943e30-9a47-11e8-b64d-95841ca0b247',interval:auto,query:(language:kuery,query:''),sort:!())
Loads discover, but the query bar disappears because an uncaught error is thrown in the react component:
date_modes.js:31 Uncaught TypeError: Cannot read property 'includes' of null
at getDateMode (date_modes.js:31)
at isRelativeToNow (pretty_duration.js:68)
at prettyDuration (pretty_duration.js:110)
at renderSharingMetaFields (query_bar_top_row.tsx:165)
at QueryBarTopRow (query_bar_top_row.tsx:305)
at renderWithHooks (react-dom.development.js:16260)
at mountIndeterminateComponent (react-dom.development.js:18794)
at beginWork$1 (react-dom.development.js:20162)
at HTMLUnknownElement.callCallback (react-dom.development.js:336)
at Object.invokeGuardedCallbackDev (react-dom.development.js:385)
No idea why it behaves different than your example in the description
Just realized this is a separate problem that probably has to be resolved within the query top row component in the data plugin, not within discover. I'm fine with handling that separately, but in that case we should create an issue for it. |
Checked out locally and ran unit/functional tests for it
|
@elasticmachine merge upstream |
thx @flash1293 created an issue, could reproduce it in Discover, Visualize, Dashboard: #69941 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case LGTM as well, solves the unnecessary fetch 👍
💚 Build SucceededBuild metricspage load asset sizebeta
History
To update your PR or re-run it, just comment with: |
* master: (34 commits) Upgrade `elliptic` dependency (`6.5.2` → `6.5.3`). (elastic#70054) [License Management] Do not break when `telemetry.enabled:false` (elastic#69711) [SECURITY] Redirect app/security to app/security/overview (elastic#70005) "Explore underlying data" in-chart action (elastic#69494) Api reference docs for state_containers and state_sync (elastic#67354) prep state transfer for passing embeddables by value to editor and back (elastic#69991) move Metrics API to start (elastic#69787) refactor: 💡 fix typo in embeddable (elastic#69417) [alerting] migrates the old `alerting` consumer to be `alerts` (elastic#69982) [APM]Create API to return data to be used on the Overview page (elastic#69137) [Lens] Fix delete button position in dimension panel for long labels (elastic#69495) [Lens] Add toolbar api (elastic#69263) Fixes bug on color picker defaults on TSVB (elastic#69889) [DOCS] Fixes wording in Upload a CSV section (elastic#69969) [Discover] Validate timerange before submitting query to ES (elastic#69363) [Maps] avoid using MAP_SAVED_OBJECT_TYPE constant when defining URL paths (elastic#69723) [Maps] Fix icon palettes are not working (elastic#69937) [Ingest Manager] Fix typo in constant name (elastic#69919) [test] skip status.allowAnonymous tests on cloud (elastic#69017) Fix backport (elastic#70003) ...
…t-pipelines/editor-error-messages * ingest-pipelines/editor-dropzone-refinement: (122 commits) Fixes for monaco XJSON grammar parser and update form copy Added cancel move button Refactor SCSS values to variables use classNames as it is intended to be used Remove box shadow on all nested tree items Rename variables and prefix booleans with "is" Fixes bug on color picker defaults on TSVB (elastic#69889) [DOCS] Fixes wording in Upload a CSV section (elastic#69969) [Discover] Validate timerange before submitting query to ES (elastic#69363) [Maps] avoid using MAP_SAVED_OBJECT_TYPE constant when defining URL paths (elastic#69723) [Maps] Fix icon palettes are not working (elastic#69937) [Ingest Manager] Fix typo in constant name (elastic#69919) [test] skip status.allowAnonymous tests on cloud (elastic#69017) Fix backport (elastic#70003) [Reporting] ReportingStore module (elastic#69426) Add reporting assets to the eslint ignore file (elastic#69968) [Discover] set minBarHeight for high cardinality data (elastic#69875) Add featureUsage API to licensing context provider (elastic#69838) Fix uncaught typecheck merge conflict (elastic#70001) Use TS to discourage SO mappings with dynamic: false / dynamic: true (elastic#69927) ...
Summary
Fixes entering an URL with an invalid time range in Discover. The time range is validated before the request is submitted, and an error message is displayed.
Fixes #68087
Testing
Try entering the following request in the URL:
http://localhost:5601/{kibanaPrefix}/app/discover#/?_g=(filters:!(),refreshInterval:(pause:!f,value:900000),time:(from:'',to:now-sd))&_a=(columns:!(_source),filters:!(),interval:auto,query:(language:kuery,query:''),sort:!())
Before this fix you should get an exception in master.
In recent Kibana minors, this caused different problems, more about this in #68087
Checklist
Delete any items that are not applicable to this PR.