influxdb-java icon indicating copy to clipboard operation
influxdb-java copied to clipboard

Request for annotation for null values on pojo fields of type String.

Open BrentonPoke opened this issue 4 years ago • 5 comments

A @Default annotation was requested to protect pojos coming back with null strings in #661. I interpreted it to mean something along the lines of what's in the unit test. Would appreciate feedback.

BrentonPoke avatar Aug 16 '20 12:08 BrentonPoke

Codecov Report

Merging #693 into master will increase coverage by 0.04%. The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #693      +/-   ##
============================================
+ Coverage     88.26%   88.31%   +0.04%     
  Complexity      730      730              
============================================
  Files            69       69              
  Lines          2540     2558      +18     
  Branches        268      277       +9     
============================================
+ Hits           2242     2259      +17     
  Misses          210      210              
- Partials         88       89       +1     
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/dto/Point.java 93.43% <94.44%> (+0.07%) 53.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 125a9ca...f54d76a. Read the comment docs.

codecov-commenter avatar Aug 16 '20 13:08 codecov-commenter

Anyone with the ability to re-run jobs mind running the one that failed? All the others passed.

BrentonPoke avatar Aug 19 '20 19:08 BrentonPoke

@BrentonPoke I restarted the tests, they now pass. I am not an expert in this library, I took a quick look at the new code while restarting the tests. It seems to me that

  • the Default value always overrides any value of the field
  • the new test is not testing the Default annotation, it checks that the set values are transferred to point
  • the new Default annotation always sets a String value even for other supported field types (Double, Integer, Float, BigDecimal, Instant, Boolean), which would certainly be unexpected
  • the relationship to #661 is not clear to me, it seems to me that it is already fixed by https://github.com/influxdata/influxdb-java/commit/6afc6ade28b073af2e53022316ada6a838b41815#diff-bd011207d3113c41cd8be4968d2321cb

sranka avatar Aug 20 '20 04:08 sranka

@BrentonPoke I restarted the tests, they now pass. I am not an expert in this library, I took a quick look at the new code while restarting the tests. It seems to me that

  • the Default value always overrides any value of the field
  • the new test is not testing the Default annotation, it checks that the set values are transferred to point
  • the new Default annotation always sets a String value even for other supported field types (Double, Integer, Float, BigDecimal, Instant, Boolean), which would certainly be unexpected
  • the relationship to #661 is not clear to me, it seems to me that it is already fixed by 6afc6ad#diff-bd011207d3113c41cd8be4968d2321cb

Thanks for responding, I'm taking another look at this since I originally did it early in the morning, but the comment here is what was agreed upon as the desired change. I went off the end of this discussion that resolved the need-more-info label. For the test, I did struggle a bit in terms of trying to develop a test to check that the value is set to the default. I'm trying a Black Box testing approach, but this is my first time processing an annotation, so I'm doing this PR to learn how. I'm changing the processing of the annotation to only trigger on a String variable, but the reason I didn't include numerical values is that it wasn't part of the request agreed upon at the end of #661.

BrentonPoke avatar Aug 20 '20 09:08 BrentonPoke

Ok, I think it's ready for another look, if anyone wants.

BrentonPoke avatar Aug 20 '20 16:08 BrentonPoke