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

[pkg/ottl] Fix escape sequences in strings #30839

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

quentinmit
Copy link
Contributor

Description:

Fix ottl parsing to properly handle escape sequences.
@TylerHelmuth

Link to tracking Issue:
#23238

Testing:
Added a new unit test with the example from the linked issue

Documentation:
None (pure bugfix)

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Thanks for finding this solution - I remember spending a while on that regex and was unable to come up with a suitable replacement.

Please add some new scenarios in e2e that put strings to the test. Some scenarios I think would be useful:

  • Using a string that is a single backslash
  • Using a string that is 2 backslashes
  • Using a string that is 3 backslashes
  • Using a string that is 4 backslashes
  • A test for the a("\\", "b") scenario.

plus any edge cases you can think of.

@quentinmit
Copy link
Contributor Author

@TylerHelmuth Alright, I added several tests to e2e_test.go for this.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I am happy that the new and existing test cases prove this regex works. @evan-bradley please review

@TylerHelmuth
Copy link
Member

@quentinmit please add a bug_fix changelog entry

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thank you @quentinmit!

Can you update ottlfuncs/README.md to remove all references to #23238?

pkg/ottl/grammar.go Show resolved Hide resolved
@quentinmit
Copy link
Contributor Author

@evan-bradley @TylerHelmuth I added a changelog entry and updated the README. Thanks!

@TylerHelmuth TylerHelmuth merged commit ed487b3 into open-telemetry:main Jan 31, 2024
95 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 31, 2024
@quentinmit quentinmit deleted the ottl-string branch February 1, 2024 16:32
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Fix ottl parsing to properly handle escape sequences.
@TylerHelmuth

**Link to tracking Issue:**
open-telemetry#23238

**Testing:** <Describe what testing was performed and which tests were
added.>
Added a new unit test with the example from the linked issue

**Documentation:** <Describe the documentation added.>
None (pure bugfix)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants