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

fix bug: mapping from msgpack response to int, Integer, long, Long types works fine #671

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

Conversation

chabapok
Copy link

@chabapok chabapok commented Apr 9, 2020

All of the unit tests of the InfluxDBResultMapper is based on the assumption that all numbers in response from influxd is Double. It will be ok with JSON. But msgpack format is more precise.

So, InfluxDBResultMapper have errors. For example, it throws an exception when mapping number to int, Integer, long, Long types from msgpack responses, but works with json.

This PR fix it (with unit test).

@majst01
Copy link
Collaborator

majst01 commented Apr 10, 2020

Nice catch, one small formatting nit:

[ERROR] /usr/src/mymaven/src/main/java/org/influxdb/impl/InfluxDBResultMapper.java:364:37: 'typecast' is not followed by whitespace. [WhitespaceAfter]
[ERROR] /usr/src/mymaven/src/main/java/org/influxdb/impl/InfluxDBResultMapper.java:385:33: 'typecast' is not followed by whitespace. [WhitespaceAfter]
[ERROR] /usr/src/mymaven/src/main/java/org/influxdb/impl/InfluxDBResultMapper.java:389:33: 'typecast' is not followed by whitespace. [WhitespaceAfter]

@codecov-io
Copy link

codecov-io commented Apr 10, 2020

Codecov Report

Merging #671 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #671   +/-   ##
=========================================
  Coverage     88.18%   88.18%           
  Complexity      722      722           
=========================================
  Files            69       69           
  Lines          2513     2513           
  Branches        268      268           
=========================================
  Hits           2216     2216           
  Misses          208      208           
  Partials         89       89           
Impacted Files Coverage Δ Complexity Δ
...n/java/org/influxdb/impl/InfluxDBResultMapper.java 88.81% <100.00%> (ø) 57.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3367fd...059089b. Read the comment docs.

@fmachado fmachado self-requested a review April 10, 2020 10:50
@fmachado
Copy link
Contributor

I'm reviewing this PR. Please, do not merge.

@BrentonPoke
Copy link

Did anything ever happen with this one?

@kaszperro
Copy link

What is a progress on this?

@gbennardo
Copy link

Hi @fmachado, any update on this PR?

@BrentonPoke
Copy link

I think he stopped working on this a long time ago. That's left a lot of PRs unmerged and most have likely gone to the influxdb-client-java repo.

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.

7 participants