LB-158 / LB-170: Fix listenstore code for users with special characters, add tests and upgrade influx #199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paramsingh
changed the title
|
Clicked |
paramsingh
commented
Jun 17, 2017
listenbrainz/listen.py
Outdated
| @@ -196,7 +197,7 @@ def to_influx(self, measurement): | |||
| # add the user generated keys present in additional info to fields | |||
| for key, value in self.data['additional_info'].items(): | |||
| if key not in Listen.SUPPORTED_KEYS: | |||
| data['fields'][key] = value | |||
| data['fields'][key] = escape(value) | |||
There was a problem hiding this comment.
Did this because additional_info variables may have random newlines in them, which would lead to parse errors.
Influx has different behaviour for the line protocol and for the query language, so we have to use 2 backslashes for each backslash in the line protocol and 4 backslashes for each backslash in the query language. Change the code so that it does that everywhere. Also, because newlines cause parse errors in the line protocol replace newlines in the measurement names and the values of extraneous variables in additional_info with `\n`. I've also added a unit test and an integration test with a user with a weird user name.
Setuptools might have an error that is causing build errors See here: https://travis-ci.org/paramsingh/listenbrainz-server/builds/244051410 The bug seems to be known, see this: pypa/setuptools#951
mayhem
approved these changes
Jun 19, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So I spent the day looking through the influxdb-python source code. And the reason this was so hard to debug is a combination of random weird behaviour in influx with backslashes and some problems in our code.
Influxdb-python automatically escapes measurement names when we use write_points which can be seen here, so when we send a measurement with name
"para\m"to write_points, it shows up in influx as"para\\m". As a result, the line protocol only requires escaping of\ncharacters. On the other hand, the query language requires escaping of everything and since influx had replaced the\in the measurement name with\\, we need FOUR backslashes in the query to get the data.So, I have changed the code so that it does this and added a bunch of comments explaining stuff as best I could. I expect more changes will be requested to make it clearer. Also, I have upgraded to influx 1.2.4 and influxdb-python 4.1.0 .
Also, there is some weird behaviour when we backslash quotes in influx. Influx SOMETIMES totally ignores backslashes present in measurement names, if they come before quotes. This behaviour is present in influx itself (not influxdb-python) and I am not sure if this is a bug or if I am misunderstanding something. For example:
Backslash away from quotes:
Backslash in front of a quote gets ignored
Stuff like this is hard to write code for, unless there is something that I am missing completely...