lasdf1234 commented on code in PR #11043: URL: https://github.com/apache/gravitino/pull/11043#discussion_r3224145882
########## 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: test ########## 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: Keeping this as-is for now. This PostgreSQL behavior matches the existing provider pattern in the repository, where PostgreSQL commonly uses DB-side current time instead of propagating the caller timestamp. I agree the cross-backend semantics are not perfectly uniform, but I do not want to broaden this PR beyond the current scope. ########## 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: Keeping this as-is for now. This factory follows the same pattern used by existing SQL provider factories in the repository, so adding a special null-handling path only here would make this PR inconsistent with the current codebase style. If we want stricter diagnostics for unsupported backends, I would prefer to handle that as a separate cleanup across the shared factory pattern. ########## 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: Keeping this as-is for now. The current batch-insert providers in this codebase generally rely on the caller contract to avoid empty collections, and this implementation follows that existing pattern. If we decide to harden empty-batch handling, I would rather do it consistently across the batch SQL providers in a follow-up change. ########## 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: Keeping the test as-is. The SQL assertion already normalizes whitespace with replaceAll("\\s+", " ").trim() before applying the regex, so the current pattern is not sensitive to formatter-only whitespace differences in practice. -- 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]
