Fixes/msgpack default by jgspiro · Pull Request #783 · influxdata/influxdb-python · GitHub

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=True in the init function of the client.

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. 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

raise proper exception when send_packet is called with invalid protocol (accessing uninitialzed member)
docstring : fix missing 'optional' flags for query() parameters
fix shadowed parameters response and filter
client_test : some tests using @raises(Exception) were succeeding for the wrong reason
(assertion in mock function): using self.assertRaises() instead.

Others

test all client functions with both msgpack enabled and disabled
add extra msgpack test starting from json with int, float and string values
retention policy creation : cast replication to int, adjust docs

Contributor checklist

Builds are passing --> waiting for feedback on msgpack issue
New tests have been added (for feature additions)

- 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)

Why text/plain and not application/json ?

You would have to ask the original implementor - this is the way it was implemented before moving the default to msgpack.

I identified an issue with the move to the default use of msgpack

What is this issue you identified ?

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.

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.

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.

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.

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).

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.

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.

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 ?

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?

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.

jgspiro added 2 commits February 3, 2020 14:55

jgspiro requested review from aviau, sebito91 and xginn8 as code owners February 4, 2020 13:11

Fix client tests that are failing.

088da3c

lovasoa reviewed Feb 21, 2020

View reviewed changes

jgspiro added 2 commits February 21, 2020 15:24

Make msgpack default option, fix demonstrator test.

065ed44

Fix server test test_write_points_mixed_type

9a8b66d

lovasoa suggested changes Feb 24, 2020

View reviewed changes

jgspiro added 2 commits February 24, 2020 11:49

Remove check for JSON result in server test test_write_points_mixed_type

51d0213

Add use_msgpack docstring.

44a26eb

sebito91 self-assigned this Apr 8, 2020

sebito91 added the pending maintainer label Apr 10, 2020

+                      else:
+                          self._headers = {
+                              'Content-Type': 'application/json',
+                              'Accept': 'text/plain'

+                               'time': '2009-11-10T23:02:00Z',
+                               "host": "server01",
+                               "region": "us-west"},
+                              {'value': 1,

		client_1 = InfluxDBClient('localhost', self.influxd_inst.http_port,
		'root', '', database='db', use_msgpack=False)

Fixes/msgpack default by jgspiro · Pull Request #783 · influxdata/influxdb-python · GitHub

Uh oh!

Fixes/msgpack default - #783

Fixes/msgpack default#783jgspiro wants to merge 7 commits intoinfluxdata:masterinfluxdata/influxdb-python:masterfrom jgspiro:fixes/msgpack_defaultjgspiro/influxdb-python:fixes/msgpack_defaultCopy head branch name to clipboard

Conversation

jgspiro commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

msgpack issue

Fixes

Others

Contributor checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lovasoa commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lovasoa Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fixes/msgpack default#783
jgspiro wants to merge 7 commits into
influxdata:masterfrom
jgspiro:fixes/msgpack_default

jgspiro commented Feb 4, 2020 •
edited
Loading

lovasoa commented Feb 21, 2020 •
edited
Loading

lovasoa Feb 24, 2020 •
edited
Loading