Copilot commented on code in PR #8138: URL: https://github.com/apache/incubator-seata/pull/8138#discussion_r3451007888
########## integration-tx-api/src/test/java/org/apache/seata/integration/tx/api/fence/store/db/sql/CommonFenceStoreSqlsTest.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.fence.store.db.sql; + +import org.apache.seata.integration.tx.api.fence.constant.CommonFenceConstant; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class CommonFenceStoreSqlsTest { + + private static final String TABLE = "tcc_fence_log"; + + private static final String END_STATUS_IN = "status in (" + CommonFenceConstant.STATUS_COMMITTED + " , " + + CommonFenceConstant.STATUS_ROLLBACKED + " , " + CommonFenceConstant.STATUS_SUSPENDED + ")"; + + /** + * The date-based cleanup must select distinct xids, so a row limit bounds the number of distinct xids + * (one xid may own multiple branch rows) and the limit comparison in the cleanup loop stays consistent. + */ + @Test + public void queryEndStatusByDateSelectsDistinctXids() { + String mysql = CommonFenceStoreSqls.getQueryEndStatusSQLByDate(TABLE, false); + assertTrue(mysql.contains("select distinct xid"), mysql); + assertTrue(mysql.contains("gmt_modified <"), mysql); + assertTrue(mysql.contains(END_STATUS_IN), mysql); + assertTrue(mysql.contains("limit ?"), mysql); + + String oracle = CommonFenceStoreSqls.getQueryEndStatusSQLByDate(TABLE, true); + assertTrue(oracle.contains("ROWNUM <= ?"), oracle); + } + + /** + * Core regression: deleting expired fence logs by xid must also be restricted by gmt_modified and end status. + * Without these predicates, deleting by xid alone would purge sibling branch rows of the same global + * transaction that are still in progress (TRIED) or not yet expired. + */ + @Test + public void deleteByXidsIsRestrictedByDateAndEndStatus() { + String sql = CommonFenceStoreSqls.getDeleteSQLByXids(TABLE, "?, ?"); + + assertTrue(sql.contains("xid in (?, ?)"), sql); + // must not be a bare delete-by-xid: the date and end-status guards have to be present + assertTrue(sql.contains("gmt_modified <"), sql); + assertTrue(sql.contains(END_STATUS_IN), sql); + // the in-progress status must never be a deletion target + assertFalse(sql.contains("status in (" + CommonFenceConstant.STATUS_TRIED), sql); Review Comment: The assertion on line 62 is substring-based and can produce false failures or false passes if status codes share prefixes (e.g., `STATUS_TRIED=1` and another status is `11`) or if formatting changes (spaces/newlines). Prefer asserting the exact expected end-status list (e.g., match the full `status in (...)` clause) or parse/extract the `status in (...)` contents and compare as a set of integers. ########## integration-tx-api/src/test/java/org/apache/seata/integration/tx/api/fence/store/db/sql/CommonFenceStoreSqlsTest.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.fence.store.db.sql; + +import org.apache.seata.integration.tx.api.fence.constant.CommonFenceConstant; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class CommonFenceStoreSqlsTest { + + private static final String TABLE = "tcc_fence_log"; + + private static final String END_STATUS_IN = "status in (" + CommonFenceConstant.STATUS_COMMITTED + " , " + + CommonFenceConstant.STATUS_ROLLBACKED + " , " + CommonFenceConstant.STATUS_SUSPENDED + ")"; + + /** + * The date-based cleanup must select distinct xids, so a row limit bounds the number of distinct xids + * (one xid may own multiple branch rows) and the limit comparison in the cleanup loop stays consistent. + */ + @Test + public void queryEndStatusByDateSelectsDistinctXids() { + String mysql = CommonFenceStoreSqls.getQueryEndStatusSQLByDate(TABLE, false); + assertTrue(mysql.contains("select distinct xid"), mysql); + assertTrue(mysql.contains("gmt_modified <"), mysql); + assertTrue(mysql.contains(END_STATUS_IN), mysql); + assertTrue(mysql.contains("limit ?"), mysql); Review Comment: These `contains(...)` checks are sensitive to harmless SQL formatting changes (whitespace, line breaks, casing). To reduce test brittleness, normalize SQL before assertions (e.g., collapse whitespace and lowercase), or use a regex matcher focused on structure rather than exact spacing. ########## integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/fence/store/db/sql/CommonFenceStoreSqls.java: ########## @@ -86,16 +88,14 @@ private CommonFenceStoreSqls() { "delete from " + LOCAL_TCC_LOG_PLACEHOLD + " where xid = ? and branch_id = ? "; /** - * The constant DELETE_BY_BRANCH_ID_AND_XID. - */ - protected static final String DELETE_BY_BRANCH_XIDS = - "delete from " + LOCAL_TCC_LOG_PLACEHOLD + " where xid in (" + PRAMETER_PLACEHOLD + ")"; - - /** - * The constant DELETE_BY_DATE_AND_STATUS. + * The constant DELETE_BY_BRANCH_XIDS. + * The gmt_modified and status predicates must match {@link #QUERY_END_STATUS_BY_DATE}: deleting by xid alone + * would also remove sibling branch rows of the same global transaction that are still in a non-end status + * (e.g. TRIED) or not yet expired, which must be preserved. */ - protected static final String DELETE_BY_DATE_AND_STATUS = "delete from " + LOCAL_TCC_LOG_PLACEHOLD - + " where gmt_modified < ? " + protected static final String DELETE_BY_BRANCH_XIDS = "delete from " + LOCAL_TCC_LOG_PLACEHOLD + " where xid in (" + + PRAMETER_PLACEHOLD + ")" + + " and gmt_modified < ? " + " and status in (" + CommonFenceConstant.STATUS_COMMITTED + " , " + CommonFenceConstant.STATUS_ROLLBACKED + " , " + CommonFenceConstant.STATUS_SUSPENDED + ")"; Review Comment: `DELETE_BY_BRANCH_XIDS` is misleading now that the SQL is “delete by xids + datetime + end-status” and doesn’t reference `branch_id`. Consider renaming to reflect the actual behavior (e.g., `DELETE_BY_XIDS_AND_DATE_AND_END_STATUS`) to reduce confusion and make call sites self-documenting. -- 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]
