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

add escaping for backslashes, fixes #718 #720

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oliverrahner
Copy link

@oliverrahner oliverrahner commented May 24, 2024

Closes #718

Proposed Changes

Backslashes should also be escaped.
This is generally optional in InfluxDB, but in special cases such as the end of a tag value, it might become crucial

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • mvn test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (dc1d442) to head (8f23758).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #720      +/-   ##
============================================
+ Coverage     88.37%   88.39%   +0.01%     
+ Complexity      786      784       -2     
============================================
  Files           174      174              
  Lines          7091     7093       +2     
  Branches        391      391              
============================================
+ Hits           6267     6270       +3     
+ Misses          698      697       -1     
  Partials        126      126              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@bednar bednar 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 so much for your Pull Request!

To ensure that we maintain the quality of our client and to prevent potential regressions in the future, could you please add your example from this issue comment into our integration tests? Specifically, we would appreciate it if you could incorporate it into com.influxdb.client.ITWriteApiBlocking.

This will not only help in verifying the current functionality but also in maintaining robustness through future updates.

Looking forward to your update, and thanks again for your valuable contribution!

Best regards.

@oliverrahner
Copy link
Author

Good thing you asked about the integration tests... turns out my fix doesn't work :-/
I'll work on it.

@oliverrahner
Copy link
Author

So, this probably still is true:
influxdata/docs.influxdata.com-ARCHIVE#2676
I've opened a new docs issue here:
influxdata/docs-v2#5481

Not fully aware of this library's policy, should it try to inform the user about the actual cause of the issue, such as a tag value ending in a backslash? Or should this be left to the Influx daemon (which doesn't do a good job explaining the issue in this case)?

Independent from this question, the actual change introduced with this PR is probably still a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Escaping issue with tag values
4 participants