Fixes/msgpack default #783
Conversation
- raise proper exception when send_packet is called with invalid protocol - fix missing 'optional' flag for query() parameters - fix shadowed parameters response and filter - add extra msgpack test starting from json with int, float and string values - default to old behaviour for client (don't use msgpack by default - see failing test)
… is int. - retention policy creation : cast to int, adjust docs - client_test : some tests using @raises(Exception) were succeeding for the wrong reason (assertion in mock function): using self.assertRaises() instead. - test all functions with msgpack enabled and disabled (2 clients)
| else: | ||
| self._headers = { | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'text/plain' |
There was a problem hiding this comment.
Why text/plain and not application/json ?
There was a problem hiding this comment.
You would have to ask the original implementor - this is the way it was implemented before moving the default to msgpack.
What is this issue you identified ? |
| client_1 = InfluxDBClient('localhost', self.influxd_inst.http_port, | ||
| 'root', '', database='db', use_msgpack=False) |
There was a problem hiding this comment.
I don't think this test should be added. There is a bug in influxdb (influxdata/influxdb#8707) that causes the JSON response to be parsed as the wrong datatype.
There was a problem hiding this comment.
Like I said in the description :
The test test_write_points_mixed_type fails on purpose now because of a difference between the json and msgpack result returned by the server.
I can remove the defaulting to json and just make sure all tests run with both json and msgpack client.
I think users would want to be made aware of the consequences of defaulting to msgpack.
There was a problem hiding this comment.
The consequences you are talking about is a bug that is now fixed with floats being misinterpreted as integers. I agree with you that this bugfix should be mentioned is the change log.
In any case, you don't want to have test_write_points_mixed_type run with the json client, because it fails due to a bug in influxdb, not in this library.
| 'time': '2009-11-10T23:02:00Z', | ||
| "host": "server01", | ||
| "region": "us-west"}, | ||
| {'value': 1, |
There was a problem hiding this comment.
The fact that influxdb returns 1 instead of 1.0 here is a bug. I don't think you should assert that the bug is present. A future version of influxdb could fix that bug, and you wouldn't want the tests too break then. I think you should just remove this assertion.
There was a problem hiding this comment.
The issue you mentioned was closed without a fix, so I doubt this will change soon. It is a result of the default json marshaling in GoLang (they could probably work around it by customizing).
In contrast to you I believe it would be a good thing that the test would fail if the expected result would suddenly change (since the influx versions used to test are pinned it should not).
There was a problem hiding this comment.
it would be a good thing that the test would fail if the expected result would suddenly change
Yes, but the expected result in this case is 1.0, not 1, since the database contains a float. The problem is that the library does not return the expected result. I don't think this buggy behavior should be enforced by a test.
There was a problem hiding this comment.
Both are the same Number in JSON, which is the reason the issue was closed in InfluxDB.
I did remove the JSON result from the test - altough I still think it should be there, because it is a way to document the current and future behaviour.
There was a problem hiding this comment.
Yes, both are the same Number in JSON, but that does not make this less of a bug 😛 Python has a different type for int and float, and so does InfluxDB.
And it is indeed a good idea to document the problematic behavior. Maybe you could add something to the doc comments ?
There was a problem hiding this comment.
I noticed I forgot to add the use_msgpack param docstring to InfluxDBClient doc, so I just pushed a commit documenting that paramter.
Is there a specific place you want me to describe the msgpack/Json difference?
There was a problem hiding this comment.
The doc string of the influxdb client seems like a good place to me.
Also, I'm not sure it's clear: I do not have commit rights on this repository. None of the persons with commit rights are active on the repository anymore.
msgpack issue
I identified an issue with the move to the default use of msgpack, so added this pull request to revert to the old behaviour, and optionally enable msgpack by setting
use_msgpack=Truein the init function of the client.The test
test_write_points_mixed_typefails on purpose now because of a difference between the json and msgpack result returned by the server. The question is : is moving to msgpack going to be a breaking change? If so, I propose to merge this pull request (after fixing the test) and let users manually upgrade to msgpack based parsing.This pull request has a couple of other changes identified while getting to the bottom of the above issue.
Fixes
(assertion in mock function): using self.assertRaises() instead.
Others
Contributor checklist