chore: Align the field for mirrornode response with specification - #239
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Note
|
| Layer / File(s) | Summary |
|---|---|
Response record contracts hiero-enterprise-base/src/main/java/org/hiero/base/data/* |
Response records now define nullable identifiers, timestamps, numeric values, and optional fields. ChunkInfo removes nonce and scheduled. |
MicroProfile JSON conversion and validation hiero-enterprise-microprofile/src/main/java/.../MirrorNodeJsonConverterImpl.java, hiero-enterprise-microprofile/src/test/java/.../MirrorNodeJsonConverterTest.java, hiero-enterprise-microprofile/src/test/resources/json/* |
The converter handles optional fields, precise timestamps, chunked topic messages, custom fees, and invalid array values. Tests and fixtures cover the converted resources. |
Spring JSON conversion and validation hiero-enterprise-spring/src/main/java/.../MirrorNodeJsonConverterImpl.java, hiero-enterprise-spring/src/test/java/.../MirrorNodeJsonConverterTest.java, hiero-enterprise-spring/src/test/resources/json/* |
The Spring converter applies the same nullable-field, timestamp, chunk, fee, and array handling. Tests and fixtures cover the supported response types. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Mergeability Score: 🟠 High · up to 44772
The current implementation can fail to parse valid Mirror Node responses when nullable fields are absent and can return incorrect binary data by preserving Base64 text instead of decoding it. Because these issues affect runtime behavior in both supported implementations, the PR is not merge-ready until they are corrected.
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check | The PR aligns nullable response fields and adds fixtures, but it does not add the required AccountInfo fields or HIP-1313 TransactionInfo fields from [#238]. |
Add the missing AccountInfo fields and HIP-1313 fields to TransactionInfo, with converter support and tests. | |
| Docstring Coverage | Docstring coverage is 13.87% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the main change: aligning Mirror Node response fields with the specification. |
| Description check | ✅ Passed | The description explains the nullable-field alignment, testing fixtures, and linked issue, which match the changeset. |
| Out of Scope Changes check | ✅ Passed | The converter updates, response records, documentation comments, and JSON fixtures all support REST specification alignment and parsing tests. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
8aac711 to
47fd194
Compare
manishdait
changed the title
eb2a558 to
d45373c
Compare
|
Hey @manishdait 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #238 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d7e2f5e3-d653-4ef1-800f-0af6812fffdf
📒 Files selected for processing (57)
hiero-enterprise-base/src/main/java/org/hiero/base/data/AccountInfo.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Balance.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Block.javahiero-enterprise-base/src/main/java/org/hiero/base/data/ChunkInfo.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Contract.javahiero-enterprise-base/src/main/java/org/hiero/base/data/CustomFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/FixedFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/FractionalFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/RoyaltyFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/StakingRewardTransfer.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TimestampRange.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Token.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TokenInfo.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TokenTransfer.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Topic.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TopicMessage.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Transfer.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/MirrorNodeJsonConverterImpl.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/MirrorNodeJsonConverterTest.javahiero-enterprise-microprofile/src/test/resources/json/account-info.jsonhiero-enterprise-microprofile/src/test/resources/json/block-list.jsonhiero-enterprise-microprofile/src/test/resources/json/block.jsonhiero-enterprise-microprofile/src/test/resources/json/contract-list.jsonhiero-enterprise-microprofile/src/test/resources/json/contract.jsonhiero-enterprise-microprofile/src/test/resources/json/exchange-rate.jsonhiero-enterprise-microprofile/src/test/resources/json/network-fee.jsonhiero-enterprise-microprofile/src/test/resources/json/network-stake.jsonhiero-enterprise-microprofile/src/test/resources/json/network-supply.jsonhiero-enterprise-microprofile/src/test/resources/json/nft-list.jsonhiero-enterprise-microprofile/src/test/resources/json/nft.jsonhiero-enterprise-microprofile/src/test/resources/json/token-info.jsonhiero-enterprise-microprofile/src/test/resources/json/token-list.jsonhiero-enterprise-microprofile/src/test/resources/json/topic-message-list.jsonhiero-enterprise-microprofile/src/test/resources/json/topic-message.jsonhiero-enterprise-microprofile/src/test/resources/json/topic.jsonhiero-enterprise-microprofile/src/test/resources/json/transaction-list.jsonhiero-enterprise-microprofile/src/test/resources/json/transaction.jsonhiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/MirrorNodeJsonConverterImpl.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/MirrorNodeJsonConverterTest.javahiero-enterprise-spring/src/test/resources/json/account-info.jsonhiero-enterprise-spring/src/test/resources/json/block-list.jsonhiero-enterprise-spring/src/test/resources/json/block.jsonhiero-enterprise-spring/src/test/resources/json/contract-list.jsonhiero-enterprise-spring/src/test/resources/json/contract.jsonhiero-enterprise-spring/src/test/resources/json/exchange-rate.jsonhiero-enterprise-spring/src/test/resources/json/network-fee.jsonhiero-enterprise-spring/src/test/resources/json/network-stake.jsonhiero-enterprise-spring/src/test/resources/json/network-supply.jsonhiero-enterprise-spring/src/test/resources/json/nft-list.jsonhiero-enterprise-spring/src/test/resources/json/nft.jsonhiero-enterprise-spring/src/test/resources/json/token-info.jsonhiero-enterprise-spring/src/test/resources/json/token-list.jsonhiero-enterprise-spring/src/test/resources/json/topic-message-list.jsonhiero-enterprise-spring/src/test/resources/json/topic-message.jsonhiero-enterprise-spring/src/test/resources/json/topic.jsonhiero-enterprise-spring/src/test/resources/json/transaction-list.jsonhiero-enterprise-spring/src/test/resources/json/transaction.json
78350f5 to
63995ba
Compare
63995ba to
024fb8b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 57ff472d-086c-49ee-90b3-6371cffc7a76
📒 Files selected for processing (57)
hiero-enterprise-base/src/main/java/org/hiero/base/data/AccountInfo.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Balance.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Block.javahiero-enterprise-base/src/main/java/org/hiero/base/data/ChunkInfo.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Contract.javahiero-enterprise-base/src/main/java/org/hiero/base/data/CustomFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/FixedFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/FractionalFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/RoyaltyFee.javahiero-enterprise-base/src/main/java/org/hiero/base/data/StakingRewardTransfer.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TimestampRange.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Token.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TokenInfo.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TokenTransfer.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Topic.javahiero-enterprise-base/src/main/java/org/hiero/base/data/TopicMessage.javahiero-enterprise-base/src/main/java/org/hiero/base/data/Transfer.javahiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/MirrorNodeJsonConverterImpl.javahiero-enterprise-microprofile/src/test/java/org/hiero/microprofile/test/MirrorNodeJsonConverterTest.javahiero-enterprise-microprofile/src/test/resources/json/account-info.jsonhiero-enterprise-microprofile/src/test/resources/json/block-list.jsonhiero-enterprise-microprofile/src/test/resources/json/block.jsonhiero-enterprise-microprofile/src/test/resources/json/contract-list.jsonhiero-enterprise-microprofile/src/test/resources/json/contract.jsonhiero-enterprise-microprofile/src/test/resources/json/exchange-rate.jsonhiero-enterprise-microprofile/src/test/resources/json/network-fee.jsonhiero-enterprise-microprofile/src/test/resources/json/network-stake.jsonhiero-enterprise-microprofile/src/test/resources/json/network-supply.jsonhiero-enterprise-microprofile/src/test/resources/json/nft-list.jsonhiero-enterprise-microprofile/src/test/resources/json/nft.jsonhiero-enterprise-microprofile/src/test/resources/json/token-info.jsonhiero-enterprise-microprofile/src/test/resources/json/token-list.jsonhiero-enterprise-microprofile/src/test/resources/json/topic-message-list.jsonhiero-enterprise-microprofile/src/test/resources/json/topic-message.jsonhiero-enterprise-microprofile/src/test/resources/json/topic.jsonhiero-enterprise-microprofile/src/test/resources/json/transaction-list.jsonhiero-enterprise-microprofile/src/test/resources/json/transaction.jsonhiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/MirrorNodeJsonConverterImpl.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/MirrorNodeJsonConverterTest.javahiero-enterprise-spring/src/test/resources/json/account-info.jsonhiero-enterprise-spring/src/test/resources/json/block-list.jsonhiero-enterprise-spring/src/test/resources/json/block.jsonhiero-enterprise-spring/src/test/resources/json/contract-list.jsonhiero-enterprise-spring/src/test/resources/json/contract.jsonhiero-enterprise-spring/src/test/resources/json/exchange-rate.jsonhiero-enterprise-spring/src/test/resources/json/network-fee.jsonhiero-enterprise-spring/src/test/resources/json/network-stake.jsonhiero-enterprise-spring/src/test/resources/json/network-supply.jsonhiero-enterprise-spring/src/test/resources/json/nft-list.jsonhiero-enterprise-spring/src/test/resources/json/nft.jsonhiero-enterprise-spring/src/test/resources/json/token-info.jsonhiero-enterprise-spring/src/test/resources/json/token-list.jsonhiero-enterprise-spring/src/test/resources/json/topic-message-list.jsonhiero-enterprise-spring/src/test/resources/json/topic-message.jsonhiero-enterprise-spring/src/test/resources/json/topic.jsonhiero-enterprise-spring/src/test/resources/json/transaction-list.jsonhiero-enterprise-spring/src/test/resources/json/transaction.json
024fb8b to
2d17e9f
Compare
manishdait
added
the
status: ready-for-review
2d17e9f to
447723d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/MirrorNodeJsonConverterImpl.java (1)
455-455: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard the nullable
deletedflag.Line 455 calls
getBoolean("deleted")without a null check. A response with"deleted": nullthrows and fails the whole conversion. The Spring implementation useshasNonNull(...) && ...at line 576, so the two modules also disagree.Proposed fix
- final boolean deleted = jsonObject.getBoolean("deleted"); + final boolean deleted = + hasNonNull(jsonObject, "deleted") && jsonObject.getBoolean("deleted");hiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/MirrorNodeJsonConverterImpl.java (1)
605-608: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not convert an absent
fee_exempt_key_listinto an empty list.Line 606 hides a malformed response. Based on learnings,
GET /api/v1/topics/{topicId}always returnsfee_exempt_key_listas an array, including[]. CalljsonArrayToStream(node.get("fee_exempt_key_list"))directly. The MicroProfile implementation has the same fallback at lines 491-496.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8a315cd-8680-40d9-8d7f-c13f7c345087
📒 Files selected for processing (2)
hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/MirrorNodeJsonConverterImpl.javahiero-enterprise-spring/src/main/java/org/hiero/spring/implementation/MirrorNodeJsonConverterImpl.java
Ndacyayisenga-droid
left a comment
Ndacyayisenga-droid
left a comment
There was a problem hiding this comment.
Thanks for the great work @manishdait.
the .json files are raw mirror-node responses for parsing tests. Should we expect these to be updated when the REST spec or mirror-node implementation changes?. Am curious to know if tests will break if anything changes in mirror-node responses
They are currently hardcoded. so if the mirror spec changes, for eg. if fields are added, removed, or renamed, we would need to update these files accordingly. Whether the tests break if they are not updated depends on the type of change. so if a field changes from non nullable to nullable, or a non nullable field is renamed/removed, the parsing tests would likely fail. If new fields are added while the existing fields remain unchanged, the tests should generally continue to pass. |
Signed-off-by: Manish Dait <daitmanish88@gmail.com> # Conflicts: # hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/MirrorNodeJsonConverterImpl.java
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com> # Conflicts: # hiero-enterprise-microprofile/src/main/java/org/hiero/microprofile/implementation/MirrorNodeJsonConverterImpl.java
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
447723d to
adebf02
Compare
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
458a228 to
2fc7699
Compare
There was a problem hiding this comment.
@manishdait Awesome job!!
Can you resolve the open conversations for merge? Thank you!!! :)
aceppaluni
added
status: merge-ready
4 participants
Description:
This PR align the mirrornode response record to match the nullable/non-nullable fields with the rest api specifications.
Changes Made:
Related issue(s):
Fixes #238
Notes for reviewer:
The
.jsonfiles only contains the raw json response from the mirrornode api to unit test the parsing.Checklist