Copilot commented on code in PR #11023: URL: https://github.com/apache/gravitino/pull/11023#discussion_r3214744356
########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpUserMetaBaseSQLProvider.java: ########## @@ -0,0 +1,98 @@ +/* + * 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 AND user_name IN " + + "<foreach item='item' collection='userNames' open='(' separator=',' close=')'>" + + "#{item}" + + "</foreach>" + + "</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, + @Param("currentVersion") Long currentVersion, + @Param("newCurrentVersion") Long newCurrentVersion, + @Param("newLastVersion") Long newLastVersion) { + return "UPDATE " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " SET password_hash = #{passwordHash}," + + " current_version = #{newCurrentVersion}," + + " last_version = #{newLastVersion}" + + " WHERE user_id = #{userId}" + + " AND current_version = #{currentVersion}" + + " AND deleted_at = 0"; + } + + public String softDeleteIdpUser( + @Param("userId") Long userId, @Param("deletedAt") Long deletedAt) { + return "UPDATE " + + IdpUserMetaMapper.IDP_USER_TABLE_NAME + + " SET deleted_at = #{deletedAt}," Review Comment: `softDeleteIdpUser` sets `deleted_at` from the caller-provided `deletedAt` parameter. In the relational store codebase, soft-delete SQLs typically set `deleted_at` using a backend-specific database clock expression (e.g., `UserMetaBaseSQLProvider.softDeleteUserMetaByUserId` uses `UNIX_TIMESTAMP()`), which avoids app/DB clock skew and removes the need for callers to compute timestamps. Consider aligning this provider (and corresponding mapper/service API) to generate `deleted_at` in SQL per backend instead of taking it as an input parameter. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpGroupUserRelBaseSQLProvider.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.gravitino.storage.relational.mapper.provider.base; + +import java.util.List; +import org.apache.gravitino.storage.relational.mapper.IdpGroupUserRelMapper; +import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupUserRelBaseSQLProvider { + + public String selectGroupNamesByUserId(@Param("userId") Long userId) { + return "SELECT g.group_name" + + " FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " r JOIN " + + IdpGroupUserRelMapper.IDP_GROUP_TABLE_NAME + + " g ON g.group_id = r.group_id" + + " WHERE r.user_id = #{userId}" + + " AND r.deleted_at = 0" + + " AND g.deleted_at = 0" + + " ORDER BY g.group_name"; + } + + public String selectUserNamesByGroupId(@Param("groupId") Long groupId) { + return "SELECT u.user_name" + + " FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " r JOIN " + + IdpGroupUserRelMapper.IDP_USER_TABLE_NAME + + " u ON u.user_id = r.user_id" + + " WHERE r.group_id = #{groupId}" + + " AND r.deleted_at = 0" + + " AND u.deleted_at = 0" + + " ORDER BY u.user_name"; + } + + public String selectRelatedUserIds( + @Param("groupId") Long groupId, @Param("userIds") List<Long> userIds) { + return "<script>" + + "SELECT user_id" + + " FROM " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " WHERE group_id = #{groupId} " + + "<choose>" + + "<when test='userIds != null and userIds.size() > 0'>" + + "AND user_id IN (" + + "<foreach collection='userIds' item='userId' separator=','>" + + "#{userId}" + + "</foreach>" + + ") " + + "</when>" + + "<otherwise>" + + "AND 1 = 0 " + + "</otherwise>" + + "</choose>" + + "AND deleted_at = 0" + + "</script>"; + } + + public String batchInsertIdpGroupUsers(@Param("relations") List<IdpGroupUserRelPO> relations) { + return "<script>" + + "INSERT INTO " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " (id, group_id, user_id, current_version, last_version, deleted_at)" + + " VALUES " + + "<foreach item='item' collection='relations' separator=','>" + + "(#{item.id}, #{item.groupId}, #{item.userId}, #{item.currentVersion}," + + " #{item.lastVersion}, #{item.deletedAt})" + + "</foreach>" + + "</script>"; + } + + public String softDeleteIdpGroupUsers( + @Param("groupId") Long groupId, + @Param("userIds") List<Long> userIds, + @Param("deletedAt") Long deletedAt) { + return "<script>" + + "UPDATE " + + IdpGroupUserRelMapper.IDP_GROUP_USER_REL_TABLE_NAME + + " SET deleted_at = #{deletedAt}," + + " current_version = current_version + 1," + + " last_version = last_version + 1" Review Comment: `softDeleteIdpGroupUsers` uses the caller-provided `deletedAt` value (`deleted_at = #{deletedAt}`), while other relational-store soft-delete SQLs typically compute `deleted_at` using database time functions. Aligning to DB-generated `deleted_at` would reduce reliance on caller timestamps and match the existing storage-layer convention across backends. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/service/IdpUserMetaService.java: ########## @@ -0,0 +1,117 @@ +/* + * 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.service; + +import com.google.common.base.Preconditions; +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import org.apache.gravitino.storage.relational.mapper.IdpGroupUserRelMapper; +import org.apache.gravitino.storage.relational.mapper.IdpUserMetaMapper; +import org.apache.gravitino.storage.relational.po.IdpUserPO; +import org.apache.gravitino.storage.relational.utils.SessionUtils; + +/** The service class for user metadata. It provides the basic database operations for user. */ +public class IdpUserMetaService { + private static final IdpUserMetaService INSTANCE = new IdpUserMetaService(); + + public static IdpUserMetaService getInstance() { + return INSTANCE; + } + + private IdpUserMetaService() {} Review Comment: This new storage service introduces non-trivial logic (multi-table deletes via `SessionUtils.doMultipleWithCommit`, version-checked password updates, empty-list short-circuiting), but there are no unit/integration tests in this module exercising `IdpUserMetaService` behavior. Adding focused tests (similar to existing relational service tests under `core/src/test/java/.../service`) would help prevent regressions across different JDBC backends. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/IdpGroupMetaBaseSQLProvider.java: ########## @@ -0,0 +1,66 @@ +/* + * 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 org.apache.gravitino.storage.relational.mapper.IdpGroupMetaMapper; +import org.apache.gravitino.storage.relational.po.IdpGroupPO; +import org.apache.ibatis.annotations.Param; + +public class IdpGroupMetaBaseSQLProvider { + + public String selectIdpGroup(@Param("groupName") String groupName) { + return "SELECT group_id as groupId, group_name as groupName," + + " current_version as currentVersion," + + " last_version as lastVersion, deleted_at as deletedAt" + + " FROM " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " WHERE group_name = #{groupName} AND deleted_at = 0"; + } + + public String insertIdpGroup(@Param("groupMeta") IdpGroupPO groupPO) { + return "INSERT INTO " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " (group_id, group_name, current_version, last_version, deleted_at)" + + " VALUES (" + + " #{groupMeta.groupId}," + + " #{groupMeta.groupName}," + + " #{groupMeta.currentVersion}," + + " #{groupMeta.lastVersion}," + + " #{groupMeta.deletedAt}" + + " )"; + } + + public String softDeleteIdpGroup( + @Param("groupId") Long groupId, @Param("deletedAt") Long deletedAt) { + return "UPDATE " + + IdpGroupMetaMapper.IDP_GROUP_TABLE_NAME + + " SET deleted_at = #{deletedAt}," Review Comment: `softDeleteIdpGroup` sets `deleted_at = #{deletedAt}` from an input parameter. Elsewhere in the relational store, soft deletes generally use database-side timestamp expressions for `deleted_at` (to avoid clock skew and keep SQL backend-specific). Consider switching to DB-generated `deleted_at` (and updating the mapper/service signature accordingly) for consistency with the rest of the storage layer. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/service/IdpGroupMetaService.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.service; + +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import org.apache.gravitino.storage.relational.mapper.IdpGroupMetaMapper; +import org.apache.gravitino.storage.relational.mapper.IdpGroupUserRelMapper; +import org.apache.gravitino.storage.relational.po.IdpGroupPO; +import org.apache.gravitino.storage.relational.po.IdpGroupUserRelPO; +import org.apache.gravitino.storage.relational.utils.SessionUtils; + +/** The service class for group metadata. It provides the basic database operations for group. */ +public class IdpGroupMetaService { + private static final IdpGroupMetaService INSTANCE = new IdpGroupMetaService(); + + public static IdpGroupMetaService getInstance() { + return INSTANCE; + } + + private IdpGroupMetaService() {} Review Comment: This new storage service coordinates multiple mapper operations (group deletion + relation deletion, batch inserts, guarded removes), but the module currently lacks tests that validate `IdpGroupMetaService` end-to-end behavior. Consider adding service-level tests (patterned after existing relational service tests in `core/src/test/java/.../service`) to cover success and edge cases (empty lists, missing group, etc.). ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/po/IdpUserPO.java: ########## @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.storage.relational.po; + +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; + +public class IdpUserPO { + private Long userId; + private String userName; + private String passwordHash; + private Long currentVersion; + private Long lastVersion; + private Long deletedAt; + + public Long getUserId() { + return userId; + } + + public String getUserName() { + return userName; + } + + public String getPasswordHash() { + return passwordHash; + } + + public Long getCurrentVersion() { + return currentVersion; + } + + public Long getLastVersion() { + return lastVersion; + } + + public Long getDeletedAt() { + return deletedAt; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof IdpUserPO)) { + return false; + } + IdpUserPO tablePO = (IdpUserPO) o; + return Objects.equal(getUserId(), tablePO.getUserId()) + && Objects.equal(getUserName(), tablePO.getUserName()) + && Objects.equal(getPasswordHash(), tablePO.getPasswordHash()) + && Objects.equal(getCurrentVersion(), tablePO.getCurrentVersion()) + && Objects.equal(getLastVersion(), tablePO.getLastVersion()) + && Objects.equal(getDeletedAt(), tablePO.getDeletedAt()); Review Comment: In `equals`, the local variable is named `tablePO`, which is misleading in this IdP user PO and makes the method harder to read. Rename it to something representative like `userPO` (or similar) to reflect the actual type. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/po/IdpGroupPO.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.po; + +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; + +public class IdpGroupPO { + private Long groupId; + private String groupName; + private Long currentVersion; + private Long lastVersion; + private Long deletedAt; + + public Long getGroupId() { + return groupId; + } + + public String getGroupName() { + return groupName; + } + + public Long getCurrentVersion() { + return currentVersion; + } + + public Long getLastVersion() { + return lastVersion; + } + + public Long getDeletedAt() { + return deletedAt; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof IdpGroupPO)) { + return false; + } + IdpGroupPO tablePO = (IdpGroupPO) o; + return Objects.equal(getGroupId(), tablePO.getGroupId()) + && Objects.equal(getGroupName(), tablePO.getGroupName()) + && Objects.equal(getCurrentVersion(), tablePO.getCurrentVersion()) + && Objects.equal(getLastVersion(), tablePO.getLastVersion()) + && Objects.equal(getDeletedAt(), tablePO.getDeletedAt()); Review Comment: In `equals`, the local variable is named `tablePO`, which is misleading for a group PO. Rename it to something representative like `groupPO` (or similar) to improve readability and avoid copy/paste artifacts. -- 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]
