Added the removed LabelApplicationRuleDataDeserializationConfig.java file #189
Conversation
Codecov Report
@@ Coverage Diff @@
## main #189 +/- ##
=========================================
Coverage 22.31% 22.31%
Complexity 75 75
=========================================
Files 68 68
Lines 1788 1788
Branches 54 54
=========================================
Hits 399 399
Misses 1380 1380
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -133,6 +133,9 @@ StringCondition convertStringCondition( | |||
| StringCondition.Builder stringConditionBuilder = | |||
| StringCondition.newBuilder() | |||
| .setOperator(convertStringConditionOperator(stringCondition.operator())); | |||
| if (stringCondition.stringConditionValueType() == null) { | |||
There was a problem hiding this comment.
When stringCondition.stringConditionValueType() was null, the below switch case was throwing NPE. Earlier I thought default will handle null case as well.
There was a problem hiding this comment.
Rather than duplicating the return here, use something like Type type = Optional.ofNullable(condition.stringConditionValueType()).orElse(VALUE);, and then send it into the switch. Also make sure the valueType is annotated @Nullable, we do that so the IDE should have flagged the existing code.
There was a problem hiding this comment.
There was a problem hiding this comment.
Gotcha - make sure your IDE is set up to highlight the issue. We really need to update our CI-run static analysis to check things like this too.
| public List<Module> jacksonModules() { | ||
| return List.of( | ||
| new SimpleModule() | ||
| .addAbstractTypeMapping( |
There was a problem hiding this comment.
Sloppy review on my part - we can't get rid of the whole file since the arg name/schema mapping is still needed above, but all the jackson mappings are unnecessary if they're already declared elsewhere.
| @@ -133,6 +133,9 @@ StringCondition convertStringCondition( | |||
| StringCondition.Builder stringConditionBuilder = | |||
| StringCondition.newBuilder() | |||
| .setOperator(convertStringConditionOperator(stringCondition.operator())); | |||
| if (stringCondition.stringConditionValueType() == null) { | |||
There was a problem hiding this comment.
Rather than duplicating the return here, use something like Type type = Optional.ofNullable(condition.stringConditionValueType()).orElse(VALUE);, and then send it into the switch. Also make sure the valueType is annotated @Nullable, we do that so the IDE should have flagged the existing code.
Added the removed file from this PR: #185
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.