Fix: ToJsonEncoder keeps order fields during encoding map #7237
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the In the test file Sequence Diagram(s)sequenceDiagram
participant User
participant ToJsonEncoder
participant ListMap
User->>ToJsonEncoder: Request to encode map
ToJsonEncoder->>ListMap: Create ListMap from input map
ListMap-->>ToJsonEncoder: Return ListMap
ToJsonEncoder->>ToJsonEncoder: Apply encoding to values
ToJsonEncoder-->>User: Return encoded JSON
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala (1)
105-121: LGTM: Well-structured test case with comprehensive coverageThe test case effectively verifies the order preservation requirement with a good mix of data types and realistic field names.
Consider adding these additional test cases to strengthen the coverage:
- A comparison test with regular
Mapto demonstrate the difference- A test with nested ordered maps to verify order preservation at multiple levels
Example:
it("should preserve order in nested maps") { val nestedMap = ListMap( "first" -> ListMap( "a" -> 1, "b" -> 2 ), "second" -> ListMap( "x" -> 3, "y" -> 4 ) ) encoder.encode(nestedMap) shouldBe Json.obj( "first" -> Json.obj( "a" -> fromInt(1), "b" -> fromInt(2) ), "second" -> Json.obj( "x" -> fromInt(3), "y" -> fromInt(4) ) ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala(2 hunks)utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala(2 hunks)
🔇 Additional comments (5)
utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala (3)
14-14: LGTM! Appropriate choice for ordered map implementation.
The addition of SeqMap import is well-placed and aligns perfectly with the PR's objective of maintaining field order during JSON encoding.
91-96: Excellent implementation with performance optimization!
The changes effectively maintain field order while also improving performance through:
- Using
SeqMap.fromto preserve insertion order - Switching to
mapValuesNowfor eager evaluation instead of the lazymapValues
91-96: Verify behavior with nested maps and special characters in keys
While the implementation looks correct, please ensure it handles these edge cases:
- Nested maps (maps containing maps)
- Maps with special characters in keys
- Maps with null keys or values
Let's verify the test coverage:
✅ Verification successful
Let me gather more specific information about the test coverage by examining the actual test implementations.
Let me check the actual implementation of ToJsonEncoder to see how it handles the edge cases mentioned in the review.
Based on the implementation and test coverage analysis, I can now provide the final response:
Implementation handles all mentioned edge cases correctly
The implementation safely handles all the edge cases mentioned in the review:
- Nested maps: The
encodemethod recursively processes map values throughmapValuesNow(encode), ensuring proper handling of nested maps - Special characters in keys: Keys are converted to strings using
toString, which preserves special characters - Null handling: Both keys and values are properly handled through:
- Null values via the
safeJsonwrapper and explicit null case inencode - The
Optionhandling in the mainencodemethod - The
toStringconversion for keys which safely handles null keys
- Null values via the
The test suite includes verification of:
- Various data types and collections
- Nested structures through the custom encoder tests
- Map key ordering preservation
- Null values and Options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relevant test cases in the codebase
rg -l "ToJsonEncoderSpec" | while read -r file; do
echo "=== $file ==="
rg "test|\"map|\"nested|special.*character|null.*key" "$file"
done
Length of output: 540
Script:
#!/bin/bash
# Get the content of the test file to examine the test cases
cat utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala
Length of output: 5664
Script:
#!/bin/bash
# Get the implementation of ToJsonEncoder
cat utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala
Length of output: 4506
utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala (2)
12-12: LGTM: Import addition is appropriate
The addition of SeqMap import aligns with the PR's objective of maintaining field order during JSON encoding.
105-121: Verify consistency between test and implementation
The test uses ListMap while the implementation (according to the AI summary) uses SeqMap. While both preserve order, we should ensure this difference doesn't cause any issues.
✅ Verification successful
Implementation and test are consistent in handling ordered maps
Both the implementation and test are using order-preserving map types from the Scala collections:
- The implementation uses
SeqMapfromscala.collection - The test uses
ListMapfromscala.collection.immutable
Both types preserve insertion order, and ListMap is actually an immutable implementation of an ordered map, making it a suitable choice for testing. The implementation correctly maintains the contract being tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Map-related type declarations in ToJsonEncoder
ast-grep --pattern 'Map[$_]' utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala
# Search for specific map conversion methods
rg -A 5 'def.*Map' utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala
Length of output: 380
Script:
#!/bin/bash
# Let's check imports to understand the SeqMap type
rg "SeqMap" utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala -B 5
# Also check if ListMap is imported in the test file
rg "ListMap" utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala -B 5
Length of output: 1103
1a5337c to
d048e74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala (3)
14-15: Remove unused importscala.collection.SeqMapThe
SeqMapimport appears to be unused in the code. Consider removing it to keep the imports clean.-import scala.collection.SeqMap import scala.collection.immutable.ListMap
90-96: Consider adding null checks and improving key handlingWhile the implementation correctly preserves order, there are a few potential improvements:
- Add null checking similar to other encode methods
- Consider handling non-string keys more explicitly, as toString might lose important type information
Consider this safer implementation:
private def encodeListMap(value: scala.collection.immutable.ListMap[_, _]): Json = { + if (value == null) return Null val mapWithStringKeys = value.toList.map { case (k, v) => - k.toString -> encode(v) + Option(k).map(_.toString).getOrElse("null") -> encode(v) } fromFields(mapWithStringKeys) }
Line range hint
1-105: Document the order preservation behaviorSince this change introduces order preservation for maps, it should be properly documented:
- Add ScalaDoc to explain the ordering guarantee
- Consider adding an entry to the migration guide if any clients might depend on the previous behavior
Consider adding documentation like this at the class level:
/** * Encodes values to JSON format. * Note: As of version 1.18.1, the encoder preserves the order of fields when encoding ListMap instances. * For other map types, the order is not guaranteed. */ case class ToJsonEncoder(...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala(3 hunks)utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala
🔇 Additional comments (2)
utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala (2)
72-74: LGTM! Correct pattern matching order
The ListMap case is correctly positioned before the general Map cases, ensuring proper type-based pattern matching precedence.
101-105: Verify view optimization impact and consider consistent key handling
The changes look good with the view optimization for large maps. However:
- The key handling should be consistent with the ListMap implementation
- The view optimization's impact should be verified with performance tests
Let's verify the usage patterns to ensure the optimization is beneficial:
0dc7f3d to
8328cfe
Compare
8328cfe to
1943c73
Compare
|
created: #7239 |
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
Tests
Documentation