-
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
Fix issue where selected columns persist when creating a new search #10357
Conversation
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.
Nice example of the advantages of immutable state. Would it be worth it to change config.get
to always return deep copies?
The CI failure seems to be an unrelated error while uploading the artifacts to S3. |
jenkins, test this |
@weltenwort I thought about this, but I thought, there might be places that actually rely on this behavior. And there are so many places that use |
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.
LGTM
Backported to 5.x (5.4) in a3f0b8e. |
Similar issue occurs when switching between indexes. Many time different indexes don't share the same fields, thus user needs to remove the old columns one by one. Perhaps a middle ground can be to have a "clear all" button for the selected fields. |
Prior to this PR, when you visited discover and added some columns to your doc table, then clicked on "New" (to create a new search), the columns you selected would persist. This was happening because when you create a new search, if there are no columns selected, then the
columns
variable was being assigned directly to what comes back fromconfig.get('defaultColumns')
(and not a copy of this array). This is bad because the directives that use thecolumns
variable directly modify it, meaning that the next time thatconfig.get('defaultColumns')
was invoked, it would return the modified value.This PR resolves this by returning a copy of the array, rather than the array itself.
Fixes #10233.