Copilot commented on code in PR #11140: URL: https://github.com/apache/gravitino/pull/11140#discussion_r3263320140
########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/provider/base/IdpGroupUserRelBaseSQLProvider.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.gravitino.idp.storage.mapper.provider.base; + +import java.util.List; +import org.apache.gravitino.idp.storage.mapper.IdpGroupMetaMapper; +import org.apache.gravitino.idp.storage.mapper.IdpGroupUserRelMapper; +import org.apache.gravitino.idp.storage.mapper.IdpUserMetaMapper; +import org.apache.gravitino.idp.storage.po.IdpGroupUserRelPO; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupUserRelBaseSQLProvider { + + public String selectGroupNamesByUserId(@Param("userId") Long userId) { + return "SELECT g.group_name" + + " FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " r JOIN " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " g ON g.group_id = r.group_id" + + " WHERE r.user_id = #{userId}" + + " AND r.deleted_at = 0" + + " AND g.deleted_at = 0" + + " ORDER BY g.group_name"; + } + + public String selectUserNamesByGroupId(@Param("groupId") Long groupId) { + return "SELECT u.user_name" + + " FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " r JOIN " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " u ON u.user_id = r.user_id" + + " WHERE r.group_id = #{groupId}" + + " AND r.deleted_at = 0" + + " AND u.deleted_at = 0" + + " ORDER BY u.user_name"; + } + + public String selectRelatedUserIds( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds) { + return "<script>" + + "SELECT user_id" + + " FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " WHERE group_id = #{groupId} " + + "<choose>" + + "<when test='userIds != null and userIds.size() > 0'>" + + "AND user_id IN (" + + "<foreach collection='userIds' item='userId' separator=','>" + + "#{userId}" + + "</foreach>" + + ") " + + "</when>" + + "<otherwise>" + + "AND 1 = 0 " + + "</otherwise>" + + "</choose>" + + "AND deleted_at = 0" + + "</script>"; + } + + public String batchInsertIdpGroupUsers(@Param("relations") List<IdpGroupUserRelPO> relations) { + return "<script>" + + "INSERT INTO " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " (id, group_id, user_id, current_version, last_version, deleted_at)" + + " VALUES " + + "<foreach item='item' collection='relations' separator=','>" + + "(#{item.id}, #{item.groupId}, #{item.userId}, #{item.currentVersion}," + + " #{item.lastVersion}, #{item.deletedAt})" + + "</foreach>" + + "</script>"; + } + + public String softDeleteIdpGroupUsers( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds) { + return "<script>" + + "UPDATE " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + softDeleteSetClause() + + " WHERE group_id = #{groupId} " + + "<choose>" + + "<when test='userIds != null and userIds.size() > 0'>" + + "AND user_id IN (" + + "<foreach collection='userIds' item='userId' separator=','>" + + "#{userId}" + + "</foreach>" + + ") " + + "</when>" + + "<otherwise>" + + "AND 1 = 0 " + + "</otherwise>" + + "</choose>" + + "AND deleted_at = 0" + + "</script>"; + } + + public String softDeleteGroupUsersByUserId(@Param("userId") Long userId) { + return "UPDATE " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + softDeleteSetClause() + + " WHERE user_id = #{userId} AND deleted_at = 0"; + } + + public String softDeleteGroupUsersByGroupId(@Param("groupId") Long groupId) { + return "UPDATE " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + softDeleteSetClause() + + " WHERE group_id = #{groupId} AND deleted_at = 0"; + } + + public String deleteIdpGroupUserRelMetasByLegacyTimeline( + @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { + return "DELETE FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT #{limit}"; + } + + protected String softDeleteSetClause() { + return " SET deleted_at = " + + currentTimeMillisExpression() + + ", current_version = current_version + 1," + + " last_version = last_version + 1"; + } + + protected String currentTimeMillisExpression() { + return "(UNIX_TIMESTAMP() * 1000.0)"; Review Comment: Multiplying by `1000.0` makes this expression evaluate to a DOUBLE/DECIMAL, but it is used to populate the `deleted_at` column which is a BIGINT/epoch-millis value (the H2 and PostgreSQL overrides correctly return integer milliseconds). Under MySQL this can store a non-integer timestamp and lose precision/cause comparison oddities against the integer `deleted_at` values used elsewhere (e.g., `deleted_at < #{legacyTimeline}`). Consider using `(UNIX_TIMESTAMP(CURRENT_TIMESTAMP(3)) * 1000)` or `CAST(UNIX_TIMESTAMP() * 1000 AS UNSIGNED)` so the result is an integer milliseconds value. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/IdpGroupUserRelSQLProviderFactory.java: ########## @@ -0,0 +1,94 @@ +/* + * 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.gravitino.idp.storage.mapper; + +import com.google.common.collect.ImmutableMap; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.idp.storage.mapper.provider.base.IdpGroupUserRelBaseSQLProvider; +import org.apache.gravitino.idp.storage.mapper.provider.h2.IdpGroupUserRelH2Provider; +import org.apache.gravitino.idp.storage.mapper.provider.postgresql.IdpGroupUserRelPostgreSQLProvider; +import org.apache.gravitino.idp.storage.po.IdpGroupUserRelPO; +import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupUserRelSQLProviderFactory { + private static final IdpGroupUserRelBaseSQLProvider MYSQL_PROVIDER = + new IdpGroupUserRelBaseSQLProvider(); + private static final IdpGroupUserRelBaseSQLProvider H2_PROVIDER = new IdpGroupUserRelH2Provider(); + private static final IdpGroupUserRelBaseSQLProvider POSTGRESQL_PROVIDER = + new IdpGroupUserRelPostgreSQLProvider(); + private static final Map<JDBCBackendType, IdpGroupUserRelBaseSQLProvider> PROVIDER_MAP = + ImmutableMap.of( + JDBCBackendType.MYSQL, + MYSQL_PROVIDER, + JDBCBackendType.H2, + H2_PROVIDER, + JDBCBackendType.POSTGRESQL, + POSTGRESQL_PROVIDER); + + private IdpGroupUserRelSQLProviderFactory() {} + + private static IdpGroupUserRelBaseSQLProvider currentProvider() { + return SQLProviderFactoryHelper.currentProvider( + PROVIDER_MAP, IdpGroupUserRelSQLProviderFactory.class); + } + + static IdpGroupUserRelBaseSQLProvider getProvider(String databaseId) { + return SQLProviderFactoryHelper.getProvider( + databaseId, PROVIDER_MAP, IdpGroupUserRelSQLProviderFactory.class); + } + + public static String selectGroupNamesByUserId(@Param("userId") Long userId) { + return currentProvider().selectGroupNamesByUserId(userId); + } + + public static String selectUserNamesByGroupId(@Param("groupId") Long groupId) { + return currentProvider().selectUserNamesByGroupId(groupId); + } + + public static String selectRelatedUserIds( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds) { + return currentProvider().selectRelatedUserIds(groupId, userIds); + } + + public static String batchInsertIdpGroupUsers( + @Param("relations") List<IdpGroupUserRelPO> relations) { + return currentProvider().batchInsertIdpGroupUsers(relations); + } + + public static String softDeleteIdpGroupUsers( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds) { + return currentProvider().softDeleteIdpGroupUsers(groupId, userIds); + } + + public static String softDeleteGroupUsersByUserId(@Param("userId") Long userId) { + return currentProvider().softDeleteGroupUsersByUserId(userId); + } + + public static String softDeleteGroupUsersByGroupId(@Param("groupId") Long groupId) { + return currentProvider().softDeleteGroupUsersByGroupId(groupId); + } Review Comment: `@Param` annotations are only meaningful on MyBatis mapper interface methods; on these static provider-factory methods they have no effect and can mislead readers into thinking MyBatis is binding parameters here. Consider removing the `@Param` annotations from this factory class (and similarly from the base/PostgreSQL provider classes) and keeping them only on the `IdpGroupUserRelMapper` interface, where they actually drive parameter binding. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/IdpGroupUserRelMapper.java: ########## @@ -0,0 +1,81 @@ +/* + * 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.gravitino.idp.storage.mapper; + +import java.util.List; +import org.apache.gravitino.idp.storage.po.IdpGroupUserRelPO; +import org.apache.ibatis.annotations.DeleteProvider; +import org.apache.ibatis.annotations.InsertProvider; +import org.apache.ibatis.annotations.Param; +import org.apache.ibatis.annotations.SelectProvider; +import org.apache.ibatis.annotations.UpdateProvider; + +/** + * A MyBatis mapper for built-in IdP group-user relation operations. + * + * <p>This interface defines the SQL statements MyBatis executes for the built-in IdP group-user + * relation store. The SQLs are provided through {@code *Provider} annotations on this mapper + * interface. See the <a href="https://mybatis.org/mybatis-3/getting-started.html";>MyBatis getting + * started guide</a>. + */ +public interface IdpGroupUserRelMapper { + String IDP_GROUP_USER_REL_TABLE_NAME = "idp_group_user_rel"; + + @SelectProvider( + type = IdpGroupUserRelSQLProviderFactory.class, + method = "selectGroupNamesByUserId") + List<String> selectGroupNamesByUserId(@Param("userId") Long userId); + + @SelectProvider( + type = IdpGroupUserRelSQLProviderFactory.class, + method = "selectUserNamesByGroupId") + List<String> selectUserNamesByGroupId(@Param("groupId") Long groupId); + + @SelectProvider(type = IdpGroupUserRelSQLProviderFactory.class, method = "selectRelatedUserIds") + List<Long> selectRelatedUserIds( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds); + + @InsertProvider( + type = IdpGroupUserRelSQLProviderFactory.class, + method = "batchInsertIdpGroupUsers") + void batchInsertIdpGroupUsers(@Param("relations") List<IdpGroupUserRelPO> relations); + + @UpdateProvider( + type = IdpGroupUserRelSQLProviderFactory.class, + method = "softDeleteIdpGroupUsers") + void softDeleteIdpGroupUsers( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds); + + @UpdateProvider( + type = IdpGroupUserRelSQLProviderFactory.class, + method = "softDeleteGroupUsersByUserId") + void softDeleteGroupUsersByUserId(@Param("userId") Long userId); + + @UpdateProvider( + type = IdpGroupUserRelSQLProviderFactory.class, + method = "softDeleteGroupUsersByGroupId") + void softDeleteGroupUsersByGroupId(@Param("groupId") Long groupId); Review Comment: These soft-delete methods return `void`, while `deleteIdpGroupUserRelMetasByLegacyTimeline` returns `Integer`. Callers will frequently want to know whether any rows were actually updated (e.g., to short-circuit, log, or verify idempotency). Consider returning `Integer`/`int` (the number of affected rows) from the soft-delete methods for consistency and to give callers useful information. -- 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]
