Copilot commented on code in PR #11066: URL: https://github.com/apache/gravitino/pull/11066#discussion_r3240697222
########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/provider/mysql/IdpUserMetaMySQLProvider.java: ########## @@ -0,0 +1,30 @@ +/* + * 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.mysql; + +import org.apache.gravitino.idp.basic.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider; + +public class IdpUserMetaMySQLProvider extends IdpUserMetaBaseSQLProvider { + + @Override + protected String currentTimeMillisExpression() { + return "(UNIX_TIMESTAMP() * 1000.0)"; Review Comment: `UNIX_TIMESTAMP() * 1000.0` returns a floating-point value, but `deleted_at` is stored/read as a `Long` (BIGINT). Other backends here cast to BIGINT (PostgreSQL) or return an integer expression. Use an integer expression such as `(UNIX_TIMESTAMP() * 1000)` (or `CAST(UNIX_TIMESTAMP(NOW(3)) * 1000 AS UNSIGNED)` for sub-second precision) to avoid potential implicit decimal-to-bigint conversion issues and inconsistent precision with other backends. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpUserMetaMapper.java: ########## @@ -0,0 +1,62 @@ +/* + * 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 java.util.List; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.ibatis.annotations.DeleteProvider; Review Comment: `selectIdpUsers` returns a `List<IdpUserPO>` of multiple columns, but no `@Results`/`@ResultMap` mapping is declared on these `@SelectProvider` methods. The SQL uses column aliases like `user_id as userId`, which works for default MyBatis auto-mapping, but only when the auto-mapping behavior is enabled and column names match the field names — please confirm that the global MyBatis configuration enables auto-mapping (default `PARTIAL`) and that the aliases exactly match the PO field names (they do, except this relies on auto-mapping). Consider adding an explicit `@Results` mapping to make the contract clear and robust against MyBatis configuration changes. ########## 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` walks up via `context.getStore(NAMESPACE)` of the current (per-test) context, but the map is `put` in `beforeAll` on the class-level context. Depending on how JUnit propagates store lookups, this typically works (stores are inherited from parent contexts for `get`), but `synchronized (backendResources)` then locks on whatever map instance is returned — if a different invocation context ever resolves to a different store, this could break the synchronization guarantee. Consider retrieving the map from the root/class context explicitly (e.g., `context.getRoot().getStore(...)` or walking to the class-level context) to make the locking contract unambiguous. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/TestJDBCBackend.java: ########## @@ -0,0 +1,158 @@ +/* + * 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")) { Review Comment: `SHOW TABLES` is MySQL/H2-specific syntax. The `else` branch will be taken for any non-postgres backend including future ones (and the variable is `backendType`, not validated). For H2 in MySQL mode and MySQL this works, but the assumption should be documented or the supported backends explicitly enumerated; otherwise adding a new backend will produce a confusing SQL syntax error 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()); + 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); + } + jdbcBackend.initialize(config); Review Comment: Calling `close()` on a freshly constructed `JDBCBackend` to clear leftover global/shared SQL session state is non-obvious and relies on internals of `JDBCBackend`/`SqlSessionFactoryHelper`. If this is necessary because the helper is a singleton, please expand the comment to explain that, or extract a helper method (e.g., `resetSharedSqlSession()`) so the intent is clear and not easily removed by a future cleanup. ########## 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 { + + @Override + public IdpMapperTestBase testBase() { + return this; + } + + @TestTemplate + public void testInsertIdpUserAndSelectIdpUser() { + IdpUserMetaMapperTest.super.testInsertIdpUserAndSelectIdpUser(); + } Review Comment: Each backend-specific test class re-declares every test method just to forward to the interface default method and apply `@TestTemplate`. This is duplicated three times (H2/MySQL/PostgreSQL) and will require updates in three places whenever a new test case is added to `IdpUserMetaMapperTest`. Consider moving these `@TestTemplate` declarations onto the interface defaults (annotations on interface default methods are inherited in JUnit Jupiter) or to `IdpMapperTestBase`, so the per-backend subclasses only configure backend types. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/IdpMapperTestBase.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.TestJDBCBackend; +import org.apache.gravitino.idp.basic.storage.relational.po.IdpUserPO; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.session.SqlSession; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; + +abstract class IdpMapperTestBase extends TestJDBCBackend { + protected SqlSession sharedSession; + protected IdpUserMetaMapper idpUserMetaMapper; Review Comment: `sharedSession` is opened in `@BeforeEach` and is used by tests via the mapper, but `TestJDBCBackend` is annotated `@TestInstance(PER_CLASS)`. With `PER_CLASS` lifecycle plus parallel test execution, concurrent invocations of the same test method on different backends could share these fields and race. If parallel execution is not enabled this is fine; otherwise consider making the session per-invocation rather than instance state. ########## 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: The `testIdpUserPOBuilderValidation` test expects this builder to throw `IllegalArgumentException`, but `Preconditions.checkArgument(x != null, ...)` throws `IllegalArgumentException` — that's fine. However, if `validate()` is meant to ensure required fields are present, `userId != null` style checks are essentially null guards; consider `Preconditions.checkNotNull(userId, "User id is required")` for clearer intent (note this would throw `NullPointerException`, which would require updating the test). Either is acceptable, but the current approach is idiomatic only if the validation is broader than null. Optional. ########## 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 `public static` provider methods have no effect — MyBatis only honors `@Param` on mapper interface methods. They are misleading here and can be removed to avoid implying parameter-name binding semantics at the factory layer. ########## 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 passes an empty provider map to exercise the \"no provider registered\" branch, which is fine, but the production `getProvider(String)` path supplies `IDP_USER_META_SQL_PROVIDER_MAP` directly and so the missing-mapping branch can only be reached via this package-private overload. Consider whether the package-private `getProvider(JDBCBackendType, String, Map)` overload is truly needed in production code, or if it should be moved to a test helper / made `@VisibleForTesting`, to avoid exposing a backdoor API only to test an unreachable branch. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/idp/basic/storage/relational/mapper/TestIdpUserMetaMySQLProvider.java: ########## @@ -0,0 +1,37 @@ +/* + * 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; Review Comment: This test sits in the `...mapper` package and is named `TestIdpUserMetaMySQLProvider`, whereas the analogous H2/PostgreSQL provider tests live in `...mapper.provider.h2`/`...provider.postgresql`. For consistency, move this class to `org.apache.gravitino.idp.basic.storage.relational.mapper.provider.mysql` so all three provider tests share a parallel package layout. -- 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]
