Copilot commented on code in PR #11066: URL: https://github.com/apache/gravitino/pull/11066#discussion_r3239484146
########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaMapper.java: ########## @@ -0,0 +1,61 @@ +/* + * 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 java.util.List; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.ibatis.annotations.DeleteProvider; +import org.apache.ibatis.annotations.InsertProvider; +import org.apache.ibatis.annotations.Param; +import org.apache.ibatis.annotations.SelectProvider; +import org.apache.ibatis.annotations.UpdateProvider; + +/** + * A MyBatis mapper for built-in IdP user metadata operations. + * + * <p>This interface defines the SQL statements MyBatis executes for the built-in IdP user metadata + * store. The SQLs can be provided by XML files or annotations on this mapper interface. See the <a + * href="https://mybatis.org/mybatis-3/getting-started.html";>MyBatis getting started guide</a>. Review Comment: The Javadoc claims SQL may be provided via XML or annotations, but this mapper exclusively uses `*Provider` annotations dispatching to `IdpUserMetaSQLProviderFactory`. Update the comment to reflect the actual mechanism to avoid misleading future readers. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/BackendTestExtension.java: ########## @@ -0,0 +1,174 @@ +/* + * 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; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.SQLException; +import java.util.Comparator; +import java.util.List; +import java.util.stream.Stream; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.integration.test.util.BaseIT; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.junit.jupiter.api.extension.AfterEachCallback; +import org.junit.jupiter.api.extension.BeforeEachCallback; +import org.junit.jupiter.api.extension.Extension; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.jupiter.api.extension.TestTemplateInvocationContext; +import org.junit.jupiter.api.extension.TestTemplateInvocationContextProvider; + +class BackendTestExtension implements TestTemplateInvocationContextProvider { + private static final String DOCKER_TEST_FLAG = "dockerTest"; + + @Override + public boolean supportsTestTemplate(ExtensionContext context) { + return true; + } + + @Override + public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts( + ExtensionContext context) { + BackendTypes backendTypes = context.getRequiredTestClass().getAnnotation(BackendTypes.class); + List<String> backends = + backendTypes != null + ? List.of(backendTypes.value()) + : (isDockerTestEnabled() ? List.of("h2", "mysql", "postgresql") : List.of("h2")); + return backends.stream().map(BackendInvocationContext::new); + } + + static boolean isDockerTestEnabled() { + String dockerTestProperty = System.getProperty(DOCKER_TEST_FLAG); + if (dockerTestProperty != null) { + return Boolean.parseBoolean(dockerTestProperty); + } + + return Boolean.parseBoolean(System.getenv(DOCKER_TEST_FLAG)); + } + + private static class BackendInvocationContext implements TestTemplateInvocationContext { + private final String backendType; + + private BackendInvocationContext(String backendType) { + this.backendType = backendType; + } + + @Override + public String getDisplayName(int invocationIndex) { + return String.format("[%s Backend]", backendType.toUpperCase()); + } + + @Override + public List<Extension> getAdditionalExtensions() { + return List.of(new BackendSetupCallback(backendType)); + } + } + + private static class BackendSetupCallback implements BeforeEachCallback, AfterEachCallback { + private final BaseIT baseIT = new BaseIT(); + private final String backendType; + + private JDBCBackend backend; + private Path h2Path; + + private BackendSetupCallback(String backendType) { + this.backendType = backendType; + } + + @Override + public void beforeEach(ExtensionContext context) throws Exception { + backend = startBackend(); + Object testInstance = context.getRequiredTestInstance(); + if (testInstance instanceof TestJDBCBackend) { + ((TestJDBCBackend) testInstance).setBackendType(backendType); + ((TestJDBCBackend) testInstance).setBackend(backend); + } + } + + @Override + public void afterEach(ExtensionContext context) throws Exception { + SqlSessionFactoryHelper.getInstance().close(); Review Comment: `SqlSessionFactoryHelper` is a process-wide singleton. Closing it in `afterEach` means that if any test class runs tests in parallel or if multiple `BackendSetupCallback` invocations overlap (e.g., from nested `@TestTemplate` invocations), one test's teardown will close the factory still in use by another. Consider either disabling parallel execution for these IT classes explicitly, or reference-counting/owning the helper lifecycle in a single place (e.g., `@BeforeAll`/`@AfterAll`) rather than per test. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaSQLProviderFactory.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.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.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 IdpUserMetaBaseSQLProvider(), + JDBCBackendType.POSTGRESQL, new IdpUserMetaPostgreSQLProvider()); + + public static IdpUserMetaBaseSQLProvider getProvider() { + String databaseId = + SqlSessionFactoryHelper.getInstance() + .getSqlSessionFactory() + .getConfiguration() + .getDatabaseId(); + + return getProvider(databaseId); + } Review Comment: `Configuration#getDatabaseId()` returns the value populated by a configured `DatabaseIdProvider`; if none is configured it will be `null`, and `JDBCBackendType.fromString(null)` will not match any backend, causing `getProvider()` to throw `IllegalStateException` for every call at runtime. Other mapper SQL provider factories in this project typically resolve the backend type from the JDBC URL/driver configured for the entity store rather than from the MyBatis databaseId. Please confirm the SQL session factory is being initialized with a `DatabaseIdProvider` that yields `mysql`/`h2`/`postgresql`, or switch to the URL/driver-based lookup used elsewhere. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/it/IdpUserMetaMapperTest.java: ########## @@ -0,0 +1,125 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertIterableEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Comparator; +import java.util.List; +import org.apache.gravitino.storage.relational.po.IdpUserPO; + +interface IdpUserMetaMapperTest { + + default void testInsertIdpUserAndSelectIdpUser() { + IdpMapperTestBase testBase = testBase(); + IdpUserPO firstUser = testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + + assertEquals(firstUser, testBase.idpUserMetaMapper.selectIdpUser("alice")); + assertNull(testBase.idpUserMetaMapper.selectIdpUser("unknown")); + } + + default void testSelectIdpUsers() { + IdpMapperTestBase testBase = testBase(); + IdpUserPO firstUser = testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + IdpUserPO secondUser = testBase.insertUser(2L, "bob", "hash-b", 1L, 0L, 0L); + + List<IdpUserPO> users = testBase.idpUserMetaMapper.selectIdpUsers(List.of("bob", "alice")); + users.sort(Comparator.comparing(IdpUserPO::getUserId)); + assertIterableEquals(List.of(firstUser, secondUser), users); + assertTrue(testBase.idpUserMetaMapper.selectIdpUsers(List.of()).isEmpty()); + assertTrue(testBase.idpUserMetaMapper.selectIdpUsers(null).isEmpty()); + } + + default void testSelectIdpUsersIgnoresDeletedUsers() { + IdpMapperTestBase testBase = testBase(); + IdpUserPO activeUser = testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + testBase.insertUser(2L, "bob", "hash-b", 1L, 0L, 10L); + + assertIterableEquals( + List.of(activeUser), testBase.idpUserMetaMapper.selectIdpUsers(List.of("alice", "bob"))); + assertNull(testBase.idpUserMetaMapper.selectIdpUser("bob")); + } + + default void testUpdateIdpUserPassword() { + IdpMapperTestBase testBase = testBase(); + testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + + assertEquals(1, testBase.idpUserMetaMapper.updateIdpUserPassword(1L, "hash-a-2")); + assertEquals("hash-a-2", testBase.idpUserMetaMapper.selectIdpUser("alice").getPasswordHash()); + assertEquals(1L, testBase.idpUserMetaMapper.selectIdpUser("alice").getCurrentVersion()); + assertEquals(0L, testBase.idpUserMetaMapper.selectIdpUser("alice").getLastVersion()); + } + + default void testUpdateIdpUserPasswordKeepsVersionsUnchanged() { + IdpMapperTestBase testBase = testBase(); + testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + + assertEquals(1, testBase.idpUserMetaMapper.updateIdpUserPassword(1L, "hash-a-2")); + assertEquals("hash-a-2", testBase.idpUserMetaMapper.selectIdpUser("alice").getPasswordHash()); + assertEquals(1L, testBase.idpUserMetaMapper.selectIdpUser("alice").getCurrentVersion()); + assertEquals(0L, testBase.idpUserMetaMapper.selectIdpUser("alice").getLastVersion()); Review Comment: `testUpdateIdpUserPasswordKeepsVersionsUnchanged` is identical to `testUpdateIdpUserPassword` above and adds no extra coverage. To actually verify that versions are kept unchanged, insert the user with non-default `currentVersion`/`lastVersion` (e.g., 3L and 2L) and assert those exact values are returned after the update. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/it/BackendTypes.java: ########## @@ -0,0 +1,31 @@ +/* + * 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; Review Comment: The file is located in `.../mapper/it/` but declares package `org.apache.gravitino.storage.relational`. Java requires the directory layout to match the package, so this source file will fail to compile. Either move the file to `.../storage/relational/BackendTypes.java`, or change the package declaration (and the imports in the test classes that reference it) to match the actual location. The same path-vs-package mismatch exists for the other files added under the `mapper/it/` directory (`TestIdpUserMetaMapperH2/MySQL/PostgreSQL.java` and `IdpUserMetaMapperTest.java`), all of which declare packages that do not include `.mapper.it`. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.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; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; +import org.apache.gravitino.integration.test.util.CloseContainerExtension; +import org.apache.gravitino.integration.test.util.PrintFuncNameExtension; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.session.SqlSession; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.api.extension.ExtendWith; + +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +@ExtendWith({ + BackendTestExtension.class, + PrintFuncNameExtension.class, + CloseContainerExtension.class +}) +public abstract class TestJDBCBackend { + protected String backendType; + protected JDBCBackend backend; + + public void setBackendType(String backendType) { + this.backendType = backendType; + } + + public void setBackend(JDBCBackend backend) { + this.backend = backend; + } + + @BeforeEach + public void init() throws SQLException { + truncateAllTables(); + } + + protected long queryLongValue(String table, String column, String idColumn, long idValue) { + try (SqlSession sqlSession = + SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true)) { + try (Connection connection = sqlSession.getConnection()) { + String query = "SELECT " + column + " FROM " + table + " WHERE " + idColumn + " = ?"; + try (PreparedStatement statement = connection.prepareStatement(query)) { + statement.setLong(1, idValue); + try (ResultSet resultSet = statement.executeQuery()) { + assertTrue(resultSet.next()); + return resultSet.getLong(1); + } + } + } + } catch (SQLException e) { + throw new RuntimeException("Query " + column + " from " + table + " failed", e); + } + } + + protected int countRows(String table, String idColumn, long idValue) { + try (SqlSession sqlSession = + SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true)) { + try (Connection connection = sqlSession.getConnection()) { + String query = "SELECT COUNT(1) FROM " + table + " WHERE " + idColumn + " = ?"; + try (PreparedStatement statement = connection.prepareStatement(query)) { + statement.setLong(1, idValue); + try (ResultSet resultSet = statement.executeQuery()) { + assertTrue(resultSet.next()); + return resultSet.getInt(1); + } + } + } + } catch (SQLException e) { + throw new RuntimeException("Count rows from " + table + " failed", e); + } + } + + private void truncateAllTables() throws SQLException { + try (SqlSession sqlSession = + SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true)) { + try (Connection connection = sqlSession.getConnection(); + Statement statement = connection.createStatement()) { + if ("postgresql".equalsIgnoreCase(backendType)) { + truncateAllTablesForPostgreSQL(connection); + } else { + List<String> tableList = new ArrayList<>(); + try (ResultSet rs = statement.executeQuery("SHOW TABLES")) { + while (rs.next()) { + tableList.add(rs.getString(1)); + } + } + for (String table : tableList) { + statement.execute("TRUNCATE TABLE " + table); + } + } + } + } + } + + private void truncateAllTablesForPostgreSQL(Connection connection) throws SQLException { + List<String> tableList = new ArrayList<>(); + try (Statement statement = connection.createStatement()) { + String query = + "SELECT table_name FROM information_schema.tables WHERE table_schema = current_schema()"; + try (ResultSet rs = statement.executeQuery(query)) { + while (rs.next()) { + tableList.add(rs.getString(1)); + } + } + + if (tableList.isEmpty()) { + return; + } + + StringBuilder pgTruncateCommand = new StringBuilder("DO $$ BEGIN\n"); + for (String table : tableList) { + pgTruncateCommand.append( + String.format("TRUNCATE TABLE %s RESTART IDENTITY CASCADE;", table)); + } + pgTruncateCommand.append("END $$;"); + statement.execute(pgTruncateCommand.toString()); Review Comment: Statements inside a PL/pgSQL `DO $$ BEGIN ... END $$` block must be valid PL/pgSQL, which requires each statement to end with a semicolon as you have, but plain `TRUNCATE TABLE` is allowed only as a SQL statement and works here only because PL/pgSQL accepts it. More importantly, building a `DO` block per truncation is unnecessary and obscures errors; a single `TRUNCATE TABLE t1, t2, ... RESTART IDENTITY CASCADE` statement executed directly is simpler and produces clearer errors if a table name is invalid. Consider replacing the `DO $$` wrapper with a single comma-separated `TRUNCATE`. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/TestIdpUserMetaSQLProviderFactoryFailure.java: ########## @@ -0,0 +1,38 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +public class TestIdpUserMetaSQLProviderFactoryFailure { + + @Test + void testGetProviderThrowsForUnsupportedDatabaseId() { + IllegalStateException exception = + assertThrows( + IllegalStateException.class, () -> IdpUserMetaSQLProviderFactory.getProvider("sqlite")); + + assertTrue(exception.getMessage().contains("sqlite")); + assertTrue(exception.getMessage().contains("supported backends")); Review Comment: This test covers the unknown-database-id path, but `getProvider(String)` also has a second `IllegalStateException` branch when `JDBCBackendType.fromString` returns a value not present in `IDP_USER_META_SQL_PROVIDER_MAP`. That branch (lines 63-68) is currently unreachable given the map content but is fragile — consider either removing the dead branch or adding a test that exercises it (e.g., temporarily removing an entry via a test-only seam). -- 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]
