fix: allow nullable owner in Nft record and handle null account_id for burned NFTs#241
fix: allow nullable owner in Nft record and handle null account_id for burned NFTs#241Twiineenock wants to merge 3 commits into
Conversation
…r burned NFTs Signed-off-by: Twiineenock <twiineenockfox@gmail.com>
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Nft record contract and validation tests hiero-enterprise-base/src/main/java/org/hiero/base/data/Nft.java, hiero-enterprise-base/src/test/java/org/hiero/base/test/NftTest.java |
owner is annotated as nullable, burned NFTs are documented, owner null validation is removed, and constructor/accessor validation is tested. |
Mirror-node null owner parsing and coverage hiero-enterprise-microprofile/.../MirrorNodeJsonConverterImpl.java, hiero-enterprise-spring/.../MirrorNodeJsonConverterImpl.java, hiero-enterprise-spring/.../MirrorNodeJsonConverterImplTest.java |
Both converters handle JSON-null account_id; tests cover live and burned NFTs, empty inputs, missing arrays, and null-node handling. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Sequence Diagram(s)
sequenceDiagram
participant MirrorNode
participant MirrorNodeJsonConverterImpl
participant Nft
participant NftTest
MirrorNode->>MirrorNodeJsonConverterImpl: NFT JSON with account_id null
MirrorNodeJsonConverterImpl->>MirrorNodeJsonConverterImpl: Detect null account_id
MirrorNodeJsonConverterImpl->>Nft: Construct Nft(owner=null)
Nft-->>MirrorNodeJsonConverterImpl: Nullable Nft instance
NftTest->>Nft: Validate nullable owner and constructor rules
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main fix: supporting nullable NFT owners and null account_id parsing for burned NFTs. |
| Description check | ✅ Passed | The description is directly related to the changes and accurately describes the burned NFT null-handling fix. |
| Linked Issues check | ✅ Passed | The changes address #237 by preventing null account_id parsing failures and adding coverage for burned NFTs. |
| Out of Scope Changes check | ✅ Passed | The added nullable model change, parser updates, and tests all support the burned NFT fix and do not appear out of scope. |
✨ 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.
…ull account_id handling Signed-off-by: Twiineenock <twiineenockfox@gmail.com>
…ConverterImplTest Signed-off-by: Twiineenock <twiineenockfox@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c367ee47-a04b-4b1a-93f1-d39faea3eb81
📒 Files selected for processing (2)
hiero-enterprise-base/src/test/java/org/hiero/base/test/NftTest.javahiero-enterprise-spring/src/test/java/org/hiero/spring/test/MirrorNodeJsonConverterImplTest.java
| @Test | ||
| void toNft_withValidAccountId_returnsNftWithOwner() throws Exception { | ||
| // given — a live NFT with a non-null account_id | ||
| final ObjectNode node = objectMapper.createObjectNode(); | ||
| node.put("token_id", TOKEN_ID); | ||
| node.put("account_id", ACCOUNT_ID); | ||
| node.put("serial_number", SERIAL); | ||
| node.put("metadata", METADATA_BASE64); | ||
|
|
||
| // when | ||
| final Optional<Nft> result = converter.toNft(node); | ||
|
|
||
| // then | ||
| Assertions.assertTrue(result.isPresent()); | ||
| final Nft nft = result.get(); | ||
| Assertions.assertEquals(TokenId.fromString(TOKEN_ID), nft.tokenId()); | ||
| Assertions.assertEquals(SERIAL, nft.serial()); | ||
| Assertions.assertEquals(AccountId.fromString(ACCOUNT_ID), nft.owner()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider asserting metadata round-trip in the toNft tests.
Both toNft_withValidAccountId_returnsNftWithOwner and toNft_withNullAccountId_returnsNftWithNullOwner set METADATA_BASE64 in the JSON node but don't verify that nft.metadata() decodes back to the original METADATA bytes. The converter uses node.get("metadata").binaryValue() (base64 decode), so a regression in that decoding path would go undetected.
🧪 Suggested addition
Assertions.assertEquals(SERIAL, nft.serial());
Assertions.assertEquals(AccountId.fromString(ACCOUNT_ID), nft.owner());
+ Assertions.assertArrayEquals(METADATA, nft.metadata());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void toNft_withValidAccountId_returnsNftWithOwner() throws Exception { | |
| // given — a live NFT with a non-null account_id | |
| final ObjectNode node = objectMapper.createObjectNode(); | |
| node.put("token_id", TOKEN_ID); | |
| node.put("account_id", ACCOUNT_ID); | |
| node.put("serial_number", SERIAL); | |
| node.put("metadata", METADATA_BASE64); | |
| // when | |
| final Optional<Nft> result = converter.toNft(node); | |
| // then | |
| Assertions.assertTrue(result.isPresent()); | |
| final Nft nft = result.get(); | |
| Assertions.assertEquals(TokenId.fromString(TOKEN_ID), nft.tokenId()); | |
| Assertions.assertEquals(SERIAL, nft.serial()); | |
| Assertions.assertEquals(AccountId.fromString(ACCOUNT_ID), nft.owner()); | |
| } | |
| `@Test` | |
| void toNft_withValidAccountId_returnsNftWithOwner() throws Exception { | |
| // given — a live NFT with a non-null account_id | |
| final ObjectNode node = objectMapper.createObjectNode(); | |
| node.put("token_id", TOKEN_ID); | |
| node.put("account_id", ACCOUNT_ID); | |
| node.put("serial_number", SERIAL); | |
| node.put("metadata", METADATA_BASE64); | |
| // when | |
| final Optional<Nft> result = converter.toNft(node); | |
| // then | |
| Assertions.assertTrue(result.isPresent()); | |
| final Nft nft = result.get(); | |
| Assertions.assertEquals(TokenId.fromString(TOKEN_ID), nft.tokenId()); | |
| Assertions.assertEquals(SERIAL, nft.serial()); | |
| Assertions.assertEquals(AccountId.fromString(ACCOUNT_ID), nft.owner()); | |
| Assertions.assertArrayEquals(METADATA, nft.metadata()); | |
| } |
The Hedera mirror node returns
"account_id": nullfor burned NFTs — this is correct,documented behaviour. A burned NFT still exists as an on-chain record but has no owner.
Previously,
MirrorNodeJsonConverterImpl.toNft()calledAccountId.fromString(node.get("account_id").asText())without a null check, causingAccountId.fromString("null")to throw, which propagated as aJsonParseException(500)whenever a token's NFT list contained burned serial numbers.
Changes:
@Nullable AccountId ownerinNft.java— accurately models the Hedera data modelwhere burned NFTs have no owner
MirrorNodeJsonConverterImpl.toNft()before parsingaccount_idMirrorNodeJsonConverterImpl.toNft()for consistencyRelated issue(s):
Fixes #237
Notes for reviewer:
The fix is consistent with how every other nullable account field is already handled in the
same files (e.g.
receiver_account_id,collector_account_idall have null guards).account_idon the NFT object was the only field missing this guard.Screenshot
Checklist
account_idcan be null)