-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Maps] fix double fetch when filter pill is added #63024
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
I am going to remove the |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
tested and works. Thanks for clean fix.
@@ -26,9 +26,13 @@ export class DataRequest { | |||
} | |||
|
|||
getMeta(): DataMeta { | |||
return this.hasData() | |||
? _.get(this._descriptor, 'dataMeta', {}) | |||
: _.get(this._descriptor, 'dataMetaAtStart', {}); |
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.
apart from the bugfix, +1 on the removal of _.get
.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* [Maps] fix double fetch when filter pill is added * remove isDataSyncActive * set dataMetaAtStart to null instead of deleting Co-authored-by: Elastic Machine <[email protected]>
* [Maps] fix double fetch when filter pill is added * remove isDataSyncActive * set dataMetaAtStart to null instead of deleting Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* master: (29 commits) Add test:jest_integration npm script (elastic#62938) [data.search.aggs] Remove service getters from agg types (AggConfig part) (elastic#62548) [Discover] Fix broken setting of bucketInterval (elastic#62939) Disable adding conditions when in alert management context. (elastic#63514) [Alerting] fixes to allow pre-configured actions to be executed (elastic#63432) adding useMemo (elastic#63504) [Maps] fix double fetch when filter pill is added (elastic#63024) [Lens] Fix missing formatting bug in "break down by" (elastic#63288) [SIEM] [Cases] Removed double pasted line (elastic#63507) [Reporting] Improve functional test steps (elastic#63259) [SIEM][CASE] Tests for server's configuration API (elastic#63099) [SIEM] [Cases] Case container unit tests (elastic#63376) [ML] Improving parsing of large uploaded files (elastic#62970) [ML] Listing global calendars on the job management page (elastic#63124) [Ingest][Endpoint] Add Ingest rest api response types for use in Endpoint (elastic#63373) Add help text to form fields (elastic#63165) [ML] Converts utils Mocha tests to Jest (elastic#63132) [Metrics UI] Refactor With* containers to hooks (elastic#59503) [NP] Migrate logstash server side code to NP (elastic#63135) Clicking cancel in saved query save modal doesn't close it (elastic#62774) ...
* [Maps] fix double fetch when filter pill is added * remove isDataSyncActive * set dataMetaAtStart to null instead of deleting Co-authored-by: Elastic Machine <[email protected]>
Fixes #59847
#51713 changed the implementation of
DataRequest.getMeta
but this problem has existed before that change.The problem with the updated implementation of
DataRequest.getMeta
is that it pulled meta fromdataMeta
when data exists. This caused problems when multiple calls to syncData occurred beforestopLoading
could be called on the previous syncData call. Below is the path of execution that caused the problem:startLoading
. This updatesdataMetaAtStart
so filters contains[{some new filter}]
.stopLoading
is not called yet sodataMeta
contains[]
.canSkipSourceUpdate
check gets previous meta fromdataMeta
so filters is[]
.canSkipSourceUpdate
wrongly passes.canSkipSourceUpdate
should have useddataMetaAtStart
.While investigating this PR, I originally thought the problem was with async nature of
canSkipSourceUpdate
and that the multiple calls tosyncData
where getting messed up becausestartLoading
had not been called before the nextsyncData
was called. I addedisDataSyncActive
so that state is set as soon assyncData
is called. This logic does not impact the fix and can be remove if its deemed unnecessary.