fix: null handling with Structure, Value #663
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
toddbaert
changed the title
toddbaert
commented
Oct 23, 2023
toddbaert
commented
Oct 23, 2023
toddbaert
commented
Oct 23, 2023
Codecov Report
@@ Coverage Diff @@
## main #663 +/- ##
============================================
+ Coverage 94.47% 94.97% +0.49%
- Complexity 362 365 +3
============================================
Files 32 33 +1
Lines 851 856 +5
Branches 51 52 +1
============================================
+ Hits 804 813 +9
+ Misses 24 22 -2
+ Partials 23 21 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This was referenced Oct 23, 2023
beeme1mr
reviewed
Oct 23, 2023
Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Todd Baert <[email protected]>
thisthat
reviewed
Oct 24, 2023
Co-authored-by: Giovanni Liva <[email protected]> Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
fix/npe-asObjectMap
branch
from
274aba2 to
2091fb4
Compare
Signed-off-by: Todd Baert <[email protected]>
toddbaert
commented
Oct 24, 2023
| @@ -134,7 +138,9 @@ default <T extends Structure> Map<String, Value> merge(Function<Map<String, Valu | |||
| */ | |||
| static Structure mapToStructure(Map<String, Object> map) { | |||
| return new MutableStructure(map.entrySet().stream() | |||
| .filter(e -> e.getValue() != null) | |||
There was a problem hiding this comment.
We were FILTERING null values before. This resulted in maps with null entries not having converted (empty) Values. I think that's wrong. With this change, all nulls in maps are converted to null-containing Values and vice-versa.
toddbaert
commented
Oct 24, 2023
Comment on lines
+25
to
+33
| public Map<String, Object> asObjectMap() { | ||
| return attributes | ||
| .entrySet() | ||
| .stream() | ||
| // custom collector, workaround for Collectors.toMap in JDK8 | ||
| // https://bugs.openjdk.org/browse/JDK-8148463 | ||
| .collect(HashMap::new, | ||
| (accumulated, entry) -> accumulated.put(entry.getKey(), convertValue(entry.getValue())), | ||
| HashMap::putAll); |
There was a problem hiding this comment.
This was common to both the MutableContext and ImmutableContext, so I've extracted it to an abstract.
toddbaert
changed the title
toddbaert
commented
Oct 24, 2023
Signed-off-by: Todd Baert <[email protected]>
toddbaert
commented
Oct 24, 2023
| @@ -98,6 +98,20 @@ void mapToStructureTest() { | |||
| assertEquals(new Value(Instant.ofEpochSecond(0)), res.getValue("Instant")); | |||
| assertEquals(new HashMap<>(), res.getValue("Map").asStructure().asMap()); | |||
| assertEquals(new Value(immutableContext), res.getValue("ImmutableContext")); | |||
| assertNull(res.getValue("nullKey")); | |||
| assertEquals(new Value(), res.getValue("nullKey")); | |||
There was a problem hiding this comment.
Modified assertion associated with this change.
Kavindu-Dodan
approved these changes
Oct 24, 2023
thisthat
approved these changes
Oct 24, 2023
Signed-off-by: Todd Baert <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Value->nulland vice versa)