Copilot commented on code in PR #11127:
URL: https://github.com/apache/gravitino/pull/11127#discussion_r3257153799
##########
plugins/idp-basic/src/test/java/org/apache/gravitino/idp/storage/mapper/TestIdpUserMetaStorage.java:
##########
@@ -104,35 +164,87 @@ void
testUpdateIdpUserPasswordKeepsVersionsUnchanged(String type) throws IOExcep
@MethodSource("storageProvider")
void testSoftDeleteIdpUser(String type) throws IOException {
init(type);
- insertUser(1L, "alice", "hash-a", 1L, 0L, 0L);
+ idpUserMetaMapper.insertIdpUser(
+ IdpUserPO.builder()
+ .withUserId(1L)
+ .withUserName("alice")
+ .withPasswordHash("hash-a")
+ .withCurrentVersion(1L)
+ .withLastVersion(0L)
+ .withDeletedAt(0L)
+ .build());
assertEquals(1, idpUserMetaMapper.softDeleteIdpUser(1L));
assertNull(idpUserMetaMapper.selectIdpUser("alice"));
- assertTrue(queryLongValue("idp_user_meta", "deleted_at", "user_id", 1L) >
0L);
- assertEquals(1L, queryLongValue("idp_user_meta", "current_version",
"user_id", 1L));
- assertEquals(0L, queryLongValue("idp_user_meta", "last_version",
"user_id", 1L));
+ assertEquals(0, idpUserMetaMapper.softDeleteIdpUser(1L));
+ assertEquals(0, idpUserMetaMapper.updateIdpUserPassword(1L, "hash-a-2"));
+ assertEquals(1,
idpUserMetaMapper.deleteIdpUserMetasByLegacyTimeline(Long.MAX_VALUE, 10));
Review Comment:
This test no longer validates key soft-delete invariants that were
previously asserted via direct SQL (e.g., `deleted_at` is set to a positive
value and version columns remain unchanged). Consider reintroducing those
assertions via a test-only helper query (or adding a mapper method suitable for
tests that can read rows regardless of `deleted_at`) to avoid losing coverage
for those behaviors.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/IdpUserMetaSQLProviderFactory.java:
##########
@@ -19,101 +19,46 @@
package org.apache.gravitino.idp.storage.mapper;
-import com.google.common.collect.ImmutableMap;
import java.util.List;
-import java.util.Map;
import
org.apache.gravitino.idp.storage.mapper.provider.base.IdpUserMetaBaseSQLProvider;
import
org.apache.gravitino.idp.storage.mapper.provider.h2.IdpUserMetaH2Provider;
import
org.apache.gravitino.idp.storage.mapper.provider.postgresql.IdpUserMetaPostgreSQLProvider;
import org.apache.gravitino.idp.storage.po.IdpUserPO;
-import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType;
-import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper;
import org.apache.ibatis.annotations.Param;
-public class IdpUserMetaSQLProviderFactory {
- private static final IdpUserMetaBaseSQLProvider IDP_USER_META_BASE_PROVIDER =
- new IdpUserMetaBaseSQLProvider();
- private static final IdpUserMetaBaseSQLProvider IDP_USER_META_H2_PROVIDER =
- new IdpUserMetaH2Provider();
- private static final IdpUserMetaBaseSQLProvider
IDP_USER_META_POSTGRESQL_PROVIDER =
- new IdpUserMetaPostgreSQLProvider();
-
- private static final Map<JDBCBackendType, IdpUserMetaBaseSQLProvider>
- IDP_USER_META_SQL_PROVIDER_MAP =
- ImmutableMap.of(
- JDBCBackendType.MYSQL, IDP_USER_META_BASE_PROVIDER,
- JDBCBackendType.H2, IDP_USER_META_H2_PROVIDER,
- JDBCBackendType.POSTGRESQL, IDP_USER_META_POSTGRESQL_PROVIDER);
-
- static IdpUserMetaBaseSQLProvider getProvider(String databaseId) {
- if (databaseId == null) {
- throw new IllegalStateException(
- "MyBatis databaseId is not configured for IdP user SQL providers.");
- }
-
- try {
- JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId);
- IdpUserMetaBaseSQLProvider provider =
IDP_USER_META_SQL_PROVIDER_MAP.get(jdbcBackendType);
- if (provider != null) {
- return provider;
- }
-
- throw new IllegalStateException(
- String.format(
- "No IdP user SQL provider registered for backend %s (databaseId:
%s)",
- jdbcBackendType, databaseId));
- } catch (IllegalArgumentException e) {
- throw new IllegalStateException(
- String.format(
- "Unsupported IdP user SQL provider databaseId: %s, supported
backends: %s",
- databaseId, IDP_USER_META_SQL_PROVIDER_MAP.keySet()),
- e);
- }
- }
-
- public static IdpUserMetaBaseSQLProvider h2Provider() {
- return IDP_USER_META_H2_PROVIDER;
+public class IdpUserMetaSQLProviderFactory
+ extends IdpBaseSQLProviderFactory<IdpUserMetaBaseSQLProvider> {
+ public IdpUserMetaSQLProviderFactory() {
+ super(
+ "IdP user SQL provider",
+ new IdpUserMetaBaseSQLProvider(),
+ new IdpUserMetaH2Provider(),
+ new IdpUserMetaPostgreSQLProvider());
}
Review Comment:
This refactor removes the previously-available static API (e.g., static
provider accessors / static SQL methods). If any non-MyBatis callers relied on
`IdpUserMetaSQLProviderFactory.*` static methods, this is a breaking change.
Consider keeping the former static entry points as deprecated wrappers
delegating to an instance, or ensure all call sites are updated and the change
is called out as an intentional API break.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/mapper/IdpGroupMetaSQLProviderFactory.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.storage.mapper;
+
+import java.util.List;
+import
org.apache.gravitino.idp.storage.mapper.provider.base.IdpGroupMetaBaseSQLProvider;
+import
org.apache.gravitino.idp.storage.mapper.provider.h2.IdpGroupMetaH2Provider;
+import
org.apache.gravitino.idp.storage.mapper.provider.postgresql.IdpGroupMetaPostgreSQLProvider;
+import org.apache.gravitino.idp.storage.po.IdpGroupPO;
+import org.apache.ibatis.annotations.Param;
+
+public class IdpGroupMetaSQLProviderFactory
+ extends IdpBaseSQLProviderFactory<IdpGroupMetaBaseSQLProvider> {
+ public IdpGroupMetaSQLProviderFactory() {
+ super(
+ "IdP group SQL provider",
+ new IdpGroupMetaBaseSQLProvider(),
+ new IdpGroupMetaH2Provider(),
+ new IdpGroupMetaPostgreSQLProvider());
+ }
+
+ public String selectIdpGroup(@Param("groupName") String groupName) {
+ return currentProvider().selectIdpGroup(groupName);
+ }
+
+ public String selectIdpGroups(@Param("groupNames") List<String> groupNames) {
+ return currentProvider().selectIdpGroups(groupNames);
+ }
+
+ public String insertIdpGroup(@Param("groupMeta") IdpGroupPO groupPO) {
+ return currentProvider().insertIdpGroup(groupPO);
+ }
+
+ public String softDeleteIdpGroup(@Param("groupId") Long groupId) {
+ return currentProvider().softDeleteIdpGroup(groupId);
+ }
+
+ public String deleteIdpGroupMetasByLegacyTimeline(
+ @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit)
{
+ return
currentProvider().deleteIdpGroupMetasByLegacyTimeline(legacyTimeline, limit);
Review Comment:
MyBatis instantiates provider classes when provider methods are instance
methods; with the current design, each instantiation allocates 3 provider
objects and an ImmutableMap. To reduce allocations on hot paths, consider
making provider instances (and the provider map) static finals, or switching
provider methods back to static so MyBatis doesn't repeatedly construct the
factory.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/idp/storage/po/IdpGroupPO.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.storage.po;
+
+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)
Review Comment:
Using a private no-args constructor can break MyBatis result object
instantiation in some environments (e.g., when reflective 'setAccessible' is
restricted). To avoid runtime mapping failures, make the no-args constructor at
least package-private/protected (or public), or provide an explicit accessible
constructor strategy compatible with your MyBatis configuration.
##########
plugins/idp-basic/src/test/java/org/apache/gravitino/idp/storage/mapper/provider/base/TestIdpGroupMetaBaseSQLProvider.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.storage.mapper.provider.base;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.gravitino.idp.storage.po.IdpGroupPO;
+import org.apache.ibatis.builder.BuilderException;
+import org.apache.ibatis.mapping.BoundSql;
+import org.apache.ibatis.mapping.SqlSource;
+import org.apache.ibatis.scripting.xmltags.XMLLanguageDriver;
+import org.apache.ibatis.session.Configuration;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestIdpGroupMetaBaseSQLProvider {
+
+ protected IdpGroupMetaBaseSQLProvider createProvider() {
+ return new IdpGroupMetaBaseSQLProvider() {
+ @Override
+ protected String currentTimeMillisExpression() {
+ return "CURRENT_TIME_MILLIS()";
+ }
+ };
+ }
+
+ protected String expectedDeleteAtClause() {
+ return "deleted_at = CURRENT_TIME_MILLIS()";
+ }
+
+ protected String expectedDeleteIdpGroupMetasByLegacyTimelineSql() {
+ return "DELETE FROM idp_group_meta WHERE deleted_at > 0 AND deleted_at <
#{legacyTimeline}"
+ + " LIMIT #{limit}";
+ }
+
+ @Test
+ void testSelectIdpGroup() {
+ String normalizedSql =
createProvider().selectIdpGroup("group").replaceAll("\\s+", " ").trim();
+
+ Assertions.assertTrue(normalizedSql.contains("SELECT group_id as
groupId"));
+ Assertions.assertTrue(normalizedSql.contains("FROM idp_group_meta"));
+ Assertions.assertTrue(
+ normalizedSql.contains("WHERE group_name = #{groupName} AND deleted_at
= 0"));
+ }
+
+ @Test
+ void testSelectIdpGroups() {
+ String script = createProvider().selectIdpGroups(Arrays.asList("dev",
"ops"));
+ Map<String, Object> params = new HashMap<>();
+ params.put("groupNames", Arrays.asList("dev", "ops"));
+
+ String normalizedSql = renderScript(script, params);
+
+ Assertions.assertTrue(normalizedSql.contains("SELECT group_id as
groupId"));
+ Assertions.assertTrue(normalizedSql.contains("FROM idp_group_meta"));
+ Assertions.assertTrue(normalizedSql.matches(".*group_name IN \\( \\? , \\?
\\).*"));
Review Comment:
This regex is overly strict about whitespace around the comma, which can
vary based on MyBatis rendering and makes the test fragile. Consider loosening
the assertion to allow optional whitespace (e.g., matching `\\?\\s*,\\s*\\?`)
while still verifying the correct number of placeholders.
--
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]
