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 issue where selected columns persist when creating a new search #10357

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

lukasolson
Copy link
Member

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 from config.get('defaultColumns') (and not a copy of this array). This is bad because the directives that use the columns variable directly modify it, meaning that the next time that config.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.

Copy link
Member

@weltenwort weltenwort left a 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?

@weltenwort
Copy link
Member

The CI failure seems to be an unrelated error while uploading the artifacts to S3.

@weltenwort
Copy link
Member

jenkins, test this

@lukasolson
Copy link
Member Author

Would it be worth it to change config.get to always return deep copies?

@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 config.get that it might introduce other problems.

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasolson
Copy link
Member Author

lukasolson commented Feb 16, 2017

Backported to 5.x (5.4) in a3f0b8e.

@odmby
Copy link

odmby commented Aug 30, 2017

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.

@Bargs
Copy link
Contributor

Bargs commented Aug 30, 2017

@odmby there's a separate ticket for that issue #4909

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.

Columns persist in a saved search when creating a new search
4 participants