Copilot commented on code in PR #11066: URL: https://github.com/apache/gravitino/pull/11066#discussion_r3240937992
########## 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) { + 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 " + + "<choose>" + + "<when test='usernames != null and usernames.size() > 0'>" + + "AND user_name IN (" + + "<foreach item='item' collection='usernames' separator=','>" + + "#{item}" + + "</foreach>" + + ") " + + "</when>" + + "<otherwise>" + + "AND 1 = 0 " + + "</otherwise>" + + "</choose>" + + "</script>"; + } + + public String insertIdpUser(@Param("userMeta") IdpUserPO userPO) { + return "INSERT INTO " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " (user_id, user_name, password_hash, current_version, last_version, deleted_at)" + + " VALUES (" + + " #{userMeta.userId}," + + " #{userMeta.userName}," + + " #{userMeta.passwordHash}," + + " #{userMeta.currentVersion}," + + " #{userMeta.lastVersion}," + + " #{userMeta.deletedAt}" + + " )"; + } + + public String updateIdpUserPassword( + @Param("userId") Long userId, @Param("passwordHash") String passwordHash) { + return "UPDATE " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " SET password_hash = #{passwordHash}" + + " WHERE user_id = #{userId}" + + " AND deleted_at = 0"; + } + + public String softDeleteIdpUser(@Param("userId") Long userId) { + return "UPDATE " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " SET deleted_at = " + + currentTimeMillisExpression() + + " WHERE user_id = #{userId} AND deleted_at = 0"; + } + + public String deleteIdpUserMetasByLegacyTimeline( + @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { + return "DELETE FROM " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT #{limit}"; + } Review Comment: The base implementation uses `DELETE ... LIMIT`, which is valid in MySQL/H2 but not in standard SQL nor in PostgreSQL. The PostgreSQL subclass correctly overrides this, but if any other backend (or a future one) inherits from the base class without overriding, the SQL will fail at runtime on PostgreSQL-compatible engines. Consider making `deleteIdpUserMetasByLegacyTimeline` abstract (or moving the MySQL/H2 form into those subclasses) so each backend is forced to provide a syntactically valid statement, mirroring how `currentTimeMillisExpression()` is handled. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/BackendTestExtension.java: ########## @@ -0,0 +1,246 @@ +/* + * 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; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +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.JDBCBackend; +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +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; + +public class BackendTestExtension + implements TestTemplateInvocationContextProvider, BeforeAllCallback, AfterAllCallback { + + private static final String DOCKER_TEST_FLAG = "dockerTest"; + private static final ExtensionContext.Namespace NAMESPACE = + ExtensionContext.Namespace.create(BackendTestExtension.class); + private static final String STORE_KEY = "BACKEND_MAP"; + + @Override + public void beforeAll(ExtensionContext context) { + context.getStore(NAMESPACE).put(STORE_KEY, new ConcurrentHashMap<String, BackendResource>()); + } + + @Override + @SuppressWarnings("unchecked") + public void afterAll(ExtensionContext context) throws Exception { + ConcurrentHashMap<String, BackendResource> map = + (ConcurrentHashMap<String, BackendResource>) context.getStore(NAMESPACE).get(STORE_KEY); + if (map != null) { + for (BackendResource backendResource : map.values()) { + backendResource.close(); + } + map.clear(); + } + } + + @Override + public boolean supportsTestTemplate(ExtensionContext context) { + return TestJDBCBackend.class.isAssignableFrom(context.getRequiredTestClass()); + } + + @Override + public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts( + ExtensionContext context) { + return resolveBackends(context.getRequiredTestClass()).stream() + .map(BackendInvocationContext::new); + } + + private List<String> resolveBackends(Class<?> testClass) { + BackendTypes backendTypes = findBackendTypes(testClass); + if (backendTypes != null) { + return List.of(backendTypes.value()); + } + + List<String> backendsToTest = new ArrayList<>(); + backendsToTest.add("h2"); + if ("true".equalsIgnoreCase(System.getenv(DOCKER_TEST_FLAG))) { + backendsToTest.add("mysql"); + backendsToTest.add("postgresql"); + } + return backendsToTest; + } + + private BackendTypes findBackendTypes(Class<?> testClass) { + Class<?> current = testClass; + while (current != null) { + BackendTypes backendTypes = current.getDeclaredAnnotation(BackendTypes.class); + if (backendTypes != null) { + return backendTypes; + } + current = current.getSuperclass(); + } + return null; + } + + 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 Collections.singletonList(new BackendSetupCallback(backendType)); + } + } + + private static class BackendSetupCallback implements BeforeEachCallback { + private final String backendType; + + private BackendSetupCallback(String backendType) { + this.backendType = backendType; + } + + @Override + public void beforeEach(ExtensionContext context) throws Exception { + BackendResource backendResource = getOrCreateBackendResource(context, backendType); + Object testInstance = context.getRequiredTestInstance(); + if (testInstance instanceof TestJDBCBackend) { + ((TestJDBCBackend) testInstance).setBackend(backendResource.backend()); + ((TestJDBCBackend) testInstance).setBackendType(backendType); + } + } + } + + @SuppressWarnings("unchecked") + private static Map<String, BackendResource> getBackendResources(ExtensionContext context) { + return (Map<String, BackendResource>) context.getStore(NAMESPACE).get(STORE_KEY, Map.class); + } Review Comment: `getBackendResources` reads from `context.getStore(NAMESPACE)`, but the map is stored in `beforeAll` against the class-level context. When invoked from `BackendSetupCallback.beforeEach` (which runs in a method-level context), `getStore(NAMESPACE).get(STORE_KEY, ...)` will only find the value if the store lookup walks up to the parent context. JUnit's `ExtensionContext.Store` does perform parent lookup, but the value is stored on the class context's store from `beforeAll(ExtensionContext context)` where `context` may be the class context — please verify; if `BackendTestExtension` is registered at the method level by `@ExtendWith` on the abstract class, `beforeAll` receives the class context and storage is correct, but `afterAll` clearing the map will leave subclasses sharing state across classes if the extension is reused. Recommend explicitly using `context.getRoot().getStore(...)` (or the class-level context retrieved via `context.getParent()`) consistently in both `beforeAll`/ `afterAll` and `getBackendResources` to avoid lookup misses. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/BackendTestExtension.java: ########## @@ -0,0 +1,246 @@ +/* + * 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; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +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.JDBCBackend; +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +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; + +public class BackendTestExtension + implements TestTemplateInvocationContextProvider, BeforeAllCallback, AfterAllCallback { + + private static final String DOCKER_TEST_FLAG = "dockerTest"; + private static final ExtensionContext.Namespace NAMESPACE = + ExtensionContext.Namespace.create(BackendTestExtension.class); + private static final String STORE_KEY = "BACKEND_MAP"; + + @Override + public void beforeAll(ExtensionContext context) { + context.getStore(NAMESPACE).put(STORE_KEY, new ConcurrentHashMap<String, BackendResource>()); + } + + @Override + @SuppressWarnings("unchecked") + public void afterAll(ExtensionContext context) throws Exception { + ConcurrentHashMap<String, BackendResource> map = + (ConcurrentHashMap<String, BackendResource>) context.getStore(NAMESPACE).get(STORE_KEY); + if (map != null) { + for (BackendResource backendResource : map.values()) { + backendResource.close(); + } + map.clear(); + } + } + + @Override + public boolean supportsTestTemplate(ExtensionContext context) { + return TestJDBCBackend.class.isAssignableFrom(context.getRequiredTestClass()); + } + + @Override + public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts( + ExtensionContext context) { + return resolveBackends(context.getRequiredTestClass()).stream() + .map(BackendInvocationContext::new); + } + + private List<String> resolveBackends(Class<?> testClass) { + BackendTypes backendTypes = findBackendTypes(testClass); + if (backendTypes != null) { + return List.of(backendTypes.value()); + } + + List<String> backendsToTest = new ArrayList<>(); + backendsToTest.add("h2"); + if ("true".equalsIgnoreCase(System.getenv(DOCKER_TEST_FLAG))) { + backendsToTest.add("mysql"); + backendsToTest.add("postgresql"); + } + return backendsToTest; + } + + private BackendTypes findBackendTypes(Class<?> testClass) { + Class<?> current = testClass; + while (current != null) { + BackendTypes backendTypes = current.getDeclaredAnnotation(BackendTypes.class); + if (backendTypes != null) { + return backendTypes; + } + current = current.getSuperclass(); + } + return null; + } + + 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 Collections.singletonList(new BackendSetupCallback(backendType)); + } + } + + private static class BackendSetupCallback implements BeforeEachCallback { + private final String backendType; + + private BackendSetupCallback(String backendType) { + this.backendType = backendType; + } + + @Override + public void beforeEach(ExtensionContext context) throws Exception { + BackendResource backendResource = getOrCreateBackendResource(context, backendType); + Object testInstance = context.getRequiredTestInstance(); + if (testInstance instanceof TestJDBCBackend) { + ((TestJDBCBackend) testInstance).setBackend(backendResource.backend()); + ((TestJDBCBackend) testInstance).setBackendType(backendType); + } + } + } + + @SuppressWarnings("unchecked") + private static Map<String, BackendResource> getBackendResources(ExtensionContext context) { + return (Map<String, BackendResource>) context.getStore(NAMESPACE).get(STORE_KEY, Map.class); + } + + private static BackendResource getOrCreateBackendResource( + ExtensionContext context, String backendType) throws SQLException { + Map<String, BackendResource> backendResources = getBackendResources(context); + synchronized (backendResources) { + BackendResource backendResource = backendResources.get(backendType); + if (backendResource != null) { + return backendResource; + } + + backendResource = createBackendResource(backendType); + backendResources.put(backendType, backendResource); + return backendResource; + } + } + + private static BackendResource createBackendResource(String backendType) throws SQLException { + BaseIT baseIT = new BaseIT(); + Config config = new Config(false) {}; + Path h2Path = null; + config.set(Configs.ENTITY_STORE, Configs.RELATIONAL_ENTITY_STORE); + config.set(Configs.ENTITY_RELATIONAL_STORE, backendType); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS, 20); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_WAIT_MILLISECONDS, 1000L); + + if ("mysql".equals(backendType)) { + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL, baseIT.startAndInitMySQLBackend()); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER, "root"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD, "root"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER, "com.mysql.cj.jdbc.Driver"); + } else if ("postgresql".equals(backendType)) { + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL, baseIT.startAndInitPGBackend()); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER, "root"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD, "root"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER, "org.postgresql.Driver"); + } else { + try { + h2Path = Files.createTempDirectory("gravitino_jdbc_test_h2_"); + } catch (IOException e) { + throw new RuntimeException("Create H2 test directory failed", e); + } + + config.set( + Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL, + String.format("jdbc:h2:file:%s;DB_CLOSE_DELAY=-1;MODE=MYSQL", h2Path)); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER, "root"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD, "123456"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER, "org.h2.Driver"); + } + + JDBCBackend jdbcBackend = new JDBCBackend(); + try { + // Close any leftover shared SQL session state before initializing the next backend. + jdbcBackend.close(); + } catch (IOException e) { + throw new RuntimeException("Close JDBC backend before initialization failed", e); + } Review Comment: Calling `close()` on a freshly constructed `JDBCBackend` before `initialize` is confusing and likely relies on internal side effects of `close()` cleaning up the global `SqlSessionFactoryHelper` singleton. This couples the test extension to an implementation detail and will silently break if `close()` ever requires an initialized backend. Consider exposing a dedicated reset method on `SqlSessionFactoryHelper` (or `JDBCBackend`) for tests, or at minimum document why `close()` is called pre-initialization. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/TestJDBCBackend.java: ########## @@ -0,0 +1,164 @@ +/* + * 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; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.DatabaseMetaData; +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.JDBCBackend; +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); + } + } + + protected String currentJdbcUrl() { + try (SqlSession sqlSession = + SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true)) { + try (Connection connection = sqlSession.getConnection()) { + DatabaseMetaData metaData = connection.getMetaData(); + return metaData.getURL(); + } + } catch (SQLException e) { + throw new RuntimeException("Get current JDBC URL 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); Review Comment: `SHOW TABLES` is supported by MySQL and H2 but is not portable SQL. If a future backend is added without updating this branch, truncation will fail with a cryptic SQL error. Either use `DatabaseMetaData.getTables(...)` for portability, or branch explicitly per `backendType` and throw an `UnsupportedOperationException` for unknown values. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/TestIdpUserMetaSQLProviderFactoryFailure.java: ########## @@ -0,0 +1,63 @@ +/* + * 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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.common.collect.ImmutableMap; +import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType; +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")); + } + + @Test + void testGetProviderThrowsForMissingDatabaseId() { + IllegalStateException exception = + assertThrows( + IllegalStateException.class, () -> IdpUserMetaSQLProviderFactory.getProvider(null)); + + assertTrue(exception.getMessage().contains("databaseId")); + assertTrue(exception.getMessage().contains("not configured")); + } + + @Test + void testGetProviderThrowsWhenResolvedBackendHasNoProvider() { + IllegalStateException exception = + assertThrows( + IllegalStateException.class, + () -> + IdpUserMetaSQLProviderFactory.getProvider( + JDBCBackendType.H2, "h2", ImmutableMap.of())); Review Comment: This test exercises the package-private 3-arg `getProvider` overload with an empty provider map, but `IdpUserMetaSQLProviderFactory.getProvider(String)` always passes the static `IDP_USER_META_SQL_PROVIDER_MAP`, so the "no provider registered for a recognized backend" branch is unreachable in production code. Either remove the unreachable branch (and this test) or add a test exercising the public path; otherwise the code path is dead and the test only validates internal plumbing. ########## 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) { + 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 " + + "<choose>" + + "<when test='usernames != null and usernames.size() > 0'>" + + "AND user_name IN (" + + "<foreach item='item' collection='usernames' separator=','>" + + "#{item}" + + "</foreach>" + + ") " + + "</when>" + + "<otherwise>" + + "AND 1 = 0 " + + "</otherwise>" + + "</choose>" + + "</script>"; Review Comment: Mixing `<script>`-style XML SQL with concatenated static SQL strings for other methods makes maintenance harder and is inconsistent. Since `selectIdpUsers` already requires dynamic SQL, consider using `@SelectProvider` with a `ProviderMethodResolver`/`SQL`-builder approach, or switch the entire mapper to XML for consistency. At minimum, document the rationale for using `<script>` only here. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/BackendTestExtension.java: ########## @@ -0,0 +1,246 @@ +/* + * 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; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +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.JDBCBackend; +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +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; + +public class BackendTestExtension + implements TestTemplateInvocationContextProvider, BeforeAllCallback, AfterAllCallback { + + private static final String DOCKER_TEST_FLAG = "dockerTest"; + private static final ExtensionContext.Namespace NAMESPACE = + ExtensionContext.Namespace.create(BackendTestExtension.class); + private static final String STORE_KEY = "BACKEND_MAP"; + + @Override + public void beforeAll(ExtensionContext context) { + context.getStore(NAMESPACE).put(STORE_KEY, new ConcurrentHashMap<String, BackendResource>()); + } + + @Override + @SuppressWarnings("unchecked") + public void afterAll(ExtensionContext context) throws Exception { + ConcurrentHashMap<String, BackendResource> map = + (ConcurrentHashMap<String, BackendResource>) context.getStore(NAMESPACE).get(STORE_KEY); + if (map != null) { + for (BackendResource backendResource : map.values()) { + backendResource.close(); + } + map.clear(); + } + } + + @Override + public boolean supportsTestTemplate(ExtensionContext context) { + return TestJDBCBackend.class.isAssignableFrom(context.getRequiredTestClass()); + } + + @Override + public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts( + ExtensionContext context) { + return resolveBackends(context.getRequiredTestClass()).stream() + .map(BackendInvocationContext::new); + } + + private List<String> resolveBackends(Class<?> testClass) { + BackendTypes backendTypes = findBackendTypes(testClass); + if (backendTypes != null) { + return List.of(backendTypes.value()); + } + + List<String> backendsToTest = new ArrayList<>(); + backendsToTest.add("h2"); + if ("true".equalsIgnoreCase(System.getenv(DOCKER_TEST_FLAG))) { + backendsToTest.add("mysql"); + backendsToTest.add("postgresql"); + } + return backendsToTest; + } + + private BackendTypes findBackendTypes(Class<?> testClass) { + Class<?> current = testClass; + while (current != null) { + BackendTypes backendTypes = current.getDeclaredAnnotation(BackendTypes.class); + if (backendTypes != null) { + return backendTypes; + } + current = current.getSuperclass(); + } + return null; + } + + 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 Collections.singletonList(new BackendSetupCallback(backendType)); + } + } + + private static class BackendSetupCallback implements BeforeEachCallback { + private final String backendType; + + private BackendSetupCallback(String backendType) { + this.backendType = backendType; + } + + @Override + public void beforeEach(ExtensionContext context) throws Exception { + BackendResource backendResource = getOrCreateBackendResource(context, backendType); + Object testInstance = context.getRequiredTestInstance(); + if (testInstance instanceof TestJDBCBackend) { + ((TestJDBCBackend) testInstance).setBackend(backendResource.backend()); + ((TestJDBCBackend) testInstance).setBackendType(backendType); + } + } + } + + @SuppressWarnings("unchecked") + private static Map<String, BackendResource> getBackendResources(ExtensionContext context) { + return (Map<String, BackendResource>) context.getStore(NAMESPACE).get(STORE_KEY, Map.class); + } + + private static BackendResource getOrCreateBackendResource( + ExtensionContext context, String backendType) throws SQLException { + Map<String, BackendResource> backendResources = getBackendResources(context); + synchronized (backendResources) { + BackendResource backendResource = backendResources.get(backendType); + if (backendResource != null) { + return backendResource; + } + + backendResource = createBackendResource(backendType); + backendResources.put(backendType, backendResource); + return backendResource; + } + } + + private static BackendResource createBackendResource(String backendType) throws SQLException { + BaseIT baseIT = new BaseIT(); + Config config = new Config(false) {}; + Path h2Path = null; + config.set(Configs.ENTITY_STORE, Configs.RELATIONAL_ENTITY_STORE); + config.set(Configs.ENTITY_RELATIONAL_STORE, backendType); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS, 20); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_WAIT_MILLISECONDS, 1000L); + + if ("mysql".equals(backendType)) { + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL, baseIT.startAndInitMySQLBackend()); Review Comment: The string literals "mysql", "postgresql", and "h2" are used in multiple places (`resolveBackends`, `createBackendResource`, `truncateAllTables`). Extracting them into shared constants (or reusing `JDBCBackendType` values) would reduce risk of typos and make supported backends explicit. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,112 @@ +/* + * 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.h2.IdpUserMetaH2Provider; +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 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(); + + 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 String selectIdpUser(@Param("username") String username) { + return getProvider().selectIdpUser(username); + } Review Comment: `@Param` annotations on these static delegating methods have no effect — MyBatis only honors `@Param` on the mapper interface methods, not on `SqlProvider` methods. The parameter map MyBatis passes to providers is constructed from the mapper-method `@Param` names. Removing these annotations would prevent confusion about where parameter binding actually happens. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/TestIdpUserMetaMapperH2.java: ########## @@ -0,0 +1,77 @@ +/* + * 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 org.apache.gravitino.idp.basic.storage.relational.BackendTypes; +import org.junit.jupiter.api.TestTemplate; + +@BackendTypes({"h2"}) +public class TestIdpUserMetaMapperH2 extends IdpMapperTestBase implements IdpUserMetaMapperTest { Review Comment: The three `TestIdpUserMetaMapper{H2,MySQL,PostgreSQL}` classes are nearly identical — they only differ in the `@BackendTypes` value and `@Tag` annotation. Consider consolidating to a single test class annotated with `@BackendTypes({"h2","mysql","postgresql"})` (relying on `BackendTestExtension` to filter via the docker flag and `gravitino-docker-test` tag), which would eliminate ~200 lines of duplicated `@TestTemplate` shims. If per-backend tagging is required, a base class with the shared `@TestTemplate` methods and trivial subclasses would still cut most duplication. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/provider/postgresql/IdpUserMetaPostgreSQLProvider.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.basic.storage.relational.mapper.provider.postgresql; + +import org.apache.gravitino.idp.basic.storage.relational.mapper.IdpUserMetaMapper; +import org.apache.gravitino.idp.basic.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider; +import org.apache.ibatis.annotations.Param; + +public class IdpUserMetaPostgreSQLProvider extends IdpUserMetaBaseSQLProvider { + + @Override + protected String currentTimeMillisExpression() { + return "CAST(EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000 AS BIGINT)"; + } + + @Override + public String deleteIdpUserMetasByLegacyTimeline( + @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { Review Comment: As above, `@Param` is ineffective on `SqlProvider` methods; the binding comes from the mapper interface. Remove the annotations to avoid misleading readers. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/po/IdpUserPO.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.po; + +import com.google.common.base.Preconditions; +import lombok.AccessLevel; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.ToString; + +@Getter +@EqualsAndHashCode +@ToString +@NoArgsConstructor(access = AccessLevel.PRIVATE) +@AllArgsConstructor(access = AccessLevel.PRIVATE) +@Builder(builderClassName = "Builder", setterPrefix = "with") +public class IdpUserPO { + private Long userId; + private String userName; + private String passwordHash; + private Long currentVersion; + private Long lastVersion; + private Long deletedAt; + + public static class Builder { + private void validate() { + Preconditions.checkArgument(userId != null, "User id is required"); Review Comment: `Preconditions.checkArgument` throws `IllegalArgumentException`, but the matching test `testIdpUserPOBuilderValidation` asserts `IllegalArgumentException.class` — that's fine. However, note that defining an empty hand-written `Builder` class together with Lombok's `@Builder(builderClassName = "Builder")` relies on Lombok merging fields and `with*` setters into your declared class. This is supported but brittle: if any field is renamed or removed, the `validate()` references will break only at compile-time of this class; consider documenting the dependency or using `@Builder.Default`/Lombok's `@Builder(...)` `build` customization patterns to make the validation contract clearer. -- 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]
