refactor(framework): replace fastjson with jackson-databind#114
Open
halibobo1205 wants to merge 1 commit intodevelopfrom
Open
refactor(framework): replace fastjson with jackson-databind#114halibobo1205 wants to merge 1 commit intodevelopfrom
halibobo1205 wants to merge 1 commit intodevelopfrom
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 · |
codeant-ai
bot
added
the
size:XXL
Sequence DiagramThis PR replaces Fastjson with Jackson-based JSON utilities and thin wrapper types, so HTTP API servlets now parse request bodies and serialize responses through a shared, consistently configured Jackson ObjectMapper. sequenceDiagram
participant Client
participant HttpServlet
participant JSONWrapper
participant JacksonMapper
participant Wallet
Client->>HttpServlet: POST JSON request
HttpServlet->>JSONWrapper: parseObject or parseArray
JSONWrapper->>JacksonMapper: Read JSON into tree or type
JacksonMapper-->>JSONWrapper: Parsed JSONObject or JSONArray
HttpServlet->>Wallet: Build and create transaction from parsed data
Wallet-->>HttpServlet: Transaction result
HttpServlet->>JSONWrapper: toJSONString(result)
JSONWrapper->>JacksonMapper: Serialize object to JSON string
HttpServlet-->>Client: JSON response body
Generated by CodeAnt AI |
Nitpicks 🔍
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3e835eb5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+103
to
+104
| } catch (Exception e) { | ||
| return null; |
There was a problem hiding this comment.
JSON.parse now catches all parsing failures and returns null, but Util.isValidJson still treats success as “no exception” (framework/src/main/java/org/tron/core/services/http/Util.java). This makes malformed payloads (including application/x-www-form-urlencoded bodies like a=1&b=2) look like valid JSON, so Util.getJsonString skips URL-decoding and downstream request parsing fails. Previously, Fastjson surfaced parse errors via exceptions, which this validation path depends on.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+94
to
+104
| * Parses any valid JSON value. Returns {@code null} on failure (mirrors Fastjson behaviour). | ||
| * Mirrors {@code JSON.parse(String)}. | ||
| */ | ||
| public static Object parse(String text) { | ||
| if (text == null || text.isEmpty()) { | ||
| return null; | ||
| } | ||
| try { | ||
| return MAPPER.readTree(text); | ||
| } catch (Exception e) { | ||
| return null; |
There was a problem hiding this comment.
Suggestion: The parse(String) method swallows all parsing exceptions and returns null on failure, which breaks existing callers like Util.isValidJson that rely on an exception being thrown to detect invalid JSON; this will cause invalid JSON strings to be treated as valid and skip the intended URL-encoded fallback path. [logic error]
Severity Level: Critical 🚨
- ❌ Form-encoded POST /wallet/marketcancelorder requests fail to parse.
- ❌ Other wallet HTTP POSTs with form bodies break parsing.
- ⚠️ UtilMockTest.testGetJsonString unit test fails with bug.
Suggested change
| * Parses any valid JSON value. Returns {@code null} on failure (mirrors Fastjson behaviour). | |
| * Mirrors {@code JSON.parse(String)}. | |
| */ | |
| public static Object parse(String text) { | |
| if (text == null || text.isEmpty()) { | |
| return null; | |
| } | |
| try { | |
| return MAPPER.readTree(text); | |
| } catch (Exception e) { | |
| return null; | |
| * Parses any valid JSON value. Throws {@link JSONException} on invalid input | |
| * so callers can detect malformed JSON (e.g. via try/catch), mirroring | |
| * Fastjson's observable behaviour in this codebase. | |
| * Mirrors {@code JSON.parse(String)}. | |
| */ | |
| public static Object parse(String text) { | |
| if (text == null || text.isEmpty()) { | |
| return null; | |
| } | |
| try { | |
| return MAPPER.readTree(text); | |
| } catch (Exception e) { | |
| throw new JSONException("Failed to parse JSON: " + e.getMessage(), e); |
Steps of Reproduction ✅
1. Start a full node with HTTP API enabled so `FullNodeHttpApiService` is active (see
`framework/src/main/java/org/tron/core/services/http/FullNodeHttpApiService.java:37-41`).
2. Send an HTTP POST request to `/wallet/marketcancelorder` with header `Content-Type:
application/x-www-form-urlencoded` (endpoint wiring at
`FullNodeHttpApiService.java:58-60`) and a typical form body, e.g.
`owner_address=owner_address&contract_address=contract_address`.
3. The request is handled by `MarketCancelOrderServlet.doPost(...)`
(`framework/src/main/java/org/tron/core/services/http/MarketCancelOrderServlet.java:6-26`),
which calls `PostParams.getPostParams(request)` (`PostParams.java:24-32`). Because the
content type is `APPLICATION_FORM_URLENCODED`, `PostParams.getPostParams` calls
`Util.getJsonString(input)` (`PostParams.java:24-29`).
4. Inside `Util.getJsonString`
(`framework/src/main/java/org/tron/core/services/http/Util.java:614-633`),
`isValidJson(str)` (`Util.java:636-643`) calls `JSON.parse(str)`
(`common/src/main/java/org/tron/json/JSON.java:97-106`). With the current implementation,
`MAPPER.readTree(str)` throws for the URL-encoded string, `JSON.parse` catches the
exception and returns `null` instead of rethrowing, so no exception propagates.
`isValidJson` returns `true`, `getJsonString` returns the original URL-encoded string
instead of converting it to JSON, and `MarketCancelOrderServlet.doPost` then passes this
non-JSON string to `JsonFormat.merge(contract, build, visible)`
(`MarketCancelOrderServlet.java:8-12`), which fails to parse and results in an error
response instead of the previously working behaviour where URL-encoded bodies are
converted to JSON and processed successfully.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSON.java
**Line:** 94:104
**Comment:**
*Logic Error: The `parse(String)` method swallows all parsing exceptions and returns null on failure, which breaks existing callers like `Util.isValidJson` that rely on an exception being thrown to detect invalid JSON; this will cause invalid JSON strings to be treated as valid and skip the intended URL-encoded fallback path.
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 test currently only asserts HTTP 200 and a non-null response body, but since the servlet wraps all exceptions with Util.processError without changing the status code, the test will still pass even if doPost fails; you should additionally assert that the response content does not contain the error marker so that the test actually fails on servlet errors. [logic error]
Severity Level: Major ⚠️
- ⚠️ DelegateResourceServlet test passes despite internal servlet failures.
- ⚠️ CI may not catch regressions in delegate-resource endpoint.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| assertNotNull(content); | |
| assertEquals(-1, content.indexOf("\"Error\"")); |
Steps of Reproduction ✅
1. Observe the servlet implementation in
`framework/src/main/java/org/tron/core/services/http/DelegateResourceServlet.java:22-36`.
Inside `doPost(...)`, any `Exception` from parsing JSON, building the contract, or calling
`wallet.createTransactionCapsule(...)` is caught and handled by `Util.processError(e,
response)` at line 35.
2. Inspect `Util.processError` in
`framework/src/main/java/org/tron/core/services/http/Util.java:476-483`. It logs the
exception and writes `Util.printErrorMsg(e)` to the response writer, which produces a JSON
object with an `"Error"` field (`printErrorMsg` at lines 89-93), but it does not change
the HTTP status, leaving it at 200.
3. Look at the unit test `testDelegateResource` in
`framework/src/test/java/org/tron/core/services/http/DelegateResourceServletTest.java:45-61`.
The test builds a `MockHttpServletRequest` and `MockHttpServletResponse`, calls
`servlet.doPost(request, response)`, then only asserts `response.getStatus() == 200` and
that `response.getContentAsString()` is non-null (lines 57-60).
4. Simulate an application error in the servlet path by configuring the mocked `wallet` in
`DelegateResourceServletTest.setUp` (`DelegateResourceServletTest.java:29-37`) to throw a
`RuntimeException` from `wallet.createTransactionCapsule(any(), any())`. When running
`testDelegateResource`, `doPost` catches this exception, `Util.processError` writes a body
containing `"Error"` with status still 200, and the current test assertions (status==200
and non-null body) still pass, demonstrating that the test does not fail on servlet
errors.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:** 60:60
**Comment:**
*Logic Error: The test currently only asserts HTTP 200 and a non-null response body, but since the servlet wraps all exceptions with `Util.processError` without changing the status code, the test will still pass even if `doPost` fails; you should additionally assert that the response content does not contain the error marker so that the test actually fails on servlet errors.
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 test currently only asserts HTTP 200 and non-null response body, so it will still pass when DeployContractServlet hits an exception and Util.processError returns an error JSON with HTTP 200, meaning the test does not actually verify successful contract deployment and won't catch regressions in the JSON/ABI handling. [logic error]
Severity Level: Major ⚠️
- ⚠️ DeployContractServletTest can pass on error JSON responses.
- ⚠️ Deploy-contract JSON/ABI regressions may ship undetected.
- ⚠️ Error responses share HTTP 200 with successful deploys.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| assertNotNull(content); | |
| assertEquals(-1, content.indexOf("Error")); |
Steps of Reproduction ✅
1. Inspect the implementation of `DeployContractServlet.doPost` in
`framework/src/main/java/org/tron/core/services/http/DeployContractServlet.java:34-92`.
The method wraps all processing in a `try` block and, on any `Exception`, executes the
`catch` at lines 90-92, which calls `Util.processError(e, response)`.
2. Inspect `Util.processError` in
`framework/src/main/java/org/tron/core/services/http/Util.java:476-483`. This method logs
the exception and writes `Util.printErrorMsg(e)` to the response via
`response.getWriter().println(...)` but does not modify the HTTP status code.
`Util.printErrorMsg` at `Util.java:89-92` builds a JSON object with an `"Error"` field.
3. In a test similar to `testDeployContract` in
`framework/src/test/java/org/tron/core/services/http/DeployContractServletTest.java:45-66`,
construct a `MockHttpServletRequest` with a malformed `"abi"` value (e.g., invalid JSON
array) so that `JsonFormat.merge(abiSB.toString(), abiBuilder, params.isVisible())` at
`DeployContractServlet.java:54` throws a `ParseException`. Call `servlet.doPost(request,
response)` with this request.
4. Observe that the exception is caught and `Util.processError` writes an error JSON body
containing an `"Error"` key to the response, while the status remains the default 200. The
current `testDeployContract` assertions at `DeployContractServletTest.java:62-65` only
check `response.getStatus() == 200` and that `response.getContentAsString()` is non-null,
so the test will still pass even though the servlet produced an error JSON instead of a
successful deployment payload.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/DeployContractServletTest.java
**Line:** 65:65
**Comment:**
*Logic Error: The test currently only asserts HTTP 200 and non-null response body, so it will still pass when `DeployContractServlet` hits an exception and `Util.processError` returns an error JSON with HTTP 200, meaning the test does not actually verify successful contract deployment and won't catch regressions in the JSON/ABI handling.
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 assertion only checks that the response body String is non-null, which MockHttpServletResponse#getContentAsString() guarantees even when no content is written, so the test can pass while the servlet returns an empty body, failing to validate the actual behavior after the fastjson→Jackson change. [logic error]
Severity Level: Major ⚠️
- ❌ /wallet/exchangetransaction empty body regression not caught.
- ⚠️ Exchange transaction clients may receive 200 with empty body.
- ⚠️ Test ExchangeTransactionServletTest gives false confidence about payload.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| assertNotNull(content); | |
| // Ensure the servlet actually produced a non-empty JSON payload or an error message | |
| org.junit.Assert.assertFalse(content.isEmpty()); |
Steps of Reproduction ✅
1. Observe that `/wallet/exchangetransaction` is wired to `ExchangeTransactionServlet` in
`framework/src/main/java/org/tron/core/services/http/FullNodeHttpApiService.java:381`, so
this servlet backs a real HTTP endpoint.
2. In
`framework/src/main/java/org/tron/core/services/http/ExchangeTransactionServlet.java:22-35`,
`doPost` builds an `ExchangeTransactionContract`, creates a `Transaction` via
`wallet.createTransactionCapsule(...)`, and writes JSON to the HTTP response using
`response.getWriter().println(Util.printCreateTransaction(tx, params.isVisible()));`. This
is the only place in `doPost` that writes the response body.
3. The unit test `testExchangeTransaction` in
`framework/src/test/java/org/tron/core/services/http/ExchangeTransactionServletTest.java:44-61`
constructs a `MockHttpServletRequest` with JSON payload, calls `servlet.doPost(request,
response)`, then asserts `assertEquals(200, response.getStatus());` and
`assertNotNull(response.getContentAsString());` (lines 60–61).
4. According to Spring's `MockHttpServletResponse` behavior, `getContentAsString()`
returns an empty string (`""`) rather than `null` when no content is written. If a
regression (for example, a future change around `Util.printCreateTransaction(...)` or the
`println(...)` call at `ExchangeTransactionServlet.java:32`) causes the servlet to stop
writing any body while still returning HTTP 200, `response.getContentAsString()` will be
`""`, the `assertNotNull(...)` at `ExchangeTransactionServletTest.java:61` will still
pass, and the test will not detect that `/wallet/exchangetransaction` is returning an
empty body.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/ExchangeTransactionServletTest.java
**Line:** 61:61
**Comment:**
*Logic Error: The assertion only checks that the response body String is non-null, which `MockHttpServletResponse#getContentAsString()` guarantees even when no content is written, so the test can pass while the servlet returns an empty body, failing to validate the actual behavior after the fastjson→Jackson change.
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.| MockHttpServletRequest request = new MockHttpServletRequest(); | ||
| request.setMethod("GET"); | ||
| request.addParameter("value", "test"); | ||
|
|
There was a problem hiding this comment.
Suggestion: The GET test sends a plain-text asset name ("test") without the visible flag, but GetAssetIssueListByNameServlet treats value as a hex string when visible is false, causing ByteArray.fromHexString to receive an invalid hex value and exercise the error path instead of the normal success flow; adding visible=true makes the test align with the servlet's contract and avoids passing invalid hex to the decoder. [logic error]
Severity Level: Major ⚠️
- ⚠️ HTTP servlet GET test exercises error path, not success.
- ⚠️ Normal GET flow untested; regressions may go undetected.
Suggested change
| request.addParameter("visible", "true"); |
Steps of Reproduction ✅
1. Run the unit test `testGet()` in
`framework/src/test/java/org/tron/core/services/http/GetAssetIssueListByNameServletTest.java:57-65`,
which builds a GET request with `value="test"` and **no** `visible` parameter (lines
58-60).
2. Inside `GetAssetIssueListByNameServlet.doGet()` at
`framework/src/main/java/org/tron/core/services/http/GetAssetIssueListByNameServlet.java:24-28`,
`Util.getVisible(request)` returns `false` because the `visible` query parameter is absent
(`Util.getVisible()` at `Util.java:77-82`), so `fillResponse(false, "test", response)` is
invoked.
3. `fillResponse()` at `GetAssetIssueListByNameServlet.java:45-51` skips the
`Util.getHexString(value)` conversion because `visible` is `false` and calls
`wallet.getAssetIssueListByName(ByteString.copyFrom(ByteArray.fromHexString(value)))` with
`value="test"`, leading to `ByteArray.fromHexString("test")` at
`common/src/main/java/org/tron/common/utils/ByteArray.java:46-56`, which delegates to
`Hex.decode(data)` and encounters non-hex characters (`'t'`, `'s'`), triggering a decode
exception.
4. The exception from `ByteArray.fromHexString` is caught by the `try/catch` in `doGet()`
(`GetAssetIssueListByNameServlet.java:24-31`), which calls `Util.processError(e,
response)` (`Util.java:217-223`) and writes an error JSON body while leaving the HTTP
status at 200; thus the test's `assertEquals(200, response.getStatus())` at
`GetAssetIssueListByNameServletTest.java:64` passes even though the normal success flow
(valid name → `wallet.getAssetIssueListByName` without decode error) is never exercised,
confirming the test is currently bound to the error path caused by treating a plain-text
name as hex.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetAssetIssueListByNameServletTest.java
**Line:** 61:61
**Comment:**
*Logic Error: The GET test sends a plain-text asset name (`"test"`) without the `visible` flag, but `GetAssetIssueListByNameServlet` treats `value` as a hex string when `visible` is false, causing `ByteArray.fromHexString` to receive an invalid hex value and exercise the error path instead of the normal success flow; adding `visible=true` makes the test align with the servlet's contract and avoids passing invalid hex to the decoder.
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 test setup stubs the wallet to return null for getExchangeById, which causes JsonFormat.printToString in the servlet to receive a null message and execute the error-handling path; this means the tests never exercise the normal successful serialization path and will continue to pass even if the servlet fails to handle valid exchanges correctly. Returning a minimal Exchange instance from the mock ensures the tests validate the expected behavior for a found exchange and avoids implicitly relying on how the code handles a null proto message. [logic error]
Severity Level: Major ⚠️
- ⚠️ /wallet/getexchangebyid servlet success path remains untested.
- ⚠️ JsonFormat.printToString(Exchange) regressions not caught by servlet tests.
- ⚠️ Util.processError error JSON passes as apparent success.
Suggested change
| when(wallet.getExchangeById(any())).thenReturn(null); | |
| Exchange exchange = Exchange.newBuilder() | |
| .setExchangeId(1L) | |
| .build(); | |
| when(wallet.getExchangeById(any())).thenReturn(exchange); |
Steps of Reproduction ✅
1. Observe the HTTP endpoint registration in
`framework/src/main/java/org/tron/core/services/http/FullNodeHttpApiService.java:383`,
where `/wallet/getexchangebyid` is mapped to `GetExchangeByIdServlet`.
2. In
`framework/src/main/java/org/tron/core/services/http/GetExchangeByIdServlet.java:24-27`,
`fillResponse` calls `JsonFormat.printToString(wallet.getExchangeById(...), visible)` with
no null check on the `wallet.getExchangeById` result.
3. In `framework/src/main/java/org/tron/core/services/http/JsonFormat.java:161-165`,
`printToString(Message message, boolean selfType)` dereferences `message` (via
`print(message, text, selfType)` and then `message.getAllFields()`), so calling it with a
null `message` will throw a `NullPointerException`.
4. The top-level handlers `doPost` and `doGet` in `GetExchangeByIdServlet` catch any
exception and delegate to `Util.processError(e, response)` (`Util.java:7-14`), which
writes an error JSON body but does not change the HTTP status code from its default 200.
5. In the new unit test
`framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java:27-35`,
the `setUp` method stubs `wallet.getExchangeById(any())` to always return `null`, ensuring
that both `testPost` and `testGet` always hit the exception path described above.
6. Running `testPost` (`GetExchangeByIdServletTest.java:42-54`) triggers `doPost`, which
eventually calls `JsonFormat.printToString` with `null`, causing a `NullPointerException`
that is caught and converted to an error JSON via `Util.processError`; the test only
asserts `response.getStatus() == 200` and `response.getContentAsString()` is non-null,
which are both true for this error response.
7. Running `testGet` (`GetExchangeByIdServletTest.java:56-65`) similarly triggers `doGet`,
causing the same `NullPointerException` and error JSON path, and the test again only
asserts `response.getStatus() == 200`, which also passes for the error response.
8. Because the tests always drive the `GetExchangeByIdServlet` through the error-handling
path (null `Exchange` leading to `Util.processError`) instead of a normal successful
`Exchange` serialization, any future regression in the normal path (e.g.,
`JsonFormat.printToString(Exchange, visible)` producing malformed output) would not be
caught by these tests.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 test setup stubs the wallet to return null for `getExchangeById`, which causes `JsonFormat.printToString` in the servlet to receive a null message and execute the error-handling path; this means the tests never exercise the normal successful serialization path and will continue to pass even if the servlet fails to handle valid exchanges correctly. Returning a minimal `Exchange` instance from the mock ensures the tests validate the expected behavior for a found exchange and avoids implicitly relying on how the code handles a null proto message.
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 assertion in the POST test only checks that the response content string is non-null, but MockHttpServletResponse.getContentAsString() never returns null, so the test would still pass even if the servlet produced an empty or incorrect body; asserting the exact expected JSON ensures the JSON parsing and response generation are actually verified. [logic error]
Severity Level: Major ⚠️
- ⚠️ Weak test for `/wallet/getmarketorderbyaccount` JSON response.
- ⚠️ Regressions in empty-result JSON may go undetected.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| assertEquals("{}", response.getContentAsString().trim()); |
Steps of Reproduction ✅
1. In `GetMarketOrderByAccountServletTest.setUp`
(`framework/src/test/java/org/tron/core/services/http/GetMarketOrderByAccountServletTest.java:27-35`),
note that `wallet.getMarketOrderByAccount(any())` is stubbed to return `null`.
2. In `GetMarketOrderByAccountServlet.getResult`
(`framework/src/main/java/org/tron/core/services/http/GetMarketOrderByAccountServlet.java:24-37`),
when the wallet returns `null` the servlet writes `"{}"` to the HTTP response via
`response.getWriter().println("{}");`.
3. In `testPost` of the same test class (`GetMarketOrderByAccountServletTest.java:43-54`),
a POST request with JSON body is sent to `servlet.doPost(request, response)`, which calls
`getResult` and writes `"{}"`; the test then executes
`assertNotNull(response.getContentAsString());` at line 53.
4. Because `MockHttpServletResponse.getContentAsString()` (used in many tests, e.g.
`HttpServletTest` at
`framework/src/test/java/org/tron/core/services/http/HttpServletTest.java:144-536`)
returns a non-null `String` even when the body is empty, this assertion will succeed even
if a future change causes `GetMarketOrderByAccountServlet` to write an empty or incorrect
body for `/wallet/getmarketorderbyaccount` (endpoint wiring in
`FullNodeHttpApiService.addServlet` at
`framework/src/main/java/org/tron/core/services/http/FullNodeHttpApiService.java:477-478`),
so the test does not actually verify the JSON response content.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetMarketOrderByAccountServletTest.java
**Line:** 53:53
**Comment:**
*Logic Error: The assertion in the POST test only checks that the response content string is non-null, but `MockHttpServletResponse.getContentAsString()` never returns null, so the test would still pass even if the servlet produced an empty or incorrect body; asserting the exact expected JSON ensures the JSON parsing and response generation are actually verified.
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. |
Replace all usages of fastjson with Jackson - Upgrades jackson-databind from 2.18.3 to 2.18.6 to address CVE GHSA-72hv-8253-57qq - Remove fastjson
halibobo1205
force-pushed
the
feat/jackjson2
branch
from
c3e835e to
af2c452
Compare
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2c4520cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+70
to
+74
| JsonNode child = node.get(key); | ||
| if (child == null || child.isNull()) { | ||
| return null; | ||
| } | ||
| return child.asInt(); |
There was a problem hiding this comment.
getInteger now uses Jackson's asInt(), which silently returns 0 for invalid text like "abc" instead of failing. That changes request validation behavior in production code paths such as Util.setTransactionPermissionId(...): malformed permission_id values are now treated as 0 and the transaction proceeds with default permission handling, rather than surfacing a client error. This is a behavioral regression from fastjson's strict casting and can cause users to sign/broadcast with unintended permission scope.
Useful? React with 👍 / 👎.
Comment on lines
+57
to
+58
| public String getString(String key) { | ||
| return node.path(key).asText(null); |
There was a problem hiding this comment.
getString currently calls path(key).asText(null), but for object/array nodes Jackson returns an empty string instead of the JSON text. Fastjson getString returns value.toString(), so callers that read nested JSON blobs as strings (for follow-up parsing) will now receive "" and fail downstream. This breaks drop-in compatibility for mixed scalar/structured fields.
Useful? React with 👍 / 👎.
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 helpers for HTTP APIs and tracing. The diagram shows how requests and responses now flow through shared org.tron.json wrappers built on a configured Jackson ObjectMapper. sequenceDiagram
participant Client
participant HttpApi
participant JsonWrapper
participant CoreNode
Client->>HttpApi: POST API request with JSON body
HttpApi->>JsonWrapper: Parse JSON into request fields
JsonWrapper-->>HttpApi: Parsed values and typed objects
HttpApi->>CoreNode: Build and submit transaction or query
CoreNode-->>HttpApi: Transaction or query result
HttpApi->>JsonWrapper: Convert result object to JSON string
JsonWrapper-->>Client: HTTP 200 with JSON response
CoreNode->>JsonWrapper: Serialize program trace to JSON
JsonWrapper-->>CoreNode: Pretty printed trace string
Generated by CodeAnt AI |
| if (child.isLong()) { | ||
| return child.asLong(); | ||
| } | ||
| if (child.isDouble() || child.isFloat()) { |
There was a problem hiding this comment.
Suggestion: The generic getter does not handle BigDecimal/BigInteger nodes, so floating-point numbers (parsed as BigDecimal due to USE_BIG_DECIMAL_FOR_FLOATS) and large integers fall through and are returned as strings, which deviates from Fastjson behavior and can cause type mismatches or ClassCastException in existing code expecting numeric types. [logic error]
Severity Level: Critical 🚨
- ❌ Generic JSONArray.get() returns String for BigDecimal values.
- ❌ Migrated Fastjson code casting to Number will fail.
- ⚠️ Risk of silent numeric handling bugs in new callers.
Suggested change
| if (child.isDouble() || child.isFloat()) { | |
| if (child.isBigInteger()) { | |
| return child.bigIntegerValue(); | |
| } | |
| if (child.isBigDecimal()) { | |
| return child.decimalValue(); | |
| } |
Steps of Reproduction ✅
1. Note that the shared Jackson mapper is configured with
`DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS` at
`common/src/main/java/org/tron/json/JSON.java:26-37`. This means floating-point literals
in JSON are parsed into `BigDecimal`-backed `DecimalNode` instances.
2. Parse a JSON array containing floating-point or very large integer values using the
provided API, e.g. `JSONArray array = JSONArray.parseArray("[1.23,
900719925474099312345]");` which uses the `parseArray` factory at
`common/src/main/java/org/tron/json/JSONArray.java:25-35` and delegates to
`JSON.MAPPER.readTree(text)` at line 31.
3. Call the generic getter `Object value = array.get(0);` or `array.get(1);` which
executes the `get(int index)` implementation at
`common/src/main/java/org/tron/json/JSONArray.java:45-72`. For `DecimalNode`/`BigInteger`
values, none of the `isInt()`, `isLong()`, `isDouble()`, `isFloat()`, or `isBoolean()`
branches match, so execution falls through to the final `return child.asText();`.
4. Observe that `value` is a `String` (e.g. `"1.23"`), not a numeric type. If client code
written against Fastjson semantics does `BigDecimal amount = (BigDecimal) array.get(0);`
or relies on `instanceof Number`, it will either throw `ClassCastException` at runtime or
silently treat numeric values as strings, causing downstream logic errors in calculations.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONArray.java
**Line:** 65:65
**Comment:**
*Logic Error: The generic getter does not handle BigDecimal/BigInteger nodes, so floating-point numbers (parsed as BigDecimal due to USE_BIG_DECIMAL_FOR_FLOATS) and large integers fall through and are returned as strings, which deviates from Fastjson behavior and can cause type mismatches or ClassCastException in existing code expecting numeric types.
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.| @Override | ||
| public Iterator<Object> iterator() { | ||
| List<Object> list = new ArrayList<>(); | ||
| node.forEach(list::add); |
There was a problem hiding this comment.
Suggestion: The iterator currently returns raw Jackson JsonNode elements instead of the converted Java values (Strings, numbers, JSONObject/JSONArray wrappers), which breaks the "drop-in replacement" contract and will cause runtime ClassCastException or logic errors in existing for-each loops that expect Fastjson-like element types. [logic error]
Severity Level: Major ⚠️
- ⚠️ Enhanced for-each over JSONArray yields JsonNode, not values.
- ⚠️ Migrated Fastjson loops can throw ClassCastException.
- ⚠️ Breaks advertised drop-in replacement semantics for iteration.
Suggested change
| node.forEach(list::add); | |
| for (int i = 0; i < node.size(); i++) { | |
| list.add(get(i)); | |
| } |
Steps of Reproduction ✅
1. Parse any JSON array using the drop-in API, e.g. call `JSONArray.parseArray("[1, 2,
\"foo\"]")` implemented at `common/src/main/java/org/tron/json/JSONArray.java:25-35`. This
is the same factory used via `JSON.parseArray()` in tests like `MerkleTreeTest` at
`framework/src/test/java/org/tron/core/zksnark/MerkleTreeTest.java:23-25`.
2. Iterate over the resulting `JSONArray` using the `Iterable<Object>` contract, e.g. in a
test or service method: `for (Object element : array) {
System.out.println(element.getClass()); }`. This enhanced-for loop uses
`JSONArray.iterator()` defined at
`common/src/main/java/org/tron/json/JSONArray.java:160-165`.
3. Observe that each `element` is actually a Jackson
`com.fasterxml.jackson.databind.JsonNode` (e.g. `IntNode`, `TextNode`), because
`iterator()` currently builds `List<Object> list = new ArrayList<>();
node.forEach(list::add);`, adding the raw `JsonNode` instances instead of using the
`get(int)` conversion logic at `JSONArray.java:45-71`.
4. If you write Fastjson-style code such as `for (Object element : array) { String s =
(String) element; }` or `JSONObject obj = (JSONObject) element;`, which is a normal usage
pattern for a `com.alibaba.fastjson.JSONArray`, a `ClassCastException` will be thrown at
runtime when casting a `JsonNode` to `String`/`JSONObject`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONArray.java
**Line:** 163:163
**Comment:**
*Logic Error: The iterator currently returns raw Jackson JsonNode elements instead of the converted Java values (Strings, numbers, JSONObject/JSONArray wrappers), which breaks the "drop-in replacement" contract and will cause runtime ClassCastException or logic errors in existing for-each loops that expect Fastjson-like element types.
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.| } | ||
|
|
||
| public Object remove(String key) { | ||
| return node.remove(key); |
There was a problem hiding this comment.
Suggestion: The remove method currently returns Jackson's internal JsonNode instead of the same Java value types that get exposes (strings, numbers, booleans, JSONObject/JSONArray), so any caller expecting Fastjson-like semantics may get an unexpected type and encounter runtime errors; it should mirror get by returning the converted value that was removed. [logic error]
Severity Level: Major ⚠️
- ⚠️ JSONObject.remove returns JsonNode, not mapped Java value.
- ⚠️ Future callers may see ClassCastException when casting String.
- ⚠️ Current servlet usage ignores return; no runtime error.
Suggested change
| return node.remove(key); | |
| Object existing = get(key); | |
| node.remove(key); | |
| return existing; |
Steps of Reproduction ✅
1. Locate `JSONObject.remove(String)` in
`common/src/main/java/org/tron/json/JSONObject.java:242-244`, which currently returns
`node.remove(key)` (a Jackson `JsonNode`) instead of using the same mapping logic as
`get(String)` above.
2. Confirm via tool search that this class is documented as a drop-in replacement for
`com.alibaba.fastjson.JSONObject` at `JSONObject.java:15-18`, where Fastjson's
`JSONObject.remove(Object)` returns the previous mapped value (e.g., `String`, `Integer`,
`Boolean`) consistent with `Map.remove`.
3. Observe an actual use-site of `remove` in production code at
`framework/src/main/java/org/tron/core/services/http/CreateShieldedTransactionWithoutSpendAuthSigServlet.java:35-40`,
where `jsonObject.remove("txID");` is called but the return value is discarded, so the
type difference is not visible today.
4. Consider a normal Fastjson-style usage on the same object, e.g. `String txId = (String)
jsonObject.remove("txID");` in
`CreateShieldedTransactionWithoutSpendAuthSigServlet.doPost` or similar callers: with the
current implementation this call would receive a Jackson `JsonNode` instead of a `String`,
and the cast would fail with `ClassCastException`, demonstrating that `remove` does not
honour the advertised Fastjson-compatible contract once its return value is used.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/tron/json/JSONObject.java
**Line:** 243:243
**Comment:**
*Logic Error: The `remove` method currently returns Jackson's internal `JsonNode` instead of the same Java value types that `get` exposes (strings, numbers, booleans, `JSONObject`/`JSONArray`), so any caller expecting Fastjson-like semantics may get an unexpected type and encounter runtime errors; it should mirror `get` by returning the converted value that was removed.
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
+35
to
+36
| when(wallet.createTransactionCapsule(any(), any())) | ||
| .thenReturn(new TransactionCapsule(Transaction.getDefaultInstance())); |
There was a problem hiding this comment.
Suggestion: The test stubs wallet.createTransactionCapsule with a TransactionCapsule built from Transaction.getDefaultInstance(), which lacks raw_data, causing Util.printCreateTransaction (via printTransactionToJSON) to dereference a missing "raw_data" field and throw a NullPointerException, so the servlet always goes through the error path in this test instead of exercising the normal success flow. Initializing the stubbed Transaction with an empty raw_data object avoids the NPE and more accurately reflects real usage. [logic error]
Severity Level: Major ⚠️
- ⚠️ ExchangeWithdrawServletTest always hits error path due to NPE.
- ⚠️ Successful JSON response path for exchange withdraw never exercised.
Suggested change
| when(wallet.createTransactionCapsule(any(), any())) | |
| .thenReturn(new TransactionCapsule(Transaction.getDefaultInstance())); | |
| Transaction transaction = Transaction.newBuilder() | |
| .setRawData(Transaction.raw.newBuilder().build()) | |
| .build(); | |
| when(wallet.createTransactionCapsule(any(), any())) | |
| .thenReturn(new TransactionCapsule(transaction)); |
Steps of Reproduction ✅
1. In `ExchangeWithdrawServletTest.setUp` at
`framework/src/test/java/org/tron/core/services/http/ExchangeWithdrawServletTest.java:29-37`,
the mock `wallet.createTransactionCapsule(any(), any())` is stubbed to return `new
TransactionCapsule(Transaction.getDefaultInstance())`, i.e., a `Transaction` with no
`raw_data` field set.
2. Run `testExchangeWithdraw` in the same file (`ExchangeWithdrawServletTest.java:45-61`),
which builds a POST `MockHttpServletRequest` with JSON body and calls
`servlet.doPost(request, response)` on a real `ExchangeWithdrawServlet` instance.
3. Inside `ExchangeWithdrawServlet.doPost` at
`framework/src/main/java/org/tron/core/services/http/ExchangeWithdrawServlet.java:22-33`,
the servlet calls the mocked `wallet.createTransactionCapsule(...)`, receives a
`TransactionCapsule` wrapping `Transaction.getDefaultInstance()`, then calls
`Util.printCreateTransaction(tx, params.isVisible())` to serialize the transaction to
JSON.
4. `Util.printCreateTransaction` at
`framework/src/main/java/org/tron/core/services/http/Util.java:154-158` delegates to
`printTransactionToJSON`. In `printTransactionToJSON` at `Util.java:215-259`,
`JsonFormat.printToString(transaction, selfType)` serializes the default `Transaction`
without a `raw_data` field, so `jsonTransaction.get("raw_data")` returns `null`; line
`257` (`JSONObject rawData =
JSONObject.parseObject(jsonTransaction.get("raw_data").toString());`) dereferences this
`null`, throwing a `NullPointerException` which is caught by the `ExchangeWithdrawServlet`
`catch (Exception e)` block at line `33`, causing `Util.processError(e, response)` to
write an error JSON body instead of the normal transaction JSON, while the test still
observes HTTP 200 and a non-null body.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/ExchangeWithdrawServletTest.java
**Line:** 35:36
**Comment:**
*Logic Error: The test stubs `wallet.createTransactionCapsule` with a `TransactionCapsule` built from `Transaction.getDefaultInstance()`, which lacks `raw_data`, causing `Util.printCreateTransaction` (via `printTransactionToJSON`) to dereference a missing `"raw_data"` field and throw a `NullPointerException`, so the servlet always goes through the error path in this test instead of exercising the normal success flow. Initializing the stubbed `Transaction` with an empty `raw_data` object avoids the NPE and more accurately reflects real usage.
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.| public void testGet() throws Exception { | ||
| MockHttpServletRequest request = new MockHttpServletRequest(); | ||
| request.setMethod("GET"); | ||
| request.addParameter("value", "test"); |
There was a problem hiding this comment.
Suggestion: The GET test passes a non-hex string as the value parameter, which causes ByteArray.fromHexString in the servlet to throw and forces the handler into the generic error path instead of the normal success path; this means the test does not actually exercise the intended behavior for valid input and may give a false sense of coverage. [logic error]
Severity Level: Major ⚠️
- ⚠️ GET servlet test only exercises generic error path.
- ⚠️ Normal `fillResponse` flow with hex input untested.
- ⚠️ `wallet.getAssetIssueByName` never exercised by GET test.
Suggested change
| request.addParameter("value", "test"); | |
| request.addParameter("value", "74657374"); |
Steps of Reproduction ✅
1. Run `GetAssetIssueByNameServletTest.testGet` in
`framework/src/test/java/org/tron/core/services/http/GetAssetIssueByNameServletTest.java:56-65`,
which creates a `MockHttpServletRequest` with method `GET` and sets `value` to `"test"` at
line 60.
2. The test calls `servlet.doGet(request, response)` at line 63; this executes
`GetAssetIssueByNameServlet.doGet` in
`framework/src/main/java/org/tron/core/services/http/GetAssetIssueByNameServlet.java:23-33`.
3. In `doGet`, `Util.getVisible(request)` in `Util.java:37-43` returns `false` because the
`visible` parameter is absent, so `input` remains `"test"` and
`ByteArray.fromHexString(input)` is invoked at `GetAssetIssueByNameServlet.java:30`.
4. `ByteArray.fromHexString("test")` in
`common/src/main/java/org/tron/common/utils/ByteArray.java:46-56` passes `"test"` to
`Hex.decode`, which throws due to non-hex characters; this is caught by the `catch
(Exception e)` in `doGet` (lines 31-33), which calls `Util.processError(e, response)` in
`Util.java:476-483`, writing an error JSON body but never calling `fillResponse` or
`wallet.getAssetIssueByName`, while the HTTP status remains the default 200, so
`assertEquals(200, response.getStatus())` at line 64 passes even though only the generic
error path was exercised.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:** 60:60
**Comment:**
*Logic Error: The GET test passes a non-hex string as the `value` parameter, which causes `ByteArray.fromHexString` in the servlet to throw and forces the handler into the generic error path instead of the normal success path; this means the test does not actually exercise the intended behavior for valid input and may give a false sense of coverage.
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.| public void testGet() throws Exception { | ||
| MockHttpServletRequest request = new MockHttpServletRequest(); | ||
| request.setMethod("GET"); | ||
| request.addParameter("value", "test"); |
There was a problem hiding this comment.
Suggestion: The GET test sends a non-hex string "test" as the value parameter without setting visible=true, which causes ByteArray.fromHexString("test") in the servlet to throw and be caught as an error; this means the test always exercises the error path rather than a valid request, so it doesn't verify normal servlet behavior and masks potential regressions. [logic error]
Severity Level: Major ⚠️
- ⚠️ GET unit test only covers invalid input error path.
- ⚠️ Normal GET handling for hex or visible=true remains untested.
- ⚠️ Potential regressions in valid-name decoding may go unnoticed.
Suggested change
| request.addParameter("value", "test"); | |
| // Use hex-encoded value to match the servlet's expected input format | |
| request.addParameter("value", "74657374"); |
Steps of Reproduction ✅
1. Run the unit test class `GetAssetIssueListByNameServletTest` at
`framework/src/test/java/org/tron/core/services/http/GetAssetIssueListByNameServletTest.java`,
which defines `testGet()` on lines 55–64.
2. In `setUp()` (lines 26–34 of the same file), JUnit initializes
`GetAssetIssueListByNameServlet` and injects a mocked `Wallet` whose
`getAssetIssueListByName(any())` always returns `null`.
3. `testGet()` creates a `MockHttpServletRequest`, sets method `GET`, and adds parameter
`value="test"` without a `visible` parameter, then calls `servlet.doGet(request,
response)` (lines 57–62).
4. In `GetAssetIssueListByNameServlet.doGet` at
`framework/src/main/java/org/tron/core/services/http/GetAssetIssueListByNameServlet.java:24–31`,
`visible` defaults to `false` via `Util.getVisible(request)` (which checks `visible`
parameter, defaulting to `false` in `Util.getVisible` at `Util.java:77–82`),
`value="test"` is passed to `fillResponse`, and `fillResponse` calls
`ByteArray.fromHexString(value)` on line 50 with `"test"`, which is not valid hex.
`ByteArray.fromHexString`
(`common/src/main/java/org/tron/common/utils/ByteArray.java:46–56`) delegates to
`Hex.decode(data)`, which throws on invalid hex; this exception is caught in `doGet`'s
catch block and handled by `Util.processError(e, response)` at `Util.java:217–224`,
writing an error JSON body but leaving HTTP status at 200. Thus the current test only
verifies that the error path returns status 200 for invalid input and never exercises the
normal successful path where a valid hex (or visible=true plain string) is decoded and
passed to `wallet.getAssetIssueListByName`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** framework/src/test/java/org/tron/core/services/http/GetAssetIssueListByNameServletTest.java
**Line:** 59:59
**Comment:**
*Logic Error: The GET test sends a non-hex string `"test"` as the `value` parameter without setting `visible=true`, which causes `ByteArray.fromHexString("test")` in the servlet to throw and be caught as an error; this means the test always exercises the error path rather than a valid request, so it doesn't verify normal servlet behavior and masks potential 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.doPost(request, response); | ||
| assertEquals(200, response.getStatus()); | ||
| assertNotNull(response.getContentAsString()); |
There was a problem hiding this comment.
Suggestion: The test currently only asserts that getContentAsString() is not null, but MockHttpServletResponse#getContentAsString() never returns null (it returns an empty string when no body is written), so this assertion can never fail and the test will not detect regressions in the JSON response produced by the servlet. To actually validate behavior—especially important after swapping JSON libraries—the test should inspect the response content for expected markers (e.g., "raw_data" on success or "Error" on failure), similar to existing servlet tests. [logic error]
Severity Level: Major ⚠️
- ❌ /wallet/transferasset JSON regressions not caught by tests.
- ⚠️ Jackson swap for transferasset not asserted via content.
- ⚠️ TransferAsset endpoint tests weaker than TransferServlet tests.
Suggested change
| assertNotNull(response.getContentAsString()); | |
| String content = response.getContentAsString(); | |
| assertNotNull(content); | |
| org.junit.Assert.assertTrue(content.contains("raw_data") || content.contains("Error")); |
Steps of Reproduction ✅
1. Run the unit test `TransferAssetServletTest.testTransferAsset` in
`framework/src/test/java/org/tron/core/services/http/TransferAssetServletTest.java:44-61`;
it constructs a `MockHttpServletRequest`, calls `TransferAssetServlet#doPost` and asserts
only HTTP 200 and `assertNotNull(response.getContentAsString())`.
2. Note that `TransferAssetServlet#doPost` in
`framework/src/main/java/org/tron/core/services/http/TransferAssetServlet.java:27-42`
writes the JSON body via `response.getWriter().println(Util.printCreateTransaction(tx,
visible))` on success, and `Util.processError(e, response)` in `Util.processError`
(`Util.java:17-24`) writes an error JSON using `printErrorMsg`, which sets an `"Error"`
field (`Util.printErrorMsg` at `Util.java:89-92`).
3. Compare with `TransferServletTest.testTransfer` in
`framework/src/test/java/org/tron/core/services/http/TransferServletTest.java:5-22`, which
after calling `TransferServlet#doPost` asserts `assertTrue(content.contains("raw_data") ||
content.contains("Error"));` on the response content, directly validating the JSON
structure.
4. Because Spring's `MockHttpServletResponse#getContentAsString()` never returns `null`
(it returns an empty string when nothing is written), the assertion
`assertNotNull(response.getContentAsString())` at `TransferAssetServletTest.java:60` can
never fail; for example, if a future change in `TransferAssetServlet#doPost` removes or
breaks the `response.getWriter().println(...)` call at `TransferAssetServlet.java:41`,
`/wallet/transferasset` would start returning an empty body, but `testTransferAsset` would
still pass, so regressions in the JSON response for `/wallet/transferasset` 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/TransferAssetServletTest.java
**Line:** 60:60
**Comment:**
*Logic Error: The test currently only asserts that `getContentAsString()` is not null, but `MockHttpServletResponse#getContentAsString()` never returns null (it returns an empty string when no body is written), so this assertion can never fail and the test will not detect regressions in the JSON response produced by the servlet. To actually validate behavior—especially important after swapping JSON libraries—the test should inspect the response content for expected markers (e.g., `"raw_data"` on success or `"Error"` on failure), similar to existing servlet tests.
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 · |
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
Replace Fastjson with Jackson across JSON handling
What Changed
Impact
✅ Removed Fastjson from runtime JSON handling✅ Lower risk from outdated JSON parsing code✅ Fewer API and JSON-RPC breakages after the library swap💡 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.