refactor(framework): replace fastjson with jackson-databind#117
Closed
halibobo1205 wants to merge 1 commit into
Closed
refactor(framework): replace fastjson with jackson-databind#117halibobo1205 wants to merge 1 commit into
halibobo1205 wants to merge 1 commit into
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (child == null || child.isNull()) { | ||
| return null; | ||
| } | ||
| return child.asText(); |
There was a problem hiding this comment.
Suggestion: getString currently uses asText() for all node types, which returns an empty string for object/array elements instead of their JSON text. This silently corrupts values when callers read structured array elements as strings; mirror the object-wrapper behavior by returning toString() for object/array nodes. [logic error]
Severity Level: Major ⚠️
- ⚠️ JSONArray.getString loses structured elements, returning empty string.
- ⚠️ Diverges from JSONObject.getString semantics for nested values.
Suggested change
| return child.asText(); | |
| if (child.isObject() || child.isArray()) { | |
| return child.toString(); | |
| } | |
| return child.asText(null); |
Steps of Reproduction ✅
1. Parse a JSON array containing structured elements using the wrapper entrypoint
`JSON.parseArray(String)` at `common/src/main/java/org/tron/json/JSON.java:101-102`, for
example `JSON.parseArray("[{\"k\":1}, [2]]")`, which returns a `JSONArray` backed by a
Jackson `ArrayNode`.
2. Call `getString(0)` on the returned `JSONArray` instance; this executes
`JSONArray.getString(int index)` at
`common/src/main/java/org/tron/json/JSONArray.java:71-77`.
3. Inside `getString`, the element is obtained as a `JsonNode` (`child = node.get(index)`
at line 72); for an object or array element, `child` is a structured node, not textual.
4. Because the current implementation always returns `child.asText()` at
`JSONArray.java:76`, Jackson returns an empty string for non-textual nodes, so callers see
`""` instead of the JSON text (e.g. `{"k":1}` or `[2]`), unlike
`JSONObject.getString(String key)` in
`common/src/main/java/org/tron/json/JSONObject.java:58-67`, which explicitly returns
`child.toString()` for object/array nodes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONArray.java
**Line:** 76:76
**Comment:**
*Logic Error: `getString` currently uses `asText()` for all node types, which returns an empty string for object/array elements instead of their JSON text. This silently corrupts values when callers read structured array elements as strings; mirror the object-wrapper behavior by returning `toString()` for object/array nodes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| if (child == null || child.isNull()) { | ||
| return null; | ||
| } | ||
| return child.asLong(); |
There was a problem hiding this comment.
Suggestion: Numeric conversion currently uses Jackson's permissive asLong(), which silently returns 0 for non-numeric text (for example "abc"). That masks invalid input and can make bad payloads look like legitimate zero values. Parse textual values explicitly and throw JSONException on invalid casts to preserve fastjson-compatible behavior. [logic error]
Severity Level: Critical 🚨
- ❌ JSONObject.getLong misparses non-numeric text fields as zero.
- ⚠️ Business logic reading longs from JSON sees silent coercion.
Suggested change
| return child.asLong(); | |
| if (child.isNumber()) { | |
| return child.longValue(); | |
| } | |
| if (child.isTextual()) { | |
| String text = child.asText(); | |
| if (text == null || text.isEmpty() || "null".equalsIgnoreCase(text)) { | |
| return null; | |
| } | |
| try { | |
| return Long.parseLong(text); | |
| } catch (NumberFormatException e) { | |
| throw new JSONException("Cannot cast '" + text + "' to Long", e); | |
| } | |
| } | |
| if (child.isBoolean()) { | |
| return child.asBoolean() ? 1L : 0L; | |
| } | |
| throw new JSONException("Cannot cast " + child.getNodeType() + " to Long"); |
Steps of Reproduction ✅
1. In any caller, invoke `JSONObject.parseObject("{\"val\":\"abc\"}")` using the static
factory at `common/src/main/java/org/tron/json/JSONObject.java:36-38`.
2. The factory delegates to `JSON.parseObject` which builds an `ObjectNode` backed
`JSONObject` whose `"val"` field is a textual `JsonNode` with content `"abc"`.
3. Call `getLong("val")` on that instance, executing the method at
`common/src/main/java/org/tron/json/JSONObject.java:95-101`.
4. Inside `getLong`, `child.asLong()` on a non-numeric text node returns `0L` instead of
throwing, so the method silently coerces `"abc"` to `0L`, diverging from fastjson's
`getLong` which would raise a cast error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONObject.java
**Line:** 100:100
**Comment:**
*Logic Error: Numeric conversion currently uses Jackson's permissive `asLong()`, which silently returns `0` for non-numeric text (for example `"abc"`). That masks invalid input and can make bad payloads look like legitimate zero values. Parse textual values explicitly and throw `JSONException` on invalid casts to preserve fastjson-compatible behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doGet(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The GET test has the same false-positive issue: error JSON bodies are non-null and can still return HTTP 200 in this servlet stack, so the assertion does not prove the happy path worked. Validate that the response contains expected block content. [logic error]
Severity Level: Major ⚠️
- ⚠️ GET block retrieval API regressions may bypass detection.
- ⚠️ Errors in GET handling may go unnoticed in testing.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| assertEquals(true, response.getContentAsString().contains("blockID")); |
Steps of Reproduction ✅
1. `GetBlockServlet.doGet()`
(`framework/src/main/java/org/tron/core/services/http/GetBlockServlet.java:25-27`)
delegates to `handle()`, which wraps the whole flow in a try/catch and calls
`Util.processError(e, response)` on any `Exception` (`lines 33-40`).
2. `Util.processError()`
(`framework/src/main/java/org/tron/core/services/http/Util.java:17-24`) writes an
`"Error"` JSON string to the response body without changing the HTTP status code, so the
response remains 200 with a non-null body for failures.
3. In `GetBlockServletTest.testGetBlockGet()`
(`framework/src/test/java/org/tron/core/services/http/GetBlockServletTest.java:56-65`), if
the mocked `wallet.getBlock(any())` configured in `setUp()` (`lines 27-35`) is changed to
throw (for example, `thenThrow(new RuntimeException("fail"))`), `servlet.doGet(request,
response)` will still return HTTP 200 with a non-null error JSON body produced by
`processError`.
4. The existing assertion at line 66 (`assertNotNull(response.getContentAsString());`)
therefore still passes in this error scenario, so the GET test does not verify that a
valid block JSON (which should include `"blockID"` via `Util.printBlock()` at
`Util.java:105-114`) was returned rather than a generic error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetBlockServletTest.java
**Line:** 66:66
**Comment:**
*Logic Error: The GET test has the same false-positive issue: error JSON bodies are non-null and can still return HTTP 200 in this servlet stack, so the assertion does not prove the happy path worked. Validate that the response contains expected block content.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
Owner
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Sequence DiagramThis PR routes all HTTP and JSON-RPC JSON handling through new Jackson-based JSON, JSONObject, and JSONArray wrappers while keeping the existing wallet and contract flows intact. sequenceDiagram
participant Client
participant APIEndpoint
participant JsonWrapper
participant NodeCore
Client->>APIEndpoint: Send JSON request
APIEndpoint->>JsonWrapper: Parse body to JSONObject or JSONArray
JsonWrapper-->>APIEndpoint: Parsed data object
APIEndpoint->>NodeCore: Execute wallet or contract operation
NodeCore-->>APIEndpoint: Domain result
APIEndpoint->>JsonWrapper: Serialize result to JSON text
JsonWrapper-->>APIEndpoint: JSON string
APIEndpoint-->>Client: JSON response
Generated by CodeAnt AI |
Owner
Author
|
@CodeAnt-AI: Review comments skipped (comments #3, #5, #7–#20): The getBoolean/getDouble silent-zero issue (#3, #5) could break existing callers that rely on the lenient |
|
|
||
| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); |
There was a problem hiding this comment.
Suggestion: Relying on HTTP status 200 is a false-success check here because this servlet's error path (Util.processError) writes an error body without setting a non-200 status. The test can pass even when doPost throws internally. Validate that the response is not an error payload instead. [logic error]
Severity Level: Major ⚠️
- ⚠️ Delegate resource servlet test misses error JSON responses.
- ⚠️ Regressions in transaction creation may pass unit tests.
Suggested change
| assertEquals(200, response.getStatus()); | |
| org.junit.Assert.assertFalse(response.getContentAsString().contains("\"Error\"")); |
Steps of Reproduction ✅
1. In `DelegateResourceServlet.doPost` at
`framework/src/main/java/org/tron/core/services/http/DelegateResourceServlet.java:22-36`,
observe that any exception in request handling is caught and delegated to
`Util.processError(e, response)`.
2. In `Util.processError` at
`framework/src/main/java/org/tron/core/services/http/Util.java:476-483`, see that it logs
the exception and writes `Util.printErrorMsg(e)` to the response body without changing the
HTTP status code from the default 200.
3. In `Util.printErrorMsg` at
`framework/src/main/java/org/tron/core/services/http/Util.java:89-93`, confirm that the
error payload is JSON containing an `"Error"` field (e.g., `{"Error":
"java.lang.RuntimeException : ... "}`).
4. In the test `testDelegateResource` at
`framework/src/test/java/org/tron/core/services/http/DelegateResourceServletTest.java:44-60`,
note that after calling `servlet.doPost(request, response)`, the test only asserts
`assertEquals(200, response.getStatus());` on line 59 and
`assertNotNull(response.getContentAsString());` on line 60. If a realistic regression
causes `wallet.createTransactionCapsule(...)` (line 28-30 in
`DelegateResourceServlet.java`) or JSON parsing (`JSON.parseObject` at line 31) to throw,
`doPost` will catch the exception, `Util.processError` will write an `"Error"` JSON body
with status 200, and this test will still pass because it does not check that the response
content is a non-error payload (e.g., by asserting that it does not contain `"Error"`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/DelegateResourceServletTest.java
**Line:** 59:59
**Comment:**
*Logic Error: Relying on HTTP status `200` is a false-success check here because this servlet's error path (`Util.processError`) writes an error body without setting a non-200 status. The test can pass even when `doPost` throws internally. Validate that the response is not an error payload instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The POST test only checks for HTTP 200 and non-null body, but Util.processError writes an error payload without changing status, so this test can pass even when request parsing fails. Assert that the response is not an error payload and verify the wallet call happened to ensure the success path was executed. [logic error]
Severity Level: Major ⚠️
- ⚠️ /wallet/getaccount POST errors not detected by tests.
- ⚠️ Wallet account retrieval regressions may go unnoticed.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| org.junit.Assert.assertFalse(response.getContentAsString().contains("\"Error\"")); | |
| org.mockito.Mockito.verify(wallet).getAccount(any(Account.class)); |
Steps of Reproduction ✅
1. Hit the POST wallet endpoint `/wallet/getaccount` defined in
`protocol/src/main/protos/api/api.proto:24-31` so that the request is handled by
`GetAccountServlet.doPost(HttpServletRequest, HttpServletResponse)` in
`framework/src/main/java/org/tron/core/services/http/GetAccountServlet.java:34-42`.
2. Provide a request body that causes parsing to fail, e.g., malformed JSON or otherwise
invalid input, so that either `PostParams.getPostParams(request)` in
`framework/src/main/java/org/tron/core/services/http/PostParams.java:24-31` or
`JsonFormat.merge(params.getParams(), build, params.isVisible())` in
`GetAccountServlet.java:38-39` throws an exception.
3. The exception is caught by the `catch (Exception e)` block in `doPost()` at
`GetAccountServlet.java:40-42`, which calls `Util.processError(e, response)` implemented
in `framework/src/main/java/org/tron/core/services/http/Util.java:217-224`; this logs the
error and writes an error JSON body with an `"Error"` field via `Util.printErrorMsg(e)` at
`Util.java:89-93`, but never changes the HTTP status code from 200.
4. In the unit test `testGetAccountPost()` in
`framework/src/test/java/org/tron/core/services/http/GetAccountServletTest.java:42-54`,
`servlet.doPost(request, response)` is invoked and the test only asserts
`assertEquals(200, response.getStatus());` and
`assertNotNull(response.getContentAsString());` (line 53), so the test still passes even
though the servlet executed the error path and `wallet.getAccount(account)` in
`fillResponse()` (`GetAccountServlet.java:45-49`) was never called; adding assertions that
the body does not contain `"Error"` and that `wallet.getAccount(any(Account.class))` was
invoked would detect this failure.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetAccountServletTest.java
**Line:** 53:53
**Comment:**
*Logic Error: The POST test only checks for HTTP 200 and non-null body, but `Util.processError` writes an error payload without changing status, so this test can pass even when request parsing fails. Assert that the response is not an error payload and verify the wallet call happened to ensure the success path was executed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doGet(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The GET test has the same false-positive issue: a non-null body does not prove success because error handling also produces a non-null JSON response with status left at 200. Check that no error payload is returned and verify the wallet interaction. [logic error]
Severity Level: Major ⚠️
- ⚠️ /wallet/getaccount GET errors not detected by tests.
- ⚠️ Account query regressions may slip past CI.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| org.junit.Assert.assertFalse(response.getContentAsString().contains("\"Error\"")); | |
| org.mockito.Mockito.verify(wallet).getAccount(any(Account.class)); |
Steps of Reproduction ✅
1. Hit the GET wallet endpoint `/wallet/getaccount` defined in
`protocol/src/main/protos/api/api.proto:26-31`, which is served by
`GetAccountServlet.doGet(HttpServletRequest, HttpServletResponse)` in
`framework/src/main/java/org/tron/core/services/http/GetAccountServlet.java:20-32`.
2. Cause any runtime exception inside the `doGet()` try-block, for example by having
`wallet.getAccount(account)` in `fillResponse(boolean, Account, HttpServletResponse)`
(`GetAccountServlet.java:45-49`) throw (e.g., due to a downstream failure) so that the
call stack from `doGet()` (line 28) into `fillResponse()` results in an exception.
3. The exception is caught by the `catch (Exception e)` block in `doGet()` at
`GetAccountServlet.java:29-31`, which invokes `Util.processError(e, response)` in
`framework/src/main/java/org/tron/core/services/http/Util.java:217-224`; `processError`
logs and writes an error JSON body containing an `"Error"` field via
`Util.printErrorMsg(e)` (`Util.java:89-93`) but does not modify the HTTP status, leaving
it at 200.
4. In the unit test `testGetAccountGet()` in
`framework/src/test/java/org/tron/core/services/http/GetAccountServletTest.java:56-65`,
the servlet is invoked via `servlet.doGet(request, response)` (line 63) and the test only
asserts `assertEquals(200, response.getStatus());` and
`assertNotNull(response.getContentAsString());` (line 65), so the test passes even when
the servlet returns an error payload; adding checks that the response body does not
contain `"Error"` and that `wallet.getAccount(any(Account.class))` was called would ensure
the happy-path is actually exercised.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetAccountServletTest.java
**Line:** 65:65
**Comment:**
*Logic Error: The GET test has the same false-positive issue: a non-null body does not prove success because error handling also produces a non-null JSON response with status left at 200. Check that no error payload is returned and verify the wallet interaction.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doGet(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| } |
There was a problem hiding this comment.
Suggestion: Checking only HTTP status in GET is unreliable here because the servlet writes errors to the body without setting a non-200 status, so failures can still pass. Assert the response payload to ensure the success path was actually executed. [logic error]
Severity Level: Major ⚠️
- ⚠️ GET servlet errors return 200, body-only error JSON.
- ⚠️ GET unit test cannot distinguish error from success.
- ⚠️ Affects GetAssetIssueByNameOnSolidity and OnPBFT HTTP APIs.
Suggested change
| } | |
| assertEquals("{}", response.getContentAsString().trim()); |
Steps of Reproduction ✅
1. Execute `GetAssetIssueByNameServletTest.testGet` in
`framework/src/test/java/org/tron/core/services/http/GetAssetIssueByNameServletTest.java:56-65`,
which sets `value` to `"test"` and then calls `servlet.doGet(request, response)` at line
63.
2. As shown in `GetAssetIssueByNameServlet.doGet`
(`framework/src/main/java/org/tron/core/services/http/GetAssetIssueByNameServlet.java:23-33`),
the method computes `visible` via `Util.getVisible(request)` (Util.java:68-73, returns
`false` when `visible` parameter is absent), then calls `ByteArray.fromHexString(input)`
with the raw `"test"` value at line 30.
3. `ByteArray.fromHexString` in
`common/src/main/java/org/tron/common/utils/ByteArray.java:39-56` attempts to decode the
string as hex via `Hex.decode`, which throws on non-hex characters; this exception is
caught by the `catch (Exception e)` block in `doGet`, which delegates to
`Util.processError(e, response)` at
`framework/src/main/java/org/tron/core/services/http/Util.java:89-96`, writing an error
JSON body like `{"Error": ...}` to the response.
4. Neither `doGet` nor `Util.processError` modify the HTTP status code (the only
status-related logic is in `RateLimiterServlet.service` at
`framework/src/main/java/org/tron/core/services/http/RateLimiterServlet.java:20-47`, which
just delegates to `super.service` and does not change status on such exceptions), so
`response.getStatus()` remains 200 while the body contains error JSON; the existing
assertion at line 64 (`assertEquals(200, response.getStatus());`) passes even though the
normal success body `"{}"` from `fillResponse` (`GetAssetIssueByNameServlet.java:51-59`)
was never produced.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetAssetIssueByNameServletTest.java
**Line:** 65:65
**Comment:**
*Logic Error: Checking only HTTP status in GET is unreliable here because the servlet writes errors to the body without setting a non-200 status, so failures can still pass. Assert the response payload to ensure the success path was actually executed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The assertNotNull check is ineffective here because servlet response content is typically an empty string rather than null when nothing is written, so this test can pass even if the endpoint returns no payload. Assert the expected body for the mocked null proposal path to actually validate behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Null-proposal POST handling not validated for correct JSON body.
- ⚠️ Regression in null response format may pass test suite.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| assertEquals("{}", response.getContentAsString().trim()); |
Steps of Reproduction ✅
1. In
`framework/src/main/java/org/tron/core/services/http/GetProposalByIdServlet.java:45-52`,
note that `fillResponse(...)` writes `"{}"` when `wallet.getProposalById(proposalId)`
returns `null` and otherwise writes the serialized proposal.
2. In
`framework/src/test/java/org/tron/core/services/http/GetProposalByIdServletTest.java:27-35`,
the `setUp()` method injects a mocked `Wallet` into `GetProposalByIdServlet` and stubs
`wallet.getProposalById(any())` to return `null`, so the null-proposal branch is the one
under test.
3. In `testPost()` at `GetProposalByIdServletTest.java:42-53`, the test sends a JSON POST
body `{"id": 1}` and then only asserts `assertEquals(200, response.getStatus())` and
`assertNotNull(response.getContentAsString())`, without checking the actual body content.
4. Because `MockHttpServletResponse#getContentAsString()` returns an empty string `""`
(non-null) even when nothing is written, any future regression in `fillResponse(...)` that
omits writing `"{}"` for the null-proposal case (or writes an incorrect payload) would
still allow `testPost()` to pass, as the assertion only checks non-nullity rather than the
expected `"{}"` JSON body.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetProposalByIdServletTest.java
**Line:** 53:53
**Comment:**
*Logic Error: The `assertNotNull` check is ineffective here because servlet response content is typically an empty string rather than `null` when nothing is written, so this test can pass even if the endpoint returns no payload. Assert the expected body for the mocked `null` proposal path to actually validate behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+60
to
+61
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: This test can pass even when doPost fails, because the servlet's error path writes an "Error" body without changing HTTP status, and assertNotNull(response.getContentAsString()) is too weak to detect failures. Assert on successful response content instead (and ensure no error payload) so JSON parsing or transaction-building regressions are actually caught. [logic error]
Severity Level: Major ⚠️
- ⚠️ MarketSellAssetServlet failures not detected by unit test.
- ⚠️ JSON migration regressions in market-sell API less visible.
Suggested change
| assertEquals(200, response.getStatus()); | |
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| org.junit.Assert.assertTrue(content.contains("\"raw_data\"")); | |
| org.junit.Assert.assertFalse(content.contains("\"Error\"")); |
Steps of Reproduction ✅
1. In `MarketSellAssetServlet.doPost()`
(`framework/src/main/java/org/tron/core/services/http/MarketSellAssetServlet.java:26-47`),
note that any exception in request handling (e.g., thrown by
`Util.checkBodySize(contract)` at `Util.java:70-74` or `JsonFormat.merge(...)` at
`MarketSellAssetServlet.java:34`) is caught and delegated to `Util.processError(e,
response)` at `MarketSellAssetServlet.java:45-46`.
2. `Util.processError`
(`framework/src/main/java/org/tron/core/services/http/Util.java:217-224`) logs the
exception and writes `Util.printErrorMsg(e)` to the response body; `printErrorMsg`
(`Util.java:89-93`) returns JSON of the form `{"Error": "<exception class> : <message>"}`
and does not modify the HTTP status code (which remains 200 by default).
3. In the success path, `MarketSellAssetServlet.doPost()` calls
`Util.printCreateTransaction(tx, visible)` and writes its result
(`MarketSellAssetServlet.java:36-44`); `printCreateTransaction` (`Util.java:154-158`)
wraps `printTransactionToJSON`, which always includes a `"raw_data"` field
(`Util.java:215-259`) plus `"txID"`.
4. The unit test `testMarketSellAsset`
(`framework/src/test/java/org/tron/core/services/http/MarketSellAssetServletTest.java:45-61`)
currently only asserts `assertEquals(200, response.getStatus());` and
`assertNotNull(response.getContentAsString());` (lines 60-61). If a regression in JSON
handling or transaction creation causes any exception in `doPost` for the given JSON body,
the servlet will return a 200 response with an `{"Error": ...}` body, and this test will
still pass because the status is 200 and the body is non-null, failing to distinguish
success (body containing `"raw_data"`) from error (body containing `"Error"`). The
suggested improved assertions (`content.contains("\"raw_data\"")` and
`!content.contains("\"Error\"")`) would cause such regressions to fail the test.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/MarketSellAssetServletTest.java
**Line:** 60:61
**Comment:**
*Logic Error: This test can pass even when `doPost` fails, because the servlet's error path writes an `"Error"` body without changing HTTP status, and `assertNotNull(response.getContentAsString())` is too weak to detect failures. Assert on successful response content instead (and ensure no error payload) so JSON parsing or transaction-building regressions are actually caught.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The final assertion is effectively a no-op because the response body getter can return an empty string while still being non-null, so the test can pass even when no payload is produced. Assert that the response body is non-empty to actually validate behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Asset participation servlet test may miss empty response regressions.
- ⚠️ CI green even if endpoint stops returning body content.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| assertEquals(false, response.getContentAsString().isEmpty()); |
Steps of Reproduction ✅
1. Run the JUnit test method `testParticipateAssetIssue()` at lines 44–61 in
`framework/src/test/java/org/tron/core/services/http/ParticipateAssetIssueServletTest.java`;
it builds a POST request and executes `servlet.doPost(request, response)` at line 58
against `ParticipateAssetIssueServlet` (implementation at
`framework/src/main/java/org/tron/core/services/http/ParticipateAssetIssueServlet.java:27–44`).
2. `MockHttpServletResponse` from Spring
(`org.springframework.mock.web.MockHttpServletResponse`, instantiated at line 57) is used
to capture the servlet output; by design its `getContentAsString()` returns a non-null
string, returning an empty string when no content is written.
3. Even if `ParticipateAssetIssueServlet.doPost()` fails to write any body content to the
response writer (for example, a future regression removing or short-circuiting the
`response.getWriter().println(...)` call at lines 38–40 in the servlet),
`response.getContentAsString()` in the test still returns a non-null (possibly empty)
string.
4. The assertion `assertNotNull(response.getContentAsString())` at line 60 therefore
always passes whenever the test reaches this line, meaning the test cannot fail due to
missing or empty response bodies and may allow regressions in the servlet's JSON output to
go undetected.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/ParticipateAssetIssueServletTest.java
**Line:** 60:60
**Comment:**
*Logic Error: The final assertion is effectively a no-op because the response body getter can return an empty string while still being non-null, so the test can pass even when no payload is produced. Assert that the response body is non-empty to actually validate behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The current assertions can pass even when request processing fails: response.getStatus() stays 200 by default in MockHttpServletResponse, and getContentAsString() is non-null even for error payloads. This makes the test produce false positives. Assert that the response actually contains a successful transaction body and does not contain an error payload. [logic error]
Severity Level: Major ⚠️
- ⚠️ /wallet/transferasset regressions not caught by unit test.
- ⚠️ Error JSON treated as success in TransferAssetServletTest.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| assertEquals(false, content.contains("Error")); | |
| assertEquals(true, content.contains("raw_data")); |
Steps of Reproduction ✅
1. Observe the HTTP implementation in
`framework/src/main/java/org/tron/core/services/http/TransferAssetServlet.java:27-44`,
where `doPost` wraps the whole request handling in a `try`/`catch` and on any `Exception`
calls `Util.processError(e, response)` without ever changing the HTTP status code.
2. Inspect `framework/src/main/java/org/tron/core/services/http/Util.java:89-93` and
`:18-25`, where `printErrorMsg` wraps the error in a JSON object under the `"Error"` key
and `processError` simply writes this JSON to the response body (no call to
`response.setStatus(...)`), meaning `MockHttpServletResponse` will keep its default status
`200` even for error responses.
3. Look at the test in
`framework/src/test/java/org/tron/core/services/http/TransferAssetServletTest.java:44-60`,
where `testTransferAsset` builds a valid JSON body, invokes `servlet.doPost(request,
response)`, and asserts only `assertEquals(200, response.getStatus());` and
`assertNotNull(response.getContentAsString());` at lines 59–60, never checking whether the
body contains `"Error"` or a valid `"raw_data"` section.
4. Force an error path in `doPost` (for example, by temporarily changing the wallet mock
in `TransferAssetServletTest.setUp()` at lines 28–36 to
`when(wallet.createTransactionCapsule(any(), any())).thenThrow(new
RuntimeException("boom"));`), re-running the test will still pass because the response
status remains `200` and the body contains a non-null error JSON with `"Error": ...`,
which satisfies the current assertions at lines 59–60; thus regressions in
`/wallet/transferasset` can slip through unnoticed.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/TransferAssetServletTest.java
**Line:** 60:60
**Comment:**
*Logic Error: The current assertions can pass even when request processing fails: `response.getStatus()` stays 200 by default in `MockHttpServletResponse`, and `getContentAsString()` is non-null even for error payloads. This makes the test produce false positives. Assert that the response actually contains a successful transaction body and does not contain an error payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| String content = response.getContentAsString(); | ||
| assertTrue(content.contains("raw_data") || content.contains("Error")); |
There was a problem hiding this comment.
Suggestion: The assertion currently allows an error payload ("Error") as a passing condition, so the test can succeed even when TransferServlet#doPost fails to create a transaction. Tighten the assertion to require a successful transaction response so JSON parsing or request-handling regressions are actually caught. [logic error]
Severity Level: Major ⚠️
- ⚠️ Transfer creation endpoint regressions not caught by unit test.
- ⚠️ Error responses treated as success in TransferServletTest.
Suggested change
| assertTrue(content.contains("raw_data") || content.contains("Error")); | |
| assertTrue(content.contains("raw_data")); |
Steps of Reproduction ✅
1. Note that `/wallet/createtransaction` is mapped to `TransferServlet` in
`FullNodeHttpApiService.addServlet(...)` at
`framework/src/main/java/org/tron/core/services/http/FullNodeHttpApiService.java:45-47`,
where `transferServlet` is registered for that path.
2. In `TransferServlet#doPost`
(`framework/src/main/java/org/tron/core/services/http/TransferServlet.java:8-25`), the
request body is read and `Util.checkBodySize(contract)` is called at line 12; if the body
exceeds `Args.getInstance().getMaxMessageSize()` (`Util.java:10-13`), this throws an
`Exception` which is caught by the `catch (Exception e)` block at lines 22-23.
3. When that exception is caught, `Util.processError(e, response)` at
`framework/src/main/java/org/tron/core/services/http/Util.java:37-44` writes a JSON error
payload built by `printErrorMsg(e)` at `Util.java:10-13`, which contains an `"Error"`
field but no `"raw_data"` field, and it does not change the HTTP status (remains 200).
4. The unit test `testTransfer` in
`framework/src/test/java/org/tron/core/services/http/TransferServletTest.java:5-22` posts
a JSON transfer body and then executes `assertTrue(content.contains("raw_data") ||
content.contains("Error"));` at line 60; thus, if a regression or configuration issue
makes `doPost` consistently go through the error path described in steps 2–3, the test
still passes because it treats an `"Error"` response body as success, so JSON parsing or
request-handling failures for `/wallet/createtransaction` are not detected by this test.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/TransferServletTest.java
**Line:** 60:60
**Comment:**
*Logic Error: The assertion currently allows an error payload (`"Error"`) as a passing condition, so the test can succeed even when `TransferServlet#doPost` fails to create a transaction. Tighten the assertion to require a successful transaction response so JSON parsing or request-handling regressions are actually caught.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Owner
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Sequence DiagramThis PR replaces Fastjson with Jackson-backed JSON wrappers for both HTTP and JSON-RPC APIs, ensuring compatible request/response formats while tightening invalid input handling. The diagram shows how API requests now flow through the shared Jackson-based JSON layer before reaching core node logic. sequenceDiagram
participant Client
participant API
participant JsonWrapper
participant Node
Client->>API: Send JSON request
API->>JsonWrapper: Parse JSON into object model
JsonWrapper-->>API: Return parsed data
API->>Node: Execute operation using parsed parameters
Node-->>API: Return operation result
API->>JsonWrapper: Serialize result to JSON text
JsonWrapper-->>API: Return JSON string
API-->>Client: Send JSON response
Generated by CodeAnt AI |
| return null; | ||
| } | ||
| try { | ||
| return new JSONObject((ObjectNode) MAPPER.readTree(text)); |
There was a problem hiding this comment.
Suggestion: parseObject(String) casts the parsed root directly to ObjectNode, so valid JSON like "null" (or any non-object root) triggers a ClassCastException path instead of handling null cleanly. Parse into JsonNode first, return null for JSON null, and explicitly reject non-object roots with a clear JSONException. [type error]
Severity Level: Major ⚠️
- ⚠️ `JSON.parseObject(String)` rejects valid `"null"` JSON bodies.
- ⚠️ Callers expecting nullable objects may see unexpected `JSONException`.
Suggested change
| return new JSONObject((ObjectNode) MAPPER.readTree(text)); | |
| JsonNode parsed = MAPPER.readTree(text); | |
| if (parsed == null || parsed.isNull()) { | |
| return null; | |
| } | |
| if (!parsed.isObject()) { | |
| throw new JSONException("Failed to parse JSON object: root is not an object"); | |
| } | |
| return new JSONObject((ObjectNode) parsed); |
Steps of Reproduction ✅
1. Open `common/src/main/java/org/tron/json/JSON.java` and locate `public static
JSONObject parseObject(String text)` at lines 53–62.
2. Invoke `org.tron.json.JSON.parseObject("null")` from a unit test or any caller; this
uses the method at line 53.
3. At line 58, `MAPPER.readTree("null")` returns a Jackson `NullNode`, which is then cast
to `ObjectNode` in `return new JSONObject((ObjectNode) MAPPER.readTree(text));`.
4. The cast to `ObjectNode` throws a `ClassCastException` that is caught by the `catch
(Exception e)` at lines 59–61 and rethrown as a `JSONException`, so valid JSON `"null"` is
treated as an error instead of being handled as `null` or as a non-object value.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSON.java
**Line:** 58:58
**Comment:**
*Type Error: `parseObject(String)` casts the parsed root directly to `ObjectNode`, so valid JSON like `"null"` (or any non-object root) triggers a `ClassCastException` path instead of handling `null` cleanly. Parse into `JsonNode` first, return `null` for JSON null, and explicitly reject non-object roots with a clear `JSONException`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+27
to
+30
| if (text == null || text.isEmpty()) { | ||
| return new JSONArray(); | ||
| } | ||
| try { |
There was a problem hiding this comment.
Suggestion: parseArray does not treat whitespace-only input as empty, so readTree can return null and create a JSONArray with a null backing node; later calls like size() then throw NullPointerException. Normalize blank input and guard against a null parsed tree before constructing the wrapper. [null pointer]
Severity Level: Major ⚠️
- ❌ Whitespace-only JSON arrays crash when accessing size or elements.
- ⚠️ JSON.parseArray/JSONObject.parseArray unsafe on blank client payloads.
- ⚠️ Behavior inconsistent with JSON.parseObject whitespace handling in JSON.java.
Suggested change
| if (text == null || text.isEmpty()) { | |
| return new JSONArray(); | |
| } | |
| try { | |
| if (text == null || text.trim().isEmpty()) { | |
| return new JSONArray(); | |
| } | |
| try { | |
| com.fasterxml.jackson.databind.JsonNode parsed = JSON.MAPPER.readTree(text); | |
| if (parsed == null) { | |
| return new JSONArray(); | |
| } | |
| return new JSONArray((ArrayNode) parsed); |
Steps of Reproduction ✅
1. Invoke the static factory `JSONArray.parseArray(String text)` defined in
`common/src/main/java/org/tron/json/JSONArray.java:26` with a whitespace-only string (e.g.
`" "`), either directly or via the wrappers `JSON.parseArray(text)`
(`common/src/main/java/org/tron/json/JSON.java:104-105`) or `JSONObject.parseArray(text)`
(`common/src/main/java/org/tron/json/JSONObject.java:40-42`).
2. At `JSONArray.java:27-29`, the condition `if (text == null || text.isEmpty())`
evaluates to false for whitespace-only input because `isEmpty()` does not trim, so
execution falls through to the `try` block at `JSONArray.java:30-31`.
3. In the `try` block at `JSONArray.java:31`, `JSON.MAPPER.readTree(text)` is called; for
whitespace-only content Jackson may return `null` rather than an `ArrayNode`, so `new
JSONArray((ArrayNode) JSON.MAPPER.readTree(text))` effectively invokes the
`JSONArray(ArrayNode node)` constructor at `JSONArray.java:17-19` with `node == null`,
creating an instance whose internal `node` field is null.
4. When caller code later uses this instance, e.g. by calling `size()` at
`JSONArray.java:37-39` or `isEmpty()` at `JSONArray.java:41-42`, these methods
unconditionally dereference `node`, leading to a `NullPointerException` instead of a
controlled `JSONException`, which contradicts the stated goal in
`common/src/main/java/org/tron/json/JSON.java:84-98` and the fuzz test
`testInvalidInputNeverCausesNPE` in
`framework/src/test/java/org/tron/json/JsonCompatibilityFuzzTest.java:97-110` that invalid
JSON should not cause NPEs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONArray.java
**Line:** 27:30
**Comment:**
*Null Pointer: `parseArray` does not treat whitespace-only input as empty, so `readTree` can return `null` and create a `JSONArray` with a null backing node; later calls like `size()` then throw `NullPointerException`. Normalize blank input and guard against a null parsed tree before constructing the wrapper.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| if (child == null || child.isNull()) { | ||
| return 0L; | ||
| } | ||
| return child.asLong(); |
There was a problem hiding this comment.
Suggestion: getLongValue currently uses Jackson's lenient conversion, which silently turns non-numeric text (for example "abc") into 0 instead of failing. That can make invalid numeric input look valid and propagate wrong values through request handling. Parse textual values explicitly and throw JSONException on invalid content. [logic error]
Severity Level: Major ⚠️
- ⚠️ JSONObject helper may treat invalid long fields as zero.
- ⚠️ PBFT HTTP `/walletpbft/getnowblock` test uses `getLongValue`.
Suggested change
| return child.asLong(); | |
| if (child.isNumber()) { | |
| return child.longValue(); | |
| } | |
| if (child.isTextual()) { | |
| try { | |
| return Long.parseLong(child.asText()); | |
| } catch (NumberFormatException e) { | |
| throw new JSONException("Cannot cast '" + child.asText() + "' to long", e); | |
| } | |
| } | |
| throw new JSONException("Cannot cast " + child.getNodeType() + " to long"); |
Steps of Reproduction ✅
1. Add a small test method (for example in
`framework/src/test/java/org/tron/json/JsonCompatibilityFuzzTest.java`) that calls
`org.tron.json.JSONObject.parseObject` with `String badJson = "{\"number\":\"abc\"}";` so
that `JSONObject.parseObject(badJson)` constructs a `JSONObject` backed by a Jackson
`ObjectNode` (factory defined in
`common/src/main/java/org/tron/json/JSONObject.java:34-38`).
2. In that test, invoke `long value = json.getLongValue("number");` on the returned
`JSONObject`, which executes `getLongValue` implemented at
`common/src/main/java/org/tron/json/JSONObject.java:129-135`.
3. Inside `getLongValue`, `node.get("number")` returns a Jackson `TextNode` containing the
string `"abc"`, and because the method currently just calls `child.asLong()` at line 134,
no explicit parsing or validation is performed.
4. Jackson's `JsonNode.asLong()` returns `0L` for non-numeric text, so the method returns
`0L` instead of failing, making the invalid `"abc"` input indistinguishable from a valid
numeric zero for any caller using `getLongValue` on user-supplied JSON.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONObject.java
**Line:** 134:134
**Comment:**
*Logic Error: `getLongValue` currently uses Jackson's lenient conversion, which silently turns non-numeric text (for example `"abc"`) into `0` instead of failing. That can make invalid numeric input look valid and propagate wrong values through request handling. Parse textual values explicitly and throw `JSONException` on invalid content.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| if (child == null || child.isNull()) { | ||
| return 0; | ||
| } | ||
| return child.asInt(); |
There was a problem hiding this comment.
Suggestion: getIntValue has the same silent-coercion issue as getLongValue: invalid numeric text is converted to 0 by asInt(). This hides malformed input and can cause incorrect downstream behavior. Use explicit numeric/text parsing and throw on invalid input. [logic error]
Severity Level: Major ⚠️
- ⚠️ JSONObject helper may treat invalid int fields as zero.
- ⚠️ Shielded-note HTTP tests read `index` via `getIntValue`.
Suggested change
| return child.asInt(); | |
| if (child.isNumber()) { | |
| return child.intValue(); | |
| } | |
| if (child.isTextual()) { | |
| try { | |
| return Integer.parseInt(child.asText()); | |
| } catch (NumberFormatException e) { | |
| throw new JSONException("Cannot cast '" + child.asText() + "' to int", e); | |
| } | |
| } | |
| throw new JSONException("Cannot cast " + child.getNodeType() + " to int"); |
Steps of Reproduction ✅
1. Add a small test (for example in
`framework/src/test/java/org/tron/json/JsonCompatibilityFuzzTest.java`) that calls
`org.tron.json.JSONObject.parseObject` with `String badJson = "{\"index\":\"xyz\"}";` so
that `JSONObject.parseObject(badJson)` yields a `JSONObject` instance (factory defined in
`common/src/main/java/org/tron/json/JSONObject.java:34-38`).
2. In that test, invoke `int idx = json.getIntValue("index");` which routes to
`getIntValue` implemented at `common/src/main/java/org/tron/json/JSONObject.java:137-143`.
3. Inside `getIntValue`, `node.get("index")` returns a Jackson `TextNode` containing
`"xyz"`, and the method immediately calls `child.asInt()` at line 142 without checking
whether the text is a valid integer.
4. Jackson's `JsonNode.asInt()` returns `0` for non-numeric text, so `getIntValue` returns
`0` instead of signaling an error, allowing malformed `"index"` values in JSON responses
(for example those parsed in
`framework/src/test/java/org/tron/common/utils/client/utils/HttpMethed.java:3603,3655,3707,3757,3825,3894,3960,4011,4062`)
to be silently treated as zero if those fields ever become non-numeric strings.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONObject.java
**Line:** 142:142
**Comment:**
*Logic Error: `getIntValue` has the same silent-coercion issue as `getLongValue`: invalid numeric text is converted to `0` by `asInt()`. This hides malformed input and can cause incorrect downstream behavior. Use explicit numeric/text parsing and throw on invalid input.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The final assertion is ineffective because getContentAsString() is non-null even for empty or error responses, and this servlet writes errors without changing HTTP 200. As written, the test can pass even when request handling fails. Assert for a successful transaction payload (for example containing raw_data) so the test actually detects regressions. [logic error]
Severity Level: Major ⚠️
- ⚠️ AccountPermissionUpdateServlet regressions may pass tests undetected.
- ⚠️ Account-permission update HTTP API coverage remains weak.
- ⚠️ Error JSON responses still satisfy current success assertions.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| org.junit.Assert.assertTrue("Servlet returned error payload: " + content, | |
| content.contains("\"raw_data\"")); |
Steps of Reproduction ✅
1. In
`framework/src/main/java/org/tron/core/services/http/AccountPermissionUpdateServlet.java:26-40`,
note that `doPost` wraps all logic in a `try`/`catch` and, on any exception, calls
`Util.processError(e, response)` without changing the HTTP status.
2. In `framework/src/main/java/org/tron/core/services/http/Util.java:197-204`,
`processError` logs the exception and writes a JSON body `{"Error": "...message..."}` to
the response via `response.getWriter().println(Util.printErrorMsg(e))`, but never calls
`response.setStatus(...)`, so the status remains the default 200.
3. In the test
`framework/src/test/java/org/tron/core/services/http/AccountPermissionUpdateServletTest.java:44-61`,
`testAccountPermissionUpdate` invokes `servlet.doPost(request, response)` and then only
asserts `assertEquals(200, response.getStatus());` and
`assertNotNull(response.getContentAsString());` (line 60), which passes for any non-null
body, including the error JSON written by `processError`.
4. Because `MockHttpServletResponse.getContentAsString()` returns a non-null string when
anything is written (including the error JSON from `Util.processError`), any regression
inside `doPost` that throws (e.g., a failure in `PostParams.getPostParams`,
`JsonFormat.merge` or `Util.printCreateTransaction`) will still produce HTTP 200 and a
non-null body, and the current test will pass instead of detecting the failure; asserting
that the body contains `"raw_data"` (the success payload from
`Util.printCreateTransaction` at `Util.java:95-99,156-203`) would fail in such error cases
and better guard this endpoint.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/AccountPermissionUpdateServletTest.java
**Line:** 60:60
**Comment:**
*Logic Error: The final assertion is ineffective because `getContentAsString()` is non-null even for empty or error responses, and this servlet writes errors without changing HTTP 200. As written, the test can pass even when request handling fails. Assert for a successful transaction payload (for example containing `raw_data`) so the test actually detects regressions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doGet(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| } |
There was a problem hiding this comment.
Suggestion: The GET test only verifies HTTP status and does not verify that a response body was produced, so it can pass even when serialization/output regresses. Add a body assertion to ensure the endpoint actually returns content. [logic error]
Severity Level: Major ⚠️
- ⚠️ GET handler could return empty body without test failure.
- ⚠️ Missing verification of JSON output for delegated resource GET.
Suggested change
| } | |
| assertEquals(false, response.getContentAsString().isEmpty()); |
Steps of Reproduction ✅
1. Run the JUnit test `testGet()` in
`framework/src/test/java/org/tron/core/services/http/GetDelegatedResourceAccountIndexV2ServletTest.java`
(lines 57–66), which creates a `MockHttpServletRequest` with a `value` parameter and
invokes `servlet.doGet(request, response)`.
2. Observe that the test asserts only `assertEquals(200, response.getStatus())` at line 65
and does not inspect `response.getContentAsString()` at all.
3. If `GetDelegatedResourceAccountIndexV2Servlet#doGet` were modified or regressed so that
it sets status 200 but writes no body (leaving `MockHttpServletResponse` with its default
empty-string content), the test would still pass because the only assertion is on the
status code.
4. As a result, the current test cannot catch regressions where the delegated-resource GET
endpoint stops returning a JSON payload while still reporting success, weakening
protection of the servlet's response behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetDelegatedResourceAccountIndexV2ServletTest.java
**Line:** 66:66
**Comment:**
*Logic Error: The GET test only verifies HTTP status and does not verify that a response body was produced, so it can pass even when serialization/output regresses. Add a body assertion to ensure the endpoint actually returns content.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| Field f = GetExchangeByIdServlet.class.getDeclaredField("wallet"); | ||
| f.setAccessible(true); | ||
| f.set(servlet, wallet); | ||
| when(wallet.getExchangeById(any())).thenReturn(null); |
There was a problem hiding this comment.
Suggestion: The mock currently returns null, which drives the servlet into its exception path (Util.processError) instead of the normal success path. That makes this test pass even when serialization logic is broken. Return a real Exchange instance so the test validates successful behavior. [logic error]
Severity Level: Major ⚠️
- ❌ Unit test never executes successful Exchange serialization path.
- ⚠️ Regressions in /wallet/getexchangebyid JSON go undetected.
Suggested change
| when(wallet.getExchangeById(any())).thenReturn(null); | |
| when(wallet.getExchangeById(any())).thenReturn(Exchange.getDefaultInstance()); |
Steps of Reproduction ✅
1. Run `GetExchangeByIdServletTest.testPost` in
`framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java:42-54`;
in `setUp()` (line 27-35) the field `wallet` of `GetExchangeByIdServlet` is set via
reflection and stubbed with `when(wallet.getExchangeById(any())).thenReturn(null);` at
line 34.
2. `testPost()` calls `servlet.doPost(request, response);` at line 51, which executes
`GetExchangeByIdServlet.doPost()` in
`framework/src/main/java/org/tron/core/services/http/GetExchangeByIdServlet.java:22-31`,
parsing the JSON body and then calling `fillResponse(params.isVisible(), id, response);`
at line 27.
3. `fillResponse()` at `GetExchangeByIdServlet.java:43-47` invokes
`wallet.getExchangeById(ByteString.copyFrom(ByteArray.fromLong(id)))` with the mocked
`wallet`, which returns `null`, and immediately passes that `null` to
`JsonFormat.printToString(...)` at line 45–46, causing a `NullPointerException` inside
`JsonFormat.print(...)` (`JsonFormat.java:117-120` dereferences `message.getAllFields()`).
4. The exception is caught by the `catch (Exception e)` in `doPost()` at
`GetExchangeByIdServlet.java:28-30`, which calls `Util.processError(e, response)`
(`Util.java:217-224`); this writes an error JSON but does not change the HTTP status
(still 200). `testPost()` then asserts only `response.getStatus() == 200` and non-null
content (lines 52-53), so the test passes while exercising only the error path, never the
normal serialization path.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java
**Line:** 34:34
**Comment:**
*Logic Error: The mock currently returns `null`, which drives the servlet into its exception path (`Util.processError`) instead of the normal success path. That makes this test pass even when serialization logic is broken. Return a real `Exchange` instance so the test validates successful behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: assertNotNull(response.getContentAsString()) is too weak because both success and error responses are non-null, so the test can pass on failures. Assert the exact expected payload to prevent false positives. [logic error]
Severity Level: Major ⚠️
- ❌ POST /wallet/getexchangebyid errors still pass unit test.
- ⚠️ JSON handling regressions undetected in exchange-by-id POST.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| assertEquals("{}", response.getContentAsString().trim()); |
Steps of Reproduction ✅
1. In `GetExchangeByIdServletTest.setUp()`
(`framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java:27-35`),
the `wallet` field is injected and `wallet.getExchangeById(any())` is stubbed to return
`null` (line 34), guaranteeing an exception during JSON printing.
2. `testPost()` (lines 42-54) builds a POST request with body `{"id": 1}` and calls
`servlet.doPost(request, response);` at line 51, hitting `GetExchangeByIdServlet.doPost()`
(`framework/src/main/java/org/tron/core/services/http/GetExchangeByIdServlet.java:22-31`).
3. As in suggestion 1, `fillResponse()` (`GetExchangeByIdServlet.java:43-47`) passes the
mocked `null` exchange into `JsonFormat.printToString(...)`, which throws
`NullPointerException`; `doPost()` catches this and calls `Util.processError(e, response)`
(`Util.java:217-224`), which writes an error JSON string like `{"Error":"..."}` to the
response body without changing the HTTP 200 status.
4. Back in `testPost()`, the only body assertion is
`assertNotNull(response.getContentAsString());` at line 53, which succeeds for both the
normal success JSON and the error JSON written by `Util.processError`, so the test cannot
distinguish success from failure and will still pass even if every call to `fillResponse`
throws and the servlet always returns an error payload.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java
**Line:** 53:53
**Comment:**
*Logic Error: `assertNotNull(response.getContentAsString())` is too weak because both success and error responses are non-null, so the test can pass on failures. Assert the exact expected payload to prevent false positives.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| servlet.doGet(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| } |
There was a problem hiding this comment.
Suggestion: The GET test only checks HTTP status, but this servlet keeps status 200 even when exceptions occur, so the test can still pass on errors. Add a response-body assertion to verify the handler actually returned a valid exchange payload. [logic error]
Severity Level: Major ⚠️
- ❌ GET /wallet/getexchangebyid error responses pass unit test.
- ⚠️ Exchange-by-id GET JSON regressions remain undetected.
Suggested change
| } | |
| assertEquals("{}", response.getContentAsString().trim()); |
Steps of Reproduction ✅
1. In `GetExchangeByIdServletTest.setUp()`
(`framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java:27-35`),
`GetExchangeByIdServlet` is constructed with its `wallet` field replaced by a Mockito
mock, and `wallet.getExchangeById(any())` is stubbed to return `null` at line 34.
2. `testGet()` (lines 56-65) constructs a GET request with query parameter `id=1` and
invokes `servlet.doGet(request, response);` at line 63, which calls
`GetExchangeByIdServlet.doGet()`
(`framework/src/main/java/org/tron/core/services/http/GetExchangeByIdServlet.java:33-40`).
3. `doGet()` computes `visible` and parses the `"id"` parameter, then calls
`fillResponse(visible, Long.parseLong(input), response);` at
`GetExchangeByIdServlet.java:37`; inside `fillResponse()` (lines 43-47), the mocked
`wallet` returns `null` and `JsonFormat.printToString(null, visible)` throws
`NullPointerException`, which is caught by `doGet()`'s `catch (Exception e)` block (lines
38-40) and handled via `Util.processError(e, response)` (`Util.java:217-224`) writing an
error JSON body but not changing the HTTP status.
4. Despite the error path being taken, `testGet()` only asserts `assertEquals(200,
response.getStatus());` at line 64 and never inspects `response.getContentAsString()`, so
the test passes even though the servlet returned an error payload instead of a valid
exchange JSON, meaning GET behavior can be broken without the test detecting it.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java
**Line:** 65:65
**Comment:**
*Logic Error: The GET test only checks HTTP status, but this servlet keeps status 200 even when exceptions occur, so the test can still pass on errors. Add a response-body assertion to verify the handler actually returned a valid exchange payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+59
to
+60
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: This test can pass even when doPost fails, because the servlet's error path writes an "Error" JSON body without changing HTTP status, and assertNotNull(response.getContentAsString()) still succeeds for error responses. Validate that the response body is not an error payload so the test actually catches Jackson parsing/regression failures. [logic error]
Severity Level: Major ⚠️
- ⚠️ Jackson parsing failures for /wallet/participateassetissue may go unnoticed.
- ⚠️ Test suite cannot distinguish success from Util.processError JSON.
Suggested change
| assertEquals(200, response.getStatus()); | |
| assertNotNull(response.getContentAsString()); | |
| String responseBody = response.getContentAsString(); | |
| assertEquals(200, response.getStatus()); | |
| assertNotNull(responseBody); | |
| assertEquals(false, responseBody.contains("\"Error\"")); |
Steps of Reproduction ✅
1. Open `ParticipateAssetIssueServlet.doPost` in
`framework/src/main/java/org/tron/core/services/http/ParticipateAssetIssueServlet.java:27-43`.
Note that it parses the request JSON via `JsonFormat.merge(contract, build, visible)`
(line 34), then on any exception executes the `catch (Exception e)` block (lines 41-42)
which calls `Util.processError(e, response)`.
2. Open `Util.processError` in
`framework/src/main/java/org/tron/core/services/http/Util.java:217-224`. It logs the
exception and writes `Util.printErrorMsg(e)` to the response body (line 220), where
`Util.printErrorMsg` builds a JSON object with an `"Error"` field (`Util.java:89-93`), but
it never calls `response.setStatus(...)` or `sendError(...)`, so the HTTP status remains
200.
3. Open the test `testParticipateAssetIssue` in
`framework/src/test/java/org/tron/core/services/http/ParticipateAssetIssueServletTest.java:44-61`.
It sends a JSON body to `servlet.doPost(request, response)` (lines 46-58) and asserts only
`assertEquals(200, response.getStatus())` and
`assertNotNull(response.getContentAsString())` (lines 59-60), without checking for the
`"Error"` field in the body.
4. To observe the faulty behavior, make the request JSON invalid so that
`JsonFormat.merge` throws (for example by removing required fields) and run
`testParticipateAssetIssue`. The request will hit `JsonFormat.merge` in
`ParticipateAssetIssueServlet.java:34`, throw a `ParseException`, flow into
`Util.processError` (`Util.java:217-224`), which writes an `{"Error": ...}` JSON body but
leaves status as 200. The test at `ParticipateAssetIssueServletTest.java:59-60` still
passes because status is 200 and the error JSON body is non-null, demonstrating that it
cannot detect `doPost` failures and will yield false-positive passes for Jackson/JSON
parsing regressions.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/ParticipateAssetIssueServletTest.java
**Line:** 59:60
**Comment:**
*Logic Error: This test can pass even when `doPost` fails, because the servlet's error path writes an `"Error"` JSON body without changing HTTP status, and `assertNotNull(response.getContentAsString())` still succeeds for error responses. Validate that the response body is not an error payload so the test actually catches Jackson parsing/regression failures.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Owner
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Sequence DiagramThis PR replaces Fastjson with a shared Jackson-based JSON wrapper used across HTTP servlets, JSON-RPC services, and VM trace serialization, tightening value parsing while keeping response shapes stable. sequenceDiagram
participant Client
participant HttpAPI
participant JSONWrapper
participant WalletCore
participant TraceSerializer
Client->>HttpAPI: Submit API or JSON RPC request (JSON body)
HttpAPI->>JSONWrapper: Parse JSON text to JSONObject or JSONArray
JSONWrapper-->>HttpAPI: Parsed JSON with strict value handling
HttpAPI->>WalletCore: Build and execute transaction or query
WalletCore->>TraceSerializer: Serialize VM trace fields to JSON
TraceSerializer-->>WalletCore: JSON trace string
WalletCore-->>HttpAPI: Result and optional trace data
HttpAPI->>JSONWrapper: Serialize result to JSON string
JSONWrapper-->>Client: JSON response body
Generated by CodeAnt AI |
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
halibobo1205
force-pushed
the
feat/jackjson2
branch
from
0c0358f to
1850552
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
User description
Replace all usages of fastjson with Jackson
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
CodeAnt-AI Description
Switch API JSON handling to Jackson while keeping existing requests and responses working
What Changed
Impact
✅ Safer API JSON parsing✅ Fewer silent bad-value reads✅ Continued support for existing wallet and contract requests💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.