Copilot commented on code in PR #11066: URL: https://github.com/apache/gravitino/pull/11066#discussion_r3245257854
########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/AbstractIdpUserMetaStorageTest.java: ########## @@ -0,0 +1,213 @@ +/* + * 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.basic.storage.relational.mapper; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.DatabaseMetaData; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.Comparator; +import java.util.stream.Stream; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.gravitino.integration.test.container.ContainerSuite; +import org.apache.gravitino.integration.test.util.BaseIT; +import org.apache.gravitino.storage.relational.JDBCBackend; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.session.SqlSession; +import org.junit.jupiter.api.AfterEach; + +abstract class AbstractIdpUserMetaStorageTest { + protected String backendType; + protected JDBCBackend backend; + protected SqlSession sharedSession; + protected IdpUserMetaMapper idpUserMetaMapper; + + private Config config; + private Path h2Path; + + static Stream<String> storageProvider() { + return Stream.of("h2", "mysql", "postgresql"); + } + + @AfterEach + void closeSuite() throws IOException { + closeSession(); + if (backend != null) { + backend.close(); + backend = null; + } + + SqlSessionFactoryHelper.getInstance().close(); + ContainerSuite.getInstance().close(); + + if (h2Path != null && Files.exists(h2Path)) { + deleteDirectory(h2Path); + h2Path = null; + } + } + + protected void init(String type) throws IOException { + backendType = type; + config = createBackendConfig(type); + backend = new JDBCBackend(); + backend.close(); Review Comment: Calling `backend.close()` on a freshly constructed `JDBCBackend` before `initialize(config)` is unexpected and most likely unintended. At best it is dead code; at worst, depending on `JDBCBackend.close()`'s tolerance of an uninitialized instance, it could throw an NPE or leave internal state in a way that interferes with the subsequent `initialize` call. Please remove this `close()` call (it is correctly absent from `restartBackend()`). ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.basic.storage.relational.mapper; + +import com.google.common.collect.ImmutableMap; +import java.util.List; +import java.util.Map; +import org.apache.gravitino.idp.basic.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.annotations.Param; + +public class IdpUserMetaSQLProviderFactory { + private static final IdpUserMetaBaseSQLProvider IDP_USER_META_H2_PROVIDER = + new IdpUserMetaH2Provider(); + private static final IdpUserMetaBaseSQLProvider IDP_USER_META_MYSQL_PROVIDER = + new IdpUserMetaMySQLProvider(); + private static final IdpUserMetaBaseSQLProvider IDP_USER_META_POSTGRESQL_PROVIDER = + new IdpUserMetaPostgreSQLProvider(); + + private static final Map<JDBCBackendType, IdpUserMetaBaseSQLProvider> + IDP_USER_META_SQL_PROVIDER_MAP = + ImmutableMap.of( + JDBCBackendType.MYSQL, IDP_USER_META_MYSQL_PROVIDER, + JDBCBackendType.H2, IDP_USER_META_H2_PROVIDER, + JDBCBackendType.POSTGRESQL, IDP_USER_META_POSTGRESQL_PROVIDER); + + public static IdpUserMetaBaseSQLProvider getProvider() { + String databaseId = + SqlSessionFactoryHelper.getInstance() + .getSqlSessionFactory() + .getConfiguration() + .getDatabaseId(); + + return getProvider(databaseId); + } + + static IdpUserMetaBaseSQLProvider getProvider(String databaseId) { + if (databaseId == null) { + throw new IllegalStateException( + "MyBatis databaseId is not configured for IdP user SQL providers."); + } + + JDBCBackendType jdbcBackendType; + try { + jdbcBackendType = JDBCBackendType.fromString(databaseId); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + String.format( + "Unsupported IdP user SQL provider databaseId: %s, supported backends: %s", + databaseId, IDP_USER_META_SQL_PROVIDER_MAP.keySet()), + e); + } + + return getProvider(jdbcBackendType, databaseId, IDP_USER_META_SQL_PROVIDER_MAP); + } + + static IdpUserMetaBaseSQLProvider getProvider( + JDBCBackendType jdbcBackendType, + String databaseId, + Map<JDBCBackendType, IdpUserMetaBaseSQLProvider> providerMap) { + IdpUserMetaBaseSQLProvider provider = providerMap.get(jdbcBackendType); + if (provider == null) { + throw new IllegalStateException( + String.format( + "No IdP user SQL provider registered for backend %s (databaseId: %s)", + jdbcBackendType, databaseId)); + } + + return provider; + } + + public static IdpUserMetaBaseSQLProvider h2Provider() { + return IDP_USER_META_H2_PROVIDER; + } + + public static IdpUserMetaBaseSQLProvider mysqlProvider() { + return IDP_USER_META_MYSQL_PROVIDER; + } + + public static IdpUserMetaBaseSQLProvider postgresqlProvider() { + return IDP_USER_META_POSTGRESQL_PROVIDER; + } + + public static String selectIdpUser(@Param("username") String username) { + return getProvider().selectIdpUser(username); + } + + public static String selectIdpUsers(@Param("usernames") List<String> usernames) { + return getProvider().selectIdpUsers(usernames); + } + + public static String insertIdpUser(@Param("userMeta") IdpUserPO userPO) { + return getProvider().insertIdpUser(userPO); + } + + public static String updateIdpUserPassword( + @Param("userId") Long userId, @Param("passwordHash") String passwordHash) { + return getProvider().updateIdpUserPassword(userId, passwordHash); + } + + public static String softDeleteIdpUser(@Param("userId") Long userId) { + return getProvider().softDeleteIdpUser(userId); + } + + public static String deleteIdpUserMetasByLegacyTimeline( + @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { + return getProvider().deleteIdpUserMetasByLegacyTimeline(legacyTimeline, limit); + } + + static class IdpUserMetaH2Provider extends IdpUserMetaBaseSQLProvider { + + @Override + protected String currentTimeMillisExpression() { + return "DATEDIFF('MILLISECOND', TIMESTAMP '1970-01-01 00:00:00', CURRENT_TIMESTAMP())"; Review Comment: `CURRENT_TIMESTAMP()` in H2 is session-time-zone dependent, while the MySQL provider uses `UNIX_TIMESTAMP() * 1000.0` and the PostgreSQL provider uses `EXTRACT(EPOCH FROM CURRENT_TIMESTAMP)`, both of which yield UTC epoch milliseconds. As a result, `deleted_at` values stored via the H2 provider can differ from the other backends by the local timezone offset, which will cause subtle inconsistencies for `deleteIdpUserMetasByLegacyTimeline` comparisons against externally produced epoch timestamps. Consider using a UTC-anchored expression such as `DATEDIFF('MILLISECOND', TIMESTAMP '1970-01-01 00:00:00', CURRENT_TIMESTAMP(9) AT TIME ZONE 'UTC')` or simply `UNIX_MILLIS(CURRENT_TIMESTAMP())` (H2 2.x) to align semantics across backends. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/provider/base/IdpUserMetaBaseSQLProvider.java: ########## @@ -0,0 +1,100 @@ +/* + * 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.basic.storage.relational.mapper.provider.base; + +import java.util.List; +import org.apache.gravitino.idp.basic.storage.relational.mapper.IdpUserMetaMapper; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.ibatis.annotations.Param; + +public abstract 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) { + if (usernames == null || usernames.isEmpty()) { + 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 deleted_at = 0 AND 1 = 0"; + } else { + 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 " + + "<foreach collection='usernames' item='username'" + + " open='AND user_name IN (' separator=',' close=')'>" + + "#{username}" + + "</foreach>" + + "</script>"; + } Review Comment: Branching on the Java-side list to produce two different SQL strings duplicates the entire SELECT clause and bypasses MyBatis' dynamic SQL. Consider using a single `<script>` template with `<choose><when test="usernames == null or usernames.size() == 0">AND 1 = 0</when><otherwise><foreach .../></otherwise></choose>`, which removes the duplication and makes the empty/null path easier to keep in sync with the non-empty path (e.g., if you ever add filters such as version constraints). ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/AbstractIdpUserMetaStorageTest.java: ########## @@ -0,0 +1,213 @@ +/* + * 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.basic.storage.relational.mapper; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.DatabaseMetaData; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.Comparator; +import java.util.stream.Stream; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.gravitino.integration.test.container.ContainerSuite; +import org.apache.gravitino.integration.test.util.BaseIT; +import org.apache.gravitino.storage.relational.JDBCBackend; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.session.SqlSession; +import org.junit.jupiter.api.AfterEach; + +abstract class AbstractIdpUserMetaStorageTest { + protected String backendType; + protected JDBCBackend backend; + protected SqlSession sharedSession; + protected IdpUserMetaMapper idpUserMetaMapper; + + private Config config; + private Path h2Path; + + static Stream<String> storageProvider() { + return Stream.of("h2", "mysql", "postgresql"); + } + + @AfterEach + void closeSuite() throws IOException { + closeSession(); + if (backend != null) { + backend.close(); + backend = null; + } + + SqlSessionFactoryHelper.getInstance().close(); + ContainerSuite.getInstance().close(); Review Comment: `ContainerSuite.getInstance().close()` is invoked after every test method, which means the MySQL/PostgreSQL Testcontainers will be torn down and re-started for each `@ParameterizedTest` invocation. With three parameter values and several tests this multiplies container start-up cost significantly. Consider moving container teardown to `@AfterAll` (and only resetting/cleaning tables between tests), or relying on `ContainerSuite`'s normal lifecycle so containers are reused across tests in the suite. -- 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]
