Copilot commented on code in PR #11066: URL: https://github.com/apache/gravitino/pull/11066#discussion_r3246592214
########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/AbstractIdpUserMetaStorageTest.java: ########## @@ -0,0 +1,314 @@ +/* + * 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.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Arrays; +import java.util.Comparator; +import java.util.UUID; +import java.util.stream.Stream; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.config.ConfigConstants; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.gravitino.integration.test.container.ContainerSuite; +import org.apache.gravitino.integration.test.container.MySQLContainer; +import org.apache.gravitino.integration.test.container.PostgreSQLContainer; +import org.apache.gravitino.integration.test.util.TestDatabaseName; +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 { + private static final String H2_BACKEND = "h2"; + private static final String MYSQL_BACKEND = "mysql"; + private static final String POSTGRESQL_BACKEND = "postgresql"; + private static final TestDatabaseName MYSQL_TEST_DATABASE = TestDatabaseName.MYSQL_JDBC_BACKEND; + private static final TestDatabaseName POSTGRESQL_TEST_DATABASE = TestDatabaseName.PG_JDBC_BACKEND; + + protected JDBCBackend backend; + protected SqlSession sharedSession; + protected IdpUserMetaMapper idpUserMetaMapper; + + private Config config; + private Path h2Path; + + static Stream<String> storageProvider() { + return Stream.of(H2_BACKEND, MYSQL_BACKEND, POSTGRESQL_BACKEND); + } + + @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 { + config = createBackendConfig(type); + backend = new JDBCBackend(); + backend.close(); Review Comment: Calling `backend.close()` on a freshly constructed `JDBCBackend` before it has been initialized is at best a no-op and at worst will fail or leave internal state inconsistent (e.g. attempting to close a null `SqlSessionFactoryHelper`/data source). If the intent is to reset shared static state (e.g. `SqlSessionFactoryHelper`) before initializing the new backend, do so explicitly and only when needed (the `closeSuite` `@AfterEach` already handles teardown). Otherwise, remove the redundant `close()` call before `initialize(config)`. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.mapper.provider.mysql.IdpUserMetaMySQLProvider; +import org.apache.gravitino.idp.basic.storage.relational.mapper.provider.postgresql.IdpUserMetaPostgreSQLProvider; +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); + + static IdpUserMetaBaseSQLProvider getProvider( + String databaseId, Map<JDBCBackendType, IdpUserMetaBaseSQLProvider> providerMap) { + if (databaseId == null) { + throw new IllegalStateException( + "MyBatis databaseId is not configured for IdP user SQL providers."); + } + + try { + JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId); + IdpUserMetaBaseSQLProvider provider = providerMap.get(jdbcBackendType); + if (provider != null) { + return provider; + } + + throw new IllegalStateException( + String.format( + "No IdP user SQL provider registered for backend %s (databaseId: %s)", + jdbcBackendType, databaseId)); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + String.format( + "Unsupported IdP user SQL provider databaseId: %s, supported backends: %s", + databaseId, providerMap.keySet()), + e); + } + } + + 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(currentDatabaseId(), IDP_USER_META_SQL_PROVIDER_MAP).selectIdpUser(username); + } + + public static String selectIdpUsers(@Param("usernames") List<String> usernames) { Review Comment: The `@Param` annotations on these `static` SQL-provider methods have no effect — MyBatis only honors `@Param` on mapper interface method parameters. They are already declared on `IdpUserMetaMapper`. Removing them here avoids implying a binding that does not exist. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/AbstractIdpUserMetaStorageTest.java: ########## @@ -0,0 +1,314 @@ +/* + * 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.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Arrays; +import java.util.Comparator; +import java.util.UUID; +import java.util.stream.Stream; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.config.ConfigConstants; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.gravitino.integration.test.container.ContainerSuite; +import org.apache.gravitino.integration.test.container.MySQLContainer; +import org.apache.gravitino.integration.test.container.PostgreSQLContainer; +import org.apache.gravitino.integration.test.util.TestDatabaseName; +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 { + private static final String H2_BACKEND = "h2"; + private static final String MYSQL_BACKEND = "mysql"; + private static final String POSTGRESQL_BACKEND = "postgresql"; + private static final TestDatabaseName MYSQL_TEST_DATABASE = TestDatabaseName.MYSQL_JDBC_BACKEND; + private static final TestDatabaseName POSTGRESQL_TEST_DATABASE = TestDatabaseName.PG_JDBC_BACKEND; + + protected JDBCBackend backend; + protected SqlSession sharedSession; + protected IdpUserMetaMapper idpUserMetaMapper; + + private Config config; + private Path h2Path; + + static Stream<String> storageProvider() { + return Stream.of(H2_BACKEND, MYSQL_BACKEND, POSTGRESQL_BACKEND); + } + + @AfterEach + void closeSuite() throws IOException { + closeSession(); + if (backend != null) { + backend.close(); + backend = null; + } + + SqlSessionFactoryHelper.getInstance().close(); + ContainerSuite.getInstance().close(); Review Comment: `ContainerSuite.getInstance().close()` stops the shared MySQL/PostgreSQL containers after every test method. Since `storageProvider()` runs each test against three backends and there are several parameterized tests, this forces the heavy containers to start/stop many times during a single test class run, dramatically slowing the suite. Consider tearing down containers once in an `@AfterAll` hook (or relying on `ContainerSuite`'s own lifecycle), and only reset per-test state (sessions, backend, schema) in `@AfterEach`. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.mapper.provider.mysql.IdpUserMetaMySQLProvider; +import org.apache.gravitino.idp.basic.storage.relational.mapper.provider.postgresql.IdpUserMetaPostgreSQLProvider; +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); + + static IdpUserMetaBaseSQLProvider getProvider( + String databaseId, Map<JDBCBackendType, IdpUserMetaBaseSQLProvider> providerMap) { + if (databaseId == null) { + throw new IllegalStateException( + "MyBatis databaseId is not configured for IdP user SQL providers."); + } + + try { + JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId); + IdpUserMetaBaseSQLProvider provider = providerMap.get(jdbcBackendType); Review Comment: The error branches in `getProvider` (null `databaseId`, unknown `databaseId`, and the `null`-provider path) are not exercised by any test in this PR, even though other classes in this package have unit tests. Adding a small unit test for `IdpUserMetaSQLProviderFactory.getProvider` (covering valid and invalid `databaseId` values) would lock in the contract these error messages express. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.mapper.provider.mysql.IdpUserMetaMySQLProvider; +import org.apache.gravitino.idp.basic.storage.relational.mapper.provider.postgresql.IdpUserMetaPostgreSQLProvider; +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); + + static IdpUserMetaBaseSQLProvider getProvider( + String databaseId, Map<JDBCBackendType, IdpUserMetaBaseSQLProvider> providerMap) { Review Comment: `getProvider` always receives the same `IDP_USER_META_SQL_PROVIDER_MAP` from every caller in this class. Either drop the `providerMap` parameter and read the constant directly, or make the method genuinely reusable (e.g. move it to a shared utility). As written, the parameter only adds noise at every call site. ########## 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"; Review Comment: Executing a `SELECT ... WHERE 1 = 0` round-trip just to model \"no usernames requested\" still hits the database for an empty input. Consider short-circuiting at the mapper/service layer (return `Collections.emptyList()` when the input is null/empty) so no SQL is issued; the `1 = 0` fallback can be retained as a defensive last resort but should not be the primary path. -- 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]
