Copilot commented on code in PR #8027:
URL: https://github.com/apache/incubator-seata/pull/8027#discussion_r3025681213
##########
saga/seata-saga-annotation/src/main/java/org/apache/seata/saga/rm/SagaAnnotationResourceManager.java:
##########
@@ -100,12 +103,15 @@ public BranchStatus branchRollback(
String.format("SagaAnnotation resource is not available,
resourceId: %s", resourceId));
}
+ BusinessActionContext businessActionContext = null;
try {
- BusinessActionContext businessActionContext =
+ businessActionContext =
BusinessActionContextUtil.getBusinessActionContext(xid,
branchId, resourceId, applicationData);
Object[] args = this.getTwoPhaseRollbackArgs(resource,
businessActionContext);
BusinessActionContextUtil.setContext(businessActionContext);
Review Comment:
`doAfterSagaAnnotationRollback(...)` is invoked in `finally` even when
`BusinessActionContextUtil.getBusinessActionContext(...)` fails and
`businessActionContext` remains `null`, so hooks may receive a null `context`.
Even though per-hook exceptions are caught, calling hooks with a null context
changes callback semantics and can break hook implementations relying on a
non-null context. Guard the `doAfterSagaAnnotationRollback(...)` call (and any
other hook invocation) with `if (businessActionContext != null)` so hooks are
only called when a valid context exists.
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/ActionInterceptorHandler.java:
##########
@@ -325,4 +343,32 @@ protected Map<String, Object>
fetchActionRequestContext(Method method, Object[]
}
return context;
}
+
+ /**
+ * Report action status to TC
+ *
+ * @param actionContext the action context
+ * @param status the action status (success/failed)
+ */
+ protected void reportActionStatus(BusinessActionContext actionContext,
String status) {
+ try {
+ actionContext.setActionStatus(status);
+ actionContext.setUpdated(true);
+ BusinessActionContextUtil.reportContext(actionContext);
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug(
+ "Report action status: xid={}, branchId={}, status={}",
+ actionContext.getXid(),
+ actionContext.getBranchId(),
+ status);
+ }
+ } catch (Exception e) {
+ LOGGER.warn(
+ "Report action status failed: xid={}, branchId={},
status={}, error={}",
+ actionContext.getXid(),
+ actionContext.getBranchId(),
+ status,
+ e.getMessage());
Review Comment:
The warning log drops the exception stack trace, which makes diagnosing
production/reporting failures harder. Pass `e` as the last argument to the
logger (and optionally remove `error={}`/`e.getMessage()`), so stack traces are
captured in logs.
```suggestion
e.getMessage(),
e);
```
##########
integration-tx-api/src/test/java/org/apache/seata/integration/tx/api/interceptor/ActionInterceptorHandlerReportTest.java:
##########
@@ -0,0 +1,205 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.integration.tx.api.interceptor;
+
+import org.apache.seata.common.Constants;
+import org.apache.seata.common.executor.Callback;
+import org.apache.seata.core.model.BranchType;
+import org.apache.seata.integration.tx.api.fence.hook.TccHookManager;
+import org.apache.seata.rm.tcc.api.BusinessActionContext;
+import org.apache.seata.rm.tcc.api.BusinessActionContextUtil;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Tests for action status report functionality in ActionInterceptorHandler.
+ *
+ * Covers:
+ * 1. reportActionStatus method (success/failed/exception paths)
+ * 2. ENABLE_ACTION_STATUS_REPORT conditional branches in proceed method
+ */
+public class ActionInterceptorHandlerReportTest {
+
+ private MockedStatic<BusinessActionContextUtil> mockedContextUtil;
+ private boolean originalEnableActionStatusReport;
+
+ @BeforeEach
+ void setUp() throws Exception {
+ TccHookManager.clear();
+ mockedContextUtil =
Mockito.mockStatic(BusinessActionContextUtil.class);
+ mockedContextUtil
+ .when(() -> BusinessActionContextUtil.reportContext(any()))
+ .thenReturn(true);
+
+ // Enable action status report via reflection
+ originalEnableActionStatusReport = setEnableActionStatusReport(true);
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ mockedContextUtil.close();
+ TccHookManager.clear();
+
+ // Restore original value
+ setEnableActionStatusReport(originalEnableActionStatusReport);
+ }
+
+ private boolean setEnableActionStatusReport(boolean value) throws
Exception {
+ Field field =
ActionInterceptorHandler.class.getDeclaredField("ENABLE_ACTION_STATUS_REPORT");
+ field.setAccessible(true);
+
+ // Use sun.misc.Unsafe to modify static final field (works on all Java
versions)
+ Field theUnsafe = sun.misc.Unsafe.class.getDeclaredField("theUnsafe");
+ theUnsafe.setAccessible(true);
+ sun.misc.Unsafe unsafe = (sun.misc.Unsafe) theUnsafe.get(null);
+
+ Object base = unsafe.staticFieldBase(field);
+ long offset = unsafe.staticFieldOffset(field);
+ boolean original = unsafe.getBoolean(base, offset);
+ unsafe.putBoolean(base, offset, value);
+ return original;
+ }
Review Comment:
This test relies on `sun.misc.Unsafe` to mutate a `static final` flag (and
the comment claiming “works on all Java versions” is inaccurate). This approach
is brittle under JPMS/module restrictions and can break test runs depending on
JVM flags. Prefer refactoring production code to make the flag testable without
`Unsafe` (e.g., a non-final flag, a protected/package-private
`isActionStatusReportEnabled()` method, or injecting a configuration reader
that can be mocked).
##########
saga/seata-saga-annotation/src/main/java/org/apache/seata/saga/rm/SagaAnnotationResourceManager.java:
##########
@@ -164,4 +171,48 @@ protected Object[] getTwoPhaseMethodParams(
}
return args;
}
+
+ /**
+ * to do some business operations before saga annotation rollback
+ * @param xid the xid
+ * @param branchId the branchId
+ * @param actionName the actionName
+ * @param context the business action context
+ */
+ private void doBeforeSagaAnnotationRollback(
+ String xid, long branchId, String actionName,
BusinessActionContext context) {
+ List<TccHook> hooks = TccHookManager.getHooks();
+ if (hooks.isEmpty()) {
+ return;
+ }
+ for (TccHook hook : hooks) {
+ try {
+ hook.beforeTccRollback(xid, branchId, actionName, context);
+ } catch (Exception e) {
+ LOGGER.error("Failed execute beforeTccRollback in hook {}",
e.getMessage(), e);
+ }
Review Comment:
The log message `"Failed execute beforeTccRollback in hook {}"` is
grammatically incorrect and the `{}` placeholder is currently used for the
exception message, which loses useful context about *which* hook failed.
Consider changing the message to “Failed to execute …” and log hook identity
(e.g., hook class/name) in the placeholder; keep the exception as the throwable
argument for stack trace.
--
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]
