Copilot commented on code in PR #11140: URL: https://github.com/apache/gravitino/pull/11140#discussion_r3263287786
########## 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}"; Review Comment: The base provider is registered as the MySQL provider in `IdpGroupUserRelSQLProviderFactory`, but `DELETE ... LIMIT` is MySQL-specific and is not valid standard SQL. The PostgreSQL provider correctly overrides this method, but H2 inherits this base implementation and H2 does not support `DELETE ... LIMIT` in all modes. Consider either overriding this in the H2 provider (mirroring the PostgreSQL approach with a subselect) or documenting the dialect assumption explicitly to avoid runtime failures on H2. ########## 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: `UNIX_TIMESTAMP() * 1000.0` returns a floating-point value, but the `deleted_at` column appears to be a `BIGINT` (the PostgreSQL provider casts to `BIGINT`, and the test compares to `> 0L`). Multiplying by `1000.0` can yield sub-second precision loss/representation issues and produces a non-integer value being assigned to an integer column. Consider using `UNIX_TIMESTAMP(NOW(3)) * 1000` with integer arithmetic or `(UNIX_TIMESTAMP() * 1000)` to keep the result as an integer. ########## 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); + } Review Comment: The `@Param` annotations on these static factory methods have no effect — MyBatis only honors `@Param` on mapper interface method parameters (which are already correctly annotated on `IdpGroupUserRelMapper`). The annotations on the factory's static methods are misleading and can be removed for clarity. This applies to all `@Param`-annotated parameters in this file (and similarly on the `IdpGroupUserRelPostgreSQLProvider.deleteIdpGroupUserRelMetasByLegacyTimeline` method). -- 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]
