Copilot commented on code in PR #11025: URL: https://github.com/apache/gravitino/pull/11025#discussion_r3215054448
########## plugins/idp-basic/src/main/java/org/apache/gravitino/server/web/rest/IdpUserOperations.java: ########## @@ -0,0 +1,169 @@ +/* + * 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.server.web.rest; + +import com.codahale.metrics.annotation.ResponseMetered; +import com.codahale.metrics.annotation.Timed; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.DELETE; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; +import org.apache.gravitino.dto.responses.RemoveResponse; +import org.apache.gravitino.idp.basic.authorization.BasicIdpManager; +import org.apache.gravitino.idp.basic.dto.requests.CreateUserRequest; +import org.apache.gravitino.idp.basic.dto.requests.ResetPasswordRequest; +import org.apache.gravitino.idp.basic.dto.responses.IdpUserResponse; +import org.apache.gravitino.metrics.MetricNames; +import org.apache.gravitino.server.authorization.NameBindings; +import org.apache.gravitino.server.authorization.annotations.AuthorizationExpression; +import org.apache.gravitino.server.web.Utils; + +/** REST resource for built-in IdP user management exposed by the {@code idp-basic} plugin. */ [email protected] +@Path("/idp/users") +public class IdpUserOperations { + private static final String NULL_REQUEST_BODY_ERROR = "Request body cannot be null"; + private static final String SERVICE_ADMIN_ERROR = + "Only Gravitino service admins can manage built-in IdP identities"; + + private final BasicIdpManager idpManager; + + @Context private HttpServletRequest httpRequest; + + /** Creates a REST resource backed by the default built-in IdP manager. */ + public IdpUserOperations() { + this(new BasicIdpManager()); + } Review Comment: The default constructor creates a new BasicIdpManager instance, which bypasses the IdpManager loaded and lifecycle-managed by GravitinoEnv (GravitinoEnv now eagerly creates/closes an IdpManager via ServiceLoader). This can lead to multiple manager instances, inconsistent implementation selection, and the env-managed manager being unused. Prefer obtaining the singleton from GravitinoEnv (or injecting the env-managed instance) instead of constructing a new manager here. ########## server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java: ########## @@ -576,6 +584,86 @@ public Response handle(OperationType op, String group, String metalake, Exceptio } } + private static class IdpUserExceptionHandler extends BaseExceptionHandler { + + private static final ExceptionHandler INSTANCE = new IdpUserExceptionHandler(); + + private static String getUserErrorMsg(String user, String operation, String reason) { + return String.format( + "Failed to operate built-in IdP user %s operation [%s], reason [%s]", + user, operation, reason); + } + + @Override + public Response handle(OperationType op, String user, String ignored, Exception e) { + String formatted = StringUtil.isBlank(user) ? "" : " [" + user + "]"; + String errorMsg = getUserErrorMsg(formatted, op.name(), getErrorMsg(e)); Review Comment: `formatted` is built with a leading space (" [" + user + "]"), but `getUserErrorMsg` already inserts a space before the `%s` placeholder ("... user %s operation ..."). This results in double spaces in the emitted error message (e.g., "user [alice]"). Consider removing the leading space from `formatted` or adjusting the format string. ########## plugins/idp-basic/src/main/java/org/apache/gravitino/server/web/rest/IdpGroupOperations.java: ########## @@ -0,0 +1,204 @@ +/* + * 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.server.web.rest; + +import com.codahale.metrics.annotation.ResponseMetered; +import com.codahale.metrics.annotation.Timed; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.DELETE; +import javax.ws.rs.DefaultValue; +import javax.ws.rs.GET; +import javax.ws.rs.POST; +import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; +import javax.ws.rs.core.Response; +import org.apache.gravitino.dto.responses.RemoveResponse; +import org.apache.gravitino.idp.basic.authorization.BasicIdpManager; +import org.apache.gravitino.idp.basic.dto.requests.CreateGroupRequest; +import org.apache.gravitino.idp.basic.dto.requests.UpdateGroupUsersRequest; +import org.apache.gravitino.idp.basic.dto.responses.IdpGroupResponse; +import org.apache.gravitino.metrics.MetricNames; +import org.apache.gravitino.server.authorization.NameBindings; +import org.apache.gravitino.server.authorization.annotations.AuthorizationExpression; +import org.apache.gravitino.server.web.Utils; + +/** REST resource for built-in IdP group management exposed by the {@code idp-basic} plugin. */ [email protected] +@Path("/idp/groups") +public class IdpGroupOperations { + private static final String NULL_REQUEST_BODY_ERROR = "Request body cannot be null"; + private static final String SERVICE_ADMIN_ERROR = + "Only Gravitino service admins can manage built-in IdP identities"; + + private final BasicIdpManager idpManager; + + @Context private HttpServletRequest httpRequest; + + /** Creates a REST resource backed by the default built-in IdP manager. */ + public IdpGroupOperations() { + this(new BasicIdpManager()); + } Review Comment: The default constructor creates a new BasicIdpManager instance instead of using the IdpManager instance created/owned by GravitinoEnv. This can result in multiple manager instances and makes the ServiceLoader-based IdpManagerFactory/GravitinoEnv.idpManager() effectively unused. Prefer wiring this resource to the env-managed manager (or injecting it) rather than instantiating a new one. ########## 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 implements IdpGroupMeta { + 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()) Review Comment: In equals(), the local variable is named `tablePO`, which is misleading for an IdP group record. Renaming it to `other`/`groupPO` would make the equality logic clearer. ########## 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 implements IdpUserMeta { + 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()) Review Comment: In equals(), the local variable is named `tablePO`, which is misleading for an IdP user record and makes the comparison harder to read/maintain. Consider renaming it to something like `other` or `userPO` to match the type. ########## core/src/main/java/org/apache/gravitino/IdpManagerFactory.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; + +import java.util.ArrayList; +import java.util.List; +import java.util.ServiceLoader; +import org.apache.gravitino.authorization.IdpManager; + +/** + * This class is responsible for creating instances of IdpManager implementations. IdpManager + * implementations are used to manage built-in IdP users and groups within the Apache Gravitino + * framework. + */ +public class IdpManagerFactory { + + // Private constructor to prevent instantiation of this factory class. + private IdpManagerFactory() {} + + /** + * Creates an instance of IdpManager from the runtime classpath. + * + * @return An instance of IdpManager. + */ + public static IdpManager createIdpManager() { + return loadService(IdpManager.class); + } + + private static <T> T loadService(Class<T> serviceClass) { + List<T> services = new ArrayList<>(); + for (T service : ServiceLoader.load(serviceClass)) { + services.add(service); + } + + if (services.isEmpty()) { + throw new IllegalStateException( + String.format("No %s implementation found", serviceClass.getSimpleName())); + } + + if (services.size() > 1) { + throw new IllegalStateException( + String.format("Multiple %s implementations found", serviceClass.getSimpleName())); + } + + return services.get(0); Review Comment: IdpManagerFactory re-implements the same single-ServiceLoader-selection logic as IdpMetaProviderLoader (collect services, error on 0 or >1). This duplication increases maintenance cost and risks future divergence. Consider extracting a shared utility (or reusing an existing loader) so all SPI loading follows one implementation. ########## server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java: ########## @@ -576,6 +584,86 @@ public Response handle(OperationType op, String group, String metalake, Exceptio } } + private static class IdpUserExceptionHandler extends BaseExceptionHandler { + + private static final ExceptionHandler INSTANCE = new IdpUserExceptionHandler(); + + private static String getUserErrorMsg(String user, String operation, String reason) { + return String.format( + "Failed to operate built-in IdP user %s operation [%s], reason [%s]", + user, operation, reason); + } + + @Override + public Response handle(OperationType op, String user, String ignored, Exception e) { + String formatted = StringUtil.isBlank(user) ? "" : " [" + user + "]"; + String errorMsg = getUserErrorMsg(formatted, op.name(), getErrorMsg(e)); + LOG.warn(errorMsg, e); + + if (e instanceof IllegalArgumentException) { + return Utils.illegalArguments(errorMsg, e); + + } else if (e instanceof NotFoundException) { + return Utils.notFound(errorMsg, e); + + } else if (e instanceof UserAlreadyExistsException) { + return Utils.alreadyExists(errorMsg, e); + + } else if (e instanceof NotInUseException) { + return Utils.notInUse(errorMsg, e); + + } else if (e instanceof ForbiddenException) { + return Utils.forbidden(errorMsg, e); + + } else if (e instanceof UnsupportedOperationException) { + return Utils.unsupportedOperation(errorMsg, e); + + } else { + return Utils.internalError(errorMsg, e); + } + } + } + + private static class IdpGroupExceptionHandler extends BaseExceptionHandler { + + private static final ExceptionHandler INSTANCE = new IdpGroupExceptionHandler(); + + private static String getGroupErrorMsg(String group, String operation, String reason) { + return String.format( + "Failed to operate built-in IdP group %s operation [%s], reason [%s]", + group, operation, reason); + } + + @Override + public Response handle(OperationType op, String group, String ignored, Exception e) { + String formatted = StringUtil.isBlank(group) ? "" : " [" + group + "]"; + String errorMsg = getGroupErrorMsg(formatted, op.name(), getErrorMsg(e)); Review Comment: `formatted` includes a leading space, but `getGroupErrorMsg` also adds a space before the `%s` placeholder, producing double spaces in the final message (e.g., "group [engineering]"). Consider removing the leading space from `formatted` or adjusting the format string. -- 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]
