Copilot commented on code in PR #8054:
URL: https://github.com/apache/incubator-seata/pull/8054#discussion_r3099113897
##########
console/src/main/java/org/apache/seata/mcp/tools/NameSpaceTools.java:
##########
@@ -44,14 +43,10 @@ public NameSpaceTools(ConsoleApiService mcpRPCService,
ObjectMapper objectMapper
public SingleResult<?> getTCNameSpaces() {
String result =
mcpRPCService.getCallNameSpace(RPCConstant.GET_NAMESPACE_PATH);
Map<String, Object> nameSpacesVo = new HashMap<>();
- try {
- JsonNode root = objectMapper.readTree(result);
- JsonNode dataNode = root.get("data");
- if (dataNode != null && !dataNode.isNull()) {
- nameSpacesVo.put("namespaces", dataNode.toString());
- }
- } catch (JsonProcessingException e) {
- return SingleResult.failure("Get namespace failed:" +
e.getMessage());
+ JsonNode root = objectMapper.readTree(result);
+ JsonNode dataNode = root.get("data");
+ if (dataNode != null && !dataNode.isNull()) {
+ nameSpacesVo.put("namespaces", dataNode.toString());
}
return SingleResult.success(nameSpacesVo);
Review Comment:
`getTCNameSpaces()` now parses the RPC response without any error handling.
If the TC returns a non-JSON error body (or a different JSON shape),
`readTree()`/`get("data")` can throw and the tool call will fail with an
exception instead of returning a `SingleResult.failure(...)`. Consider
restoring a try/catch around JSON parsing and returning a structured failure
message so MCP clients get a predictable response.
##########
server/src/test/java/org/apache/seata/server/logging/AppenderTest.java:
##########
@@ -41,9 +40,10 @@ public static void init() {
System.setProperty("logging.extend.metric-appender.enabled", "true");
}
- @Test
+ // @Test
public void testAppenderEnabled() {
- LoggerContext lc = (LoggerContext)
StaticLoggerBinder.getSingleton().getLoggerFactory();
+ LoggerContext lc =
+ (LoggerContext)
ContextSelectorStaticBinder.getSingleton().getContextSelector();
Iterator<Appender<ILoggingEvent>> appenderIterator =
Review Comment:
The test is currently disabled by commenting out @Test, so it will no longer
validate that the extra appenders are correctly configured. Additionally,
`ContextSelectorStaticBinder.getSingleton().getContextSelector()` does not
return a `LoggerContext`, so the current cast would fail if the test is
re-enabled. Please re-enable the test and obtain the `LoggerContext` via the
proper API (e.g., through the SLF4J/Logback `ILoggerFactory` or the context
selector’s logger context accessor) so the assertions actually execute.
##########
server/src/main/java/org/apache/seata/server/storage/redis/store/RedisTransactionStoreManager.java:
##########
@@ -500,7 +499,7 @@ public List<GlobalSession>
readSortByTimeoutBeginSessions(boolean withBranchSess
// queryCount
final long queryCount = Math.min(logQueryLimit, countGlobalSessions);
try (Jedis jedis = JedisPooledFactory.getJedisInstance()) {
- Set<String> values = jedis.zrangeByScore(
+ List<String> values = jedis.zrangeByScore(
REDIS_SEATA_BEGIN_TRANSACTIONS_KEY, 0,
System.currentTimeMillis(), 0, (int) queryCount);
List<Map<String, String>> rep;
Review Comment:
`jedis.zrangeByScore(...)` is now assigned to `List<String>`, but this repo
currently pins Jedis to 3.8.0 (see `dependencies/pom.xml`), where
`zrangeByScore` returns a `Set<String>`. Unless the Jedis dependency is also
upgraded as part of this PR, this change will not compile. Either keep the
`Set<String>` return type for Jedis 3.x, or update the Jedis version and adjust
all call sites consistently.
##########
namingserver/src/main/java/org/apache/seata/namingserver/service/ConsoleLocalServiceImpl.java:
##########
@@ -157,13 +156,6 @@ public String putCallTC(
@Override
public String getCallNameSpace(String path) {
- String namespace;
- try {
- namespace =
objectMapper.writeValueAsString(namingManager.namespace());
- } catch (JsonProcessingException e) {
- LOGGER.error("Get NameSpace failed: {}", e.getMessage());
- return "Failed to get namespace";
- }
- return namespace;
+ return objectMapper.writeValueAsString(namingManager.namespace());
Review Comment:
`getCallNameSpace` no longer handles JSON serialization failures.
Previously, `JsonProcessingException` was caught and a fallback string was
returned; now any `JacksonException` will propagate and can turn a simple “get
namespace” call into an unhandled 500 error. Please restore explicit exception
handling here (e.g., log + return a consistent failure payload, or wrap into a
`ServiceCallException`).
##########
console/src/main/java/org/apache/seata/mcp/tools/GlobalLockTools.java:
##########
@@ -92,11 +91,7 @@ public PageResult<McpGlobalLockVO> queryGlobalLock(
PageResult<McpGlobalLockVO> result = null;
String response = mcpRPCService.getCallTC(
nameSpaceDetail, RPCConstant.GLOBAL_LOCK_BASE_URL + "/query",
param, null, null);
- try {
- result = objectMapper.readValue(response, new
TypeReference<PageResult<McpGlobalLockVO>>() {});
- } catch (JsonProcessingException e) {
- logger.error(e.getMessage());
- }
+ result = objectMapper.readValue(response, new
TypeReference<PageResult<McpGlobalLockVO>>() {});
Review Comment:
`queryGlobalLock()` deserializes the remote response without guarding
against parse errors. Because `response` is returned from an HTTP call, it may
be empty or non-JSON in failure scenarios; without a try/catch this will throw
and bypass the existing `PageResult.failure(...)` path. Please add explicit
error handling (and optionally logging) around `readValue(...)` so callers get
a consistent failure result.
##########
server/src/test/java/org/apache/seata/server/storage/redis/store/RedisTransactionStoreManagerTest.java:
##########
@@ -292,7 +291,7 @@ public void testDeleteGlobalTransactionDO() throws
Exception {
List<String> statusList = jedis.lrange(statusKey, 0, -1);
Assertions.assertFalse(statusList.contains("testGlobalXid:222"));
- Set<String> timeoutSet =
jedis.zrangeByScore("SEATA_BEGIN_TRANSACTIONS", 0, Double.MAX_VALUE);
+ List<String> timeoutSet =
jedis.zrangeByScore("SEATA_BEGIN_TRANSACTIONS", 0, Double.MAX_VALUE);
Assertions.assertFalse(timeoutSet.contains(globalKey));
Review Comment:
Same issue as above: `zrangeByScore` is treated as returning a
`List<String>`, but with Jedis 3.8.0 it returns a `Set<String>`. Please keep
the type consistent with the Jedis version selected by dependency management,
or upgrade Jedis and update all call sites.
##########
server/src/test/java/org/apache/seata/server/storage/redis/store/RedisTransactionStoreManagerTest.java:
##########
@@ -254,7 +253,7 @@ public void testInsertGlobalTransactionDO() throws
Exception {
Assertions.assertTrue(statusList.contains("testGlobalXid:111"));
// Verify timeout sorted set
- Set<String> timeoutSet =
jedis.zrangeByScore("SEATA_BEGIN_TRANSACTIONS", 0, Double.MAX_VALUE);
+ List<String> timeoutSet =
jedis.zrangeByScore("SEATA_BEGIN_TRANSACTIONS", 0, Double.MAX_VALUE);
Assertions.assertTrue(timeoutSet.contains(globalKey));
Review Comment:
This changes `jedis.zrangeByScore(...)` to return `List<String>`, but with
the current Jedis version used in the repo (3.8.0) the method returns
`Set<String>`. Unless Jedis is upgraded, this test will not compile. Align the
type with the Jedis API used by the build.
##########
console/src/main/java/org/apache/seata/mcp/tools/GlobalSessionTools.java:
##########
@@ -102,11 +101,7 @@ public PageResult<McpGlobalSessionVO> queryGlobalSession(
PageResult<McpGlobalSessionVO> pageResult = null;
String result = mcpRPCService.getCallTC(
nameSpaceDetail, RPCConstant.GLOBAL_SESSION_BASE_URL +
"/query", param, null, null);
- try {
- pageResult = objectMapper.readValue(result, new
TypeReference<PageResult<McpGlobalSessionVO>>() {});
- } catch (JsonProcessingException e) {
- logger.error(e.getMessage());
- }
+ pageResult = objectMapper.readValue(result, new
TypeReference<PageResult<McpGlobalSessionVO>>() {});
if (pageResult == null) {
Review Comment:
`queryGlobalSession()` now calls `objectMapper.readValue(...)` without
handling parse failures. Since `result` comes from a remote call, non-JSON
responses (timeouts, HTML error pages, etc.) can now bubble up as an exception
rather than returning a `PageResult.failure(...)`. Please handle JSON
parse/deserialize errors explicitly (and consider logging the response or
exception) to keep the tool behavior predictable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
