Copilot commented on code in PR #11127: URL: https://github.com/apache/gravitino/pull/11127#discussion_r3256191917
########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/provider/base/IdpGroupMetaBaseSQLProvider.java: ########## @@ -0,0 +1,68 @@ +/* + * 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 org.apache.gravitino.idp.storage.mapper.IdpGroupMetaMapper; +import org.apache.gravitino.idp.storage.po.IdpGroupPO; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupMetaBaseSQLProvider { + + public String selectIdpGroup(@Param("groupName") String groupName) { + return "SELECT group_id as groupId, group_name as groupName," + + " current_version as currentVersion," + + " last_version as lastVersion, deleted_at as deletedAt" + + " FROM " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " WHERE group_name = #{groupName} AND deleted_at = 0"; + } + + public String insertIdpGroup(@Param("groupMeta") IdpGroupPO groupPO) { + return "INSERT INTO " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " (group_id, group_name, current_version, last_version, deleted_at)" + + " VALUES (" + + " #{groupMeta.groupId}," + + " #{groupMeta.groupName}," + + " #{groupMeta.currentVersion}," + + " #{groupMeta.lastVersion}," + + " #{groupMeta.deletedAt}" + + " )"; + } + + public String softDeleteIdpGroup(@Param("groupId") Long groupId) { + return "UPDATE " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " SET deleted_at = " + + currentTimeMillisExpression() + + " WHERE group_id = #{groupId} AND deleted_at = 0"; + } + + public String deleteIdpGroupMetasByLegacyTimeline( + @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { + return "DELETE FROM " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT #{limit}"; + } + + protected String currentTimeMillisExpression() { + return "(UNIX_TIMESTAMP() * 1000.0)"; Review Comment: The MySQL time expression returns a floating-point value (`* 1000.0`). Since `deleted_at` is typically stored as an integer epoch-millis (and other providers explicitly return BIGINT/INTEGER), it’s safer and more consistent to return an integer expression (e.g., multiply by 1000 as an integer and/or cast to a BIGINT/SIGNED integer). This avoids implicit float-to-integer coercion and keeps cross-database semantics aligned. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/provider/postgresql/IdpGroupMetaPostgreSQLProvider.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.postgresql; + +import org.apache.gravitino.idp.storage.mapper.IdpGroupMetaMapper; +import org.apache.gravitino.idp.storage.mapper.provider.base.IdpGroupMetaBaseSQLProvider; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupMetaPostgreSQLProvider extends IdpGroupMetaBaseSQLProvider { + + @Override + protected String currentTimeMillisExpression() { + return "CAST(EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000 AS BIGINT)"; + } + + @Override + public String deleteIdpGroupMetasByLegacyTimeline( + @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { + return "DELETE FROM " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " WHERE group_id IN (SELECT group_id FROM " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT #{limit})"; Review Comment: This batched delete uses `LIMIT` without an `ORDER BY`, which makes the subset chosen per batch non-deterministic. For periodic cleanup jobs, adding a stable ordering (e.g., by `deleted_at` then `group_id`) improves predictability and can reduce variance in lock/IO patterns across runs. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/storage/mapper/TestIdpUserMetaStorage.java: ########## @@ -42,7 +42,16 @@ class TestIdpUserMetaStorage extends AbstractIdpMetaStorageTest { @MethodSource("storageProvider") void testInsertIdpUserAndSelectIdpUser(String type) throws IOException { init(type); - IdpUserPO firstUser = insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + IdpUserPO firstUser = + IdpUserPO.builder() + .withUserId(1L) + .withUserName("alice") + .withPasswordHash("hash-a") + .withCurrentVersion(1L) + .withLastVersion(0L) + .withDeletedAt(0L) + .build(); + idpUserMetaMapper.insertIdpUser(firstUser); Review Comment: This pattern (building an `IdpUserPO` and inserting it) is repeated many times in this test file after removing the shared helper. Consider reintroducing a small private helper (local to `TestIdpUserMetaStorage`, or restoring a shared one) to reduce duplication and make scenario setup more readable. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/IdpGroupMetaSQLProviderFactory.java: ########## @@ -0,0 +1,109 @@ +/* + * 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.Map; +import org.apache.gravitino.idp.storage.mapper.provider.base.IdpGroupMetaBaseSQLProvider; +import org.apache.gravitino.idp.storage.mapper.provider.h2.IdpGroupMetaH2Provider; +import org.apache.gravitino.idp.storage.mapper.provider.postgresql.IdpGroupMetaPostgreSQLProvider; +import org.apache.gravitino.idp.storage.po.IdpGroupPO; +import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupMetaSQLProviderFactory { + private static final IdpGroupMetaBaseSQLProvider IDP_GROUP_META_BASE_PROVIDER = + new IdpGroupMetaBaseSQLProvider(); + private static final IdpGroupMetaBaseSQLProvider IDP_GROUP_META_H2_PROVIDER = + new IdpGroupMetaH2Provider(); + private static final IdpGroupMetaBaseSQLProvider IDP_GROUP_META_POSTGRESQL_PROVIDER = + new IdpGroupMetaPostgreSQLProvider(); + + private static final Map<JDBCBackendType, IdpGroupMetaBaseSQLProvider> + IDP_GROUP_META_SQL_PROVIDER_MAP = + ImmutableMap.of( + JDBCBackendType.MYSQL, IDP_GROUP_META_BASE_PROVIDER, + JDBCBackendType.H2, IDP_GROUP_META_H2_PROVIDER, + JDBCBackendType.POSTGRESQL, IDP_GROUP_META_POSTGRESQL_PROVIDER); + + static IdpGroupMetaBaseSQLProvider getProvider(String databaseId) { + if (databaseId == null) { + throw new IllegalStateException( + "MyBatis databaseId is not configured for IdP group SQL providers."); + } + + try { + JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId); + IdpGroupMetaBaseSQLProvider provider = IDP_GROUP_META_SQL_PROVIDER_MAP.get(jdbcBackendType); + if (provider != null) { + return provider; + } + + throw new IllegalStateException( + String.format( + "No IdP group SQL provider registered for backend %s (databaseId: %s)", + jdbcBackendType, databaseId)); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + String.format( + "Unsupported IdP group SQL provider databaseId: %s, supported backends: %s", + databaseId, IDP_GROUP_META_SQL_PROVIDER_MAP.keySet()), + e); + } + } Review Comment: The provider-dispatch/error-handling logic here is new but doesn’t appear to have direct unit test coverage. Adding a small `TestIdpGroupMetaSQLProviderFactory` in the same package can validate: (1) `null` databaseId throws the expected `IllegalStateException`, (2) supported databaseIds map to the expected provider instance, and (3) an unsupported databaseId surfaces the wrapped exception/message. -- 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]
