Copilot commented on code in PR #3029: URL: https://github.com/apache/hugegraph/pull/3029#discussion_r3328920082
########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/exceptionshandler/HugeExceptionMapper.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.hugegraph.api.exceptionshandler; + +import com.alipay.sofa.rpc.log.Logger; +import com.alipay.sofa.rpc.log.LoggerFactory; + +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.ext.ExceptionMapper; +import jakarta.ws.rs.ext.Provider; +import org.apache.hugegraph.HugeException; +import org.apache.hugegraph.rest.response.ApiResponse; + +@Provider +public class HugeExceptionMapper implements ExceptionMapper<HugeException> { Review Comment: This new provider is auto-scanned together with the existing `ExceptionFilter.HugeExceptionMapper` because `ApplicationConfig` scans `org.apache.hugegraph.api`. Registering two `ExceptionMapper<HugeException>` providers makes the server-side error response selection ambiguous and can prevent the new unified envelope from being applied consistently; the existing mapper should be replaced or excluded rather than adding a second one. ########## hugegraph-pd/hg-pd-service/pom.xml: ########## @@ -175,6 +175,7 @@ <mainClass> org.apache.hugegraph.pd.boot.HugePDServer </mainClass> + <classifier>exec</classifier> Review Comment: Adding this classifier changes the boot repackage output to `hg-pd-service-*-exec.jar`, but `hg-pd-dist` depends on the unclassified `hg-pd-service` artifact and its start script runs `${LIB}/hg-pd-service-*.jar`. As written, the distribution can package and execute the thin, non-repackaged jar instead of the Spring Boot executable jar; either keep the executable as the main artifact or update the dist dependency/assembly to use the `exec` classifier. ########## hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/exceptionshandler/PDExceptionMapper.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.hugegraph.pd.rest.exceptionshandler; + +import org.apache.hugegraph.pd.common.PDException; +import org.apache.hugegraph.rest.response.ApiResponse; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; + +@RestControllerAdvice +public class PDExceptionMapper { + + private static final Logger logger = LogManager.getLogger(PDExceptionMapper.class); + + @ExceptionHandler(PDException.class) + public ResponseEntity<ApiResponse<Object>> toResponse(PDException exception) { + + logger.error(exception.getMessage(), exception); + + HttpStatus status = resolveStatus(exception.getErrorCode()); + String reasonPhrase = status.getReasonPhrase(); + + ApiResponse<Object> apiResponse = new ApiResponse<>( + exception.getErrorCode(), + exception.getMessage(), + null, + reasonPhrase); + + return ResponseEntity + .status(status) + .body(apiResponse); + + } + + private HttpStatus resolveStatus(int code) { + try { + // Tenta mapear códigos HTTP exatos (ex: 400, 404, 500) + return HttpStatus.valueOf(code); + } catch (IllegalArgumentException e) { + // Se falhar (ex: 4001), extraímos o primeiro dígito para descobrir a família do erro + String codeStr = String.valueOf(code); + + if (codeStr.startsWith("4")) { + return HttpStatus.BAD_REQUEST; // Erros de cliente -> 400 + } + + // Tudo que for da família 5000 ou não reconhecido vira Erro de Servidor -> 500 + return HttpStatus.INTERNAL_SERVER_ERROR; + } + } Review Comment: `HttpStatus.valueOf(code)` treats existing PD business error codes such as `NOT_LEADER = 100` and `STORE_ID_NOT_EXIST = 101` as valid HTTP 1xx statuses, so uncaught PD errors can be returned with non-error HTTP responses instead of the intended error envelope. Only actual HTTP error codes should be passed through; PD-specific codes should be mapped to 4xx/5xx families explicitly. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
