-
Notifications
You must be signed in to change notification settings - Fork 80
Fix whitespace handling #53
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
Conversation
The whitespaces in tags and fields must be escaped with a backslash instead of surrounding it with double quotes.
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.
Thanks for opening this PR 🚀
I left some comments about behavior specified in the InfluxDb Line Protocol docs. I think it makes sense to finally properly implement the escaping guidelines :)
@@ -121,7 +121,7 @@ impl Display for Type { | |||
Float(x) => write!(f, "{}", x), | |||
SignedInteger(x) => write!(f, "{}", x), | |||
UnsignedInteger(x) => write!(f, "{}", x), | |||
Text(text) => write!(f, "\"{text}\"", text = text), | |||
Text(text) => write!(f, "{text}", text = text.replace(" ", "\\ ")), |
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.
What was the reason for removing these quotes? InfuxDb Docs says on quoting
Element | Double quotes | Single quotes |
---|---|---|
Timestamp | Never | Never |
Measurements, tag keys, tag values, field keys | Never* | Never* |
Field values | Double quote string field values. Do not double quote floats, integers, or Booleans. | Never |
* InfluxDB line protocol allows users to double and single quote measurement names, tag keys, tag values, and field keys. It will, however, assume that the double or single quotes are part of the name, key, or value. This can complicate query syntax (see the example below).
I think it would be best if these guidelines were implemented. This will possibly break existing uses of this library as quoting field names is not recommended, but we did so far.
For escaping special characters, InfluxDb Docs suggest the following:
In string field values, you must escape:
- double quotes
- backslash character
In tag keys, tag values, and field keys, you must escape:
- commas
- equal signs
- spaces
For example, , escapes a comma.
In measurements, you must escape:
- commas
- spaces
You do not need to escape other special characters.
That means we can safely ignore \r
, \n
, and \t
.
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.
I wanted to replace my existing python application and I wanted to keep the existing measurements. The python library doesn't add any kind of quotes. Whitespaces will be escaped with backslashes.
As the InfluxDB docs says, the quotes are part of the name. If I use the rust library I cannot append any data with the same tag key as before. The double quotes are always added. This was the reason for removing the quotes.
On the other hand, if someone updates the library to the new version the user must add the quotes in the application (if necessary). But this should be easy. I think adding the double quotes in the library code is an unnecessary restriction.
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.
But...
there is a problem with the field values. The Type formatter is also used for the field values. If the field value is a string it must be quoted with double quotes. I think this is actually wrong and should be fixed prior.
Also the escaping should be completed.
The whitespaces in tags and fields must be escaped with a backslash instead of surrounding it with double quotes.
I am not sure how to handle other special characters like \r, \n, or \t. This is not handled currently.
Closes #52