Copilot commented on code in PR #11066: URL: https://github.com/apache/gravitino/pull/11066#discussion_r3239563206
########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpUserMetaBaseSQLProvider.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.provider.base; + +import java.util.List; +import org.apache.gravitino.storage.relational.mapper.IdpUserMetaMapper; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.ibatis.annotations.Param; + +public 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 = (UNIX_TIMESTAMP() * 1000.0)" + + " WHERE user_id = #{userId} AND deleted_at = 0"; Review Comment: `UNIX_TIMESTAMP()` is a MySQL-specific function and is not available in H2 by default. Since this base provider is also used as the H2 provider (`JDBCBackendType.H2 -> new IdpUserMetaBaseSQLProvider()` in `IdpUserMetaSQLProviderFactory`), `softDeleteIdpUser` will fail at runtime against H2 unless the H2 backend is configured in MySQL compatibility mode with this function emulated. Either move this implementation to a MySQL-specific subclass and provide an H2-compatible default in the base (e.g., a portable expression using `CURRENT_TIMESTAMP`), or document/assert that H2 must run in MYSQL mode. The H2 integration test currently uses `MODE=MYSQL` which masks this portability issue. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/it/TestIdpUserMetaMapperPostgreSQL.java: ########## @@ -0,0 +1,70 @@ +/* + * 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 org.apache.gravitino.storage.relational.BackendTypes; Review Comment: The file is located under the `mapper/it/` directory but the package declaration is `org.apache.gravitino.storage.relational.mapper` (missing `.it`). This package/directory mismatch will fail to compile under the standard Maven/Gradle source layout, since Java requires the file's directory to match its package. The same issue exists in `TestIdpUserMetaMapperMySQL.java`, `TestIdpUserMetaMapperH2.java`, and `IdpUserMetaMapperTest.java`. Either move these files out of the `it/` subdirectory or update the package declarations to `org.apache.gravitino.storage.relational.mapper.it`. ########## 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", 3L, 2L, 0L); + + assertEquals(1, testBase.idpUserMetaMapper.updateIdpUserPassword(1L, "hash-a-2")); + assertEquals("hash-a-2", testBase.idpUserMetaMapper.selectIdpUser("alice").getPasswordHash()); + assertEquals(3L, testBase.idpUserMetaMapper.selectIdpUser("alice").getCurrentVersion()); + assertEquals(2L, testBase.idpUserMetaMapper.selectIdpUser("alice").getLastVersion()); + } + + default void testUpdateIdpUserPasswordReturnsZeroForDeletedUser() { + IdpMapperTestBase testBase = testBase(); + testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 10L); + + assertEquals(0, testBase.idpUserMetaMapper.updateIdpUserPassword(1L, "hash-a-2")); + assertEquals( + 1L, testBase.queryLongValueInMapperTest("idp_user_meta", "current_version", "user_id", 1L)); + assertEquals( + 0L, testBase.queryLongValueInMapperTest("idp_user_meta", "last_version", "user_id", 1L)); + assertEquals( + 10L, testBase.queryLongValueInMapperTest("idp_user_meta", "deleted_at", "user_id", 1L)); + } + + default void testSoftDeleteIdpUser() { + IdpMapperTestBase testBase = testBase(); + testBase.insertUser(1L, "alice", "hash-a", 1L, 0L, 0L); + + testBase.idpUserMetaMapper.softDeleteIdpUser(1L); + assertNull(testBase.idpUserMetaMapper.selectIdpUser("alice")); + assertTrue( + testBase.queryLongValueInMapperTest("idp_user_meta", "deleted_at", "user_id", 1L) > 0L); + assertEquals( + 1L, testBase.queryLongValueInMapperTest("idp_user_meta", "current_version", "user_id", 1L)); + assertEquals( + 0L, testBase.queryLongValueInMapperTest("idp_user_meta", "last_version", "user_id", 1L)); + } + + default void testDeleteIdpUserMetasByLegacyTimeline() { + IdpMapperTestBase testBase = testBase(); + testBase.insertUser(1L, "legacy-user", "hash", 1L, 0L, 10L); + testBase.insertUser(2L, "new-user", "hash", 1L, 0L, 30L); + testBase.insertUser(3L, "active-user", "hash", 1L, 0L, 0L); + + assertEquals(1, testBase.idpUserMetaMapper.deleteIdpUserMetasByLegacyTimeline(20L, 10)); + assertEquals(0, testBase.countRowsInMapperTest("idp_user_meta", "user_id", 1L)); + assertEquals(1, testBase.countRowsInMapperTest("idp_user_meta", "user_id", 2L)); + assertEquals(1, testBase.countRowsInMapperTest("idp_user_meta", "user_id", 3L)); + } + + private IdpMapperTestBase testBase() { + return (IdpMapperTestBase) this; + } Review Comment: Casting `this` to `IdpMapperTestBase` inside an interface relies on every implementor also extending `IdpMapperTestBase`; a `ClassCastException` will be thrown at runtime otherwise, with no compile-time guarantee. Consider either declaring an abstract `IdpMapperTestBase testBase();` method to be implemented by each concrete subclass, or making this an abstract base class instead of an interface. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/TestBackendTestExtension.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.assertFalse; +import static org.junit.jupiter.api.Assertions.assertIterableEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import org.junit.jupiter.api.Test; + +public class TestBackendTestExtension { + + @Test + public void testDockerTestSystemPropertyOverridesEnvironment() { + String originalValue = System.getProperty("dockerTest"); + try { + System.setProperty("dockerTest", "false"); + assertFalse(BackendTestExtension.isDockerTestEnabled()); + } finally { + restoreDockerTestProperty(originalValue); + } + } Review Comment: Mutating `System.setProperty(\"dockerTest\", ...)` from a unit test is not safe for parallel test execution: other tests reading `dockerTest` via `BackendTestExtension.isDockerTestEnabled()` can observe the temporarily-modified value. Since the test class is not annotated with `@ResourceLock(\"dockerTest\")` (or similar), running tests in parallel could lead to flaky results. Consider extracting the property/env lookup behind an injectable strategy or adding a `@ResourceLock` annotation on these tests. ########## plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/mapper/TestIdpUserMetaSQLProviderFactory.java: ########## @@ -0,0 +1,36 @@ +/* + * 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 org.apache.gravitino.storage.relational.BackendTypes; +import org.apache.gravitino.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider; +import org.junit.jupiter.api.TestTemplate; + +@BackendTypes({"h2"}) +public class TestIdpUserMetaSQLProviderFactory extends IdpMapperTestBase { + + @TestTemplate + void testGetProviderReturnsH2Provider() { + IdpUserMetaBaseSQLProvider provider = IdpUserMetaSQLProviderFactory.getProvider(); + assertEquals(IdpUserMetaBaseSQLProvider.class, provider.getClass()); + } Review Comment: This assertion uses exact class comparison (`assertEquals(IdpUserMetaBaseSQLProvider.class, provider.getClass())`), but the factory map registers `new IdpUserMetaBaseSQLProvider()` for H2 — which is correct only as long as the H2 provider remains the base class. However, since `IdpUserMetaBaseSQLProvider` is also the parent of MySQL/PostgreSQL providers, an exact-class assertion will silently allow a future refactor that returns a subclass to pass when it shouldn't, and will fail when an H2-specific subclass is introduced. Consider using `assertInstanceOf`/`isInstance` or asserting on a behavioral property (e.g., the H2-flavored SQL output) to make the intent of the test clearer. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/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.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 are provided through {@code *Provider} annotations on this mapper interface. See + * the <a href="https://mybatis.org/mybatis-3/getting-started.html";>MyBatis getting started + * guide</a>. + */ +public interface IdpUserMetaMapper { + String IDP_USER_TABLE_NAME = "idp_user_meta"; + + @SelectProvider(type = IdpUserMetaSQLProviderFactory.class, method = "selectIdpUser") + IdpUserPO selectIdpUser(@Param("username") String username); + + @SelectProvider(type = IdpUserMetaSQLProviderFactory.class, method = "selectIdpUsers") + List<IdpUserPO> selectIdpUsers(@Param("usernames") List<String> usernames); + + @InsertProvider(type = IdpUserMetaSQLProviderFactory.class, method = "insertIdpUser") + void insertIdpUser(@Param("userMeta") IdpUserPO userPO); + + @UpdateProvider(type = IdpUserMetaSQLProviderFactory.class, method = "updateIdpUserPassword") + Integer updateIdpUserPassword( + @Param("userId") Long userId, @Param("passwordHash") String passwordHash); + + @UpdateProvider(type = IdpUserMetaSQLProviderFactory.class, method = "softDeleteIdpUser") + void softDeleteIdpUser(@Param("userId") Long userId); Review Comment: `softDeleteIdpUser` returns `void`, but the integration test `testUpdateIdpUserPasswordReturnsZeroForDeletedUser` and the intent behind soft-delete typically rely on knowing whether a row was actually updated (e.g., to detect double-delete or already-deleted users). Returning `Integer` (the affected row count) here would be consistent with `updateIdpUserPassword` and `deleteIdpUserMetasByLegacyTimeline` in this same mapper and provide useful information to callers. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/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.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()); Review Comment: Using `IdpUserMetaBaseSQLProvider` as both an abstract-style base and the concrete H2 implementation makes intent ambiguous: callers can't tell whether instantiating the base class is intentional (H2) or a mistake. Consider either making the base class abstract and creating an explicit `IdpUserMetaH2Provider` subclass, or renaming to clearly convey that the base is also the default implementation. This will also avoid the fragile exact-class assertion in `TestIdpUserMetaSQLProviderFactory`. -- 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]
