Copilot commented on code in PR #11043: URL: https://github.com/apache/gravitino/pull/11043#discussion_r3220218009
########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,86 @@ +/* + * 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.storage.relational.mapper; + +import com.google.common.collect.ImmutableMap; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType; +import org.apache.gravitino.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider; +import org.apache.gravitino.storage.relational.mapper.provider.h2.IdpUserMetaH2Provider; +import org.apache.gravitino.storage.relational.mapper.provider.postgresql.IdpUserMetaPostgreSQLProvider; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.annotations.Param; + +public class IdpUserMetaSQLProviderFactory { + private static final Map<JDBCBackendType, IdpUserMetaBaseSQLProvider> + IDP_USER_META_SQL_PROVIDER_MAP = + ImmutableMap.of( + JDBCBackendType.MYSQL, new IdpUserMetaMySQLProvider(), + JDBCBackendType.H2, new IdpUserMetaH2Provider(), + JDBCBackendType.POSTGRESQL, new IdpUserMetaPostgreSQLProvider()); + + public static IdpUserMetaBaseSQLProvider getProvider() { + String databaseId = + SqlSessionFactoryHelper.getInstance() + .getSqlSessionFactory() + .getConfiguration() + .getDatabaseId(); + + JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId); + return IDP_USER_META_SQL_PROVIDER_MAP.get(jdbcBackendType); Review Comment: `getProvider()` can return `null` if `databaseId` is unset/unrecognized or if `fromString` maps to a backend not present in the map, which will then cause a `NullPointerException` when any factory method calls `getProvider().<method>()`. Add an explicit null-handling path (e.g., throw an `IllegalStateException` with the databaseId/backendType, or provide a safe default provider) so failures are deterministic and diagnosable. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/IdpUserMetaPostgreSQLProvider.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.storage.relational.mapper.provider.postgresql; + +import org.apache.gravitino.storage.relational.mapper.IdpUserMetaMapper; +import org.apache.gravitino.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider; +import org.apache.ibatis.annotations.Param; + +public class IdpUserMetaPostgreSQLProvider extends IdpUserMetaBaseSQLProvider { + + @Override + public String softDeleteIdpUser(Long userId, Long deletedAt) { + return "UPDATE " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " SET deleted_at = CAST(EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000 AS BIGINT)," + + " current_version = current_version + 1," + + " last_version = last_version + 1" + + " WHERE user_id = #{userId} AND deleted_at = 0"; + } Review Comment: The PostgreSQL override ignores the `deletedAt` argument and hard-codes `deleted_at` to the DB current timestamp, while MySQL/H2 use `#{deletedAt}` via the base provider. This creates cross-backend behavioral drift for the same mapper method signature. Consider aligning semantics by either (a) using `#{deletedAt}` in PostgreSQL too, or (b) switching all backends to consistently use DB-side time and removing/ignoring the `deletedAt` parameter across providers/mappers (ideally with a clear API contract). ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpUserMetaBaseSQLProvider.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.storage.relational.mapper.provider.base; + +import java.util.List; +import org.apache.gravitino.storage.relational.mapper.IdpUserMetaMapper; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.ibatis.annotations.Param; + +public class IdpUserMetaBaseSQLProvider { + + public String selectIdpUser(@Param("userName") String userName) { + return "SELECT user_id as userId, user_name as userName, password_hash as passwordHash," + + " current_version as currentVersion," + + " last_version as lastVersion, deleted_at as deletedAt" + + " FROM " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " WHERE user_name = #{userName} AND deleted_at = 0"; + } + + public String selectIdpUsers(@Param("userNames") List<String> userNames) { + return "<script>" + + "SELECT user_id as userId, user_name as userName, password_hash as passwordHash," + + " current_version as currentVersion," + + " last_version as lastVersion, deleted_at as deletedAt" + + " FROM " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " WHERE deleted_at = 0 AND user_name IN " + + "<foreach item='item' collection='userNames' open='(' separator=',' close=')'>" + + "#{item}" + + "</foreach>" Review Comment: `selectIdpUsers` will generate invalid SQL (`IN ()`) when `userNames` is empty, and may fail when `userNames` is null. Other providers in this PR explicitly guard empty/null collections using `<choose>` + `AND 1 = 0`. Consider adding the same guard here (and corresponding unit tests) to avoid runtime SQL syntax errors. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestIdpGroupUserRelBaseSQLProvider.java: ########## @@ -0,0 +1,253 @@ +/* + * 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.storage.relational.mapper.provider.base; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO; +import org.apache.ibatis.mapping.BoundSql; +import org.apache.ibatis.mapping.SqlSource; +import org.apache.ibatis.scripting.xmltags.XMLLanguageDriver; +import org.apache.ibatis.session.Configuration; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TestIdpGroupUserRelBaseSQLProvider { + + protected IdpGroupUserRelBaseSQLProvider createProvider() { + return new IdpGroupUserRelBaseSQLProvider(); + } + + protected String expectedDeleteAtClause() { + return "deleted_at = #{deletedAt}"; + } + + protected String expectedDeleteIdpGroupUserRelMetasByLegacyTimelineSql() { + return "DELETE FROM idp_group_user_rel WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT #{limit}"; + } + + @Test + public void testSelectGroupNamesByUserId() { + String normalizedSql = + createProvider().selectGroupNamesByUserId(1L).replaceAll("\\s+", " ").trim(); + + Assertions.assertTrue(normalizedSql.contains("SELECT g.group_name")); + Assertions.assertTrue( + normalizedSql.contains("FROM idp_group_user_rel r JOIN idp_group_meta g")); + Assertions.assertTrue(normalizedSql.contains("WHERE r.user_id = #{userId}")); + Assertions.assertTrue(normalizedSql.contains("ORDER BY g.group_name")); + } + + @Test + public void testSelectUserNamesByGroupId() { + String normalizedSql = + createProvider().selectUserNamesByGroupId(1L).replaceAll("\\s+", " ").trim(); + + Assertions.assertTrue(normalizedSql.contains("SELECT u.user_name")); + Assertions.assertTrue(normalizedSql.contains("FROM idp_group_user_rel r JOIN idp_user_meta u")); + Assertions.assertTrue(normalizedSql.contains("WHERE r.group_id = #{groupId}")); + Assertions.assertTrue(normalizedSql.contains("ORDER BY u.user_name")); + } + + @Test + public void testSelectRelatedUserIds() { + String script = createProvider().selectRelatedUserIds(1L, Arrays.asList(10L, 20L)); + Map<String, Object> params = new HashMap<>(); + params.put("groupId", 1L); + params.put("userIds", Arrays.asList(10L, 20L)); + + String normalizedSql = renderScript(script, params); + + Assertions.assertTrue(normalizedSql.contains("SELECT user_id FROM idp_group_user_rel")); + Assertions.assertTrue(normalizedSql.contains("WHERE group_id = ?")); + Assertions.assertTrue(normalizedSql.matches(".*user_id IN \\( \\? , \\? \\).*")); Review Comment: This regex is brittle with respect to whitespace formatting (it requires exactly `'( ? , ? )'` spacing). Since SQL rendering/normalization can vary across MyBatis versions/configuration, consider using a whitespace-tolerant pattern (e.g., allowing arbitrary `\\s*` around parentheses/comma) to reduce test flakiness. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpGroupUserRelBaseSQLProvider.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.storage.relational.mapper.provider.base; + +import java.util.List; +import org.apache.gravitino.storage.relational.mapper.IdpGroupUserRelMapper; +import org.apache.gravitino.storage.relational.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 " + + IdpGroupUserRelMapper.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 " + + IdpGroupUserRelMapper.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>"; + } Review Comment: `batchInsertIdpGroupUsers` generates invalid SQL when `relations` is empty (it will emit `INSERT ... VALUES` with no values). If an empty batch is possible, add a guard (e.g., `<choose>` to produce an unsatisfiable statement / no-op) or enforce a non-empty precondition with a clear exception so this doesn't fail as a SQL syntax error at runtime. -- 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]
