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

[Lens] Escape field names in formula #102588

Merged
merged 5 commits into from
Jun 23, 2021

Conversation

wylieconlon
Copy link
Contributor

If your field name contains any invalid characters, it will get added as a quoted string. For example my 'invalid' field becomes 'my \'invalid\' field'

Checklist

@wylieconlon wylieconlon added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 17, 2021
@wylieconlon wylieconlon requested a review from a team June 17, 2021 22:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and works as expected, LGTM

Now we should automatically insert quotes when using these field names in the generate logic, but we can do this separately

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

the only issue I fund was if I start to type the field already with a quote wrapping, selecting the field half-way from the suggestion box will add extra wrapping leading to an error state.

I guess it's something probably not so infrequent if there's a field to escape in the indexpattern, but it's not a blocking feature for this PR:

formula_escape_field

@dej611
Copy link
Contributor

dej611 commented Jun 22, 2021

Still related to this, but it can be addressed in a separated PR as well if this is merged, we should escape incoming fields when transitioning from quick functions:

transition_to_formula_1
transition_to_formula_2

@wylieconlon wylieconlon requested a review from flash1293 June 22, 2021 19:25
@wylieconlon
Copy link
Contributor Author

@flash1293 @dej611 I've updated this PR with fixes for the following cases:

  • Switching from quick function with special characters -> formula
  • Typing a field name with quotes will replace the entire word, not just append

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

Failed twice in exactly the same way:

Kibana spaces page meets a11y validations
a11y test for space selection page:

Probably some upstream issue

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and works as expected, LGTM

@wylieconlon
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1.3MB 1.3MB +69.0B
lens 1.5MB 1.5MB +640.0B
total +709.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
expressions 210.2KB 210.2KB +69.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@wylieconlon wylieconlon merged commit 4d514c6 into elastic:master Jun 23, 2021
@wylieconlon wylieconlon deleted the lens/escape-fields branch June 23, 2021 18:20
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 23, 2021
* [Lens] Escape field names in formula

* Fix handling of partially typed fields with invalid chars

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 25, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 25, 2021
kibanamachine added a commit that referenced this pull request Jun 25, 2021
* [Lens] Escape field names in formula

* Fix handling of partially typed fields with invalid chars

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Wylie Conlon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants