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 npe when has null value tag field in pojo #598

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

Conversation

flashmouse
Copy link

use addFieldsFromPOJO method build point will cause npe when some tag field hasn't set any value.

@codecov-io
Copy link

codecov-io commented May 27, 2019

Codecov Report

Merging #598 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #598      +/-   ##
============================================
- Coverage     88.19%   88.16%   -0.04%     
  Complexity      709      709              
============================================
  Files            69       69              
  Lines          2449     2450       +1     
  Branches        252      253       +1     
============================================
  Hits           2160     2160              
  Misses          204      204              
- Partials         85       86       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/dto/Point.java 91.5% <50%> (-0.46%) 41 <0> (ø)

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 a56b279...d0fa413. Read the comment docs.

@@ -273,7 +273,9 @@ private void addFieldByAttribute(final Object pojo, final Field field, final Col
}

if (column.tag()) {
this.tags.put(fieldName, (String) fieldValue);
if (fieldValue != null && !((String) fieldValue).isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the null check, but why not adding if empty, that might be on pupose ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sry I dont know what you mean exactly,can you give some code or example...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old behavior just added the value of the field, now it is only added if not empty ?? What is wrong with an empty value, this is a API breakage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

influx tag must be a string key and string value pair.if some field has column annotation :

  • This filed not a String value,I think it's just incorrect usage
  • This filed is a String value, but user havn't set or just give it "",i think we just need not to set this tag in Point.

if this field is an empty String,influxdb-java will also throw exception:
unable to parse 'example,tag1= value1=123i 1559035795995000000': missing tag value

Copy link
Author

@flashmouse flashmouse May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tag.getValue().replace("", "\\ ");

seems could resolved insert empty tag value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @majst01 tell me if you have any idea, my application will use in production soon, i don't want to maintain another branch

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any news?

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.

4 participants