Copilot commented on code in PR #771: URL: https://github.com/apache/unomi/pull/771#discussion_r3390559281
########## rest/src/main/java/org/apache/unomi/rest/exception/AbstractRestExceptionMapper.java: ########## @@ -0,0 +1,155 @@ +/* + * 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.unomi.rest.exception; + +import com.fasterxml.jackson.core.JsonParseException; +import com.fasterxml.jackson.databind.JsonMappingException; +import org.apache.cxf.jaxrs.utils.JAXRSUtils; +import org.apache.cxf.message.Message; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.UriInfo; +import java.util.HashMap; +import java.util.Map; + +/** + * Base class for the REST {@code ExceptionMapper}s, factoring out the behaviour they share: + * building a sanitized request context for logging, walking to a root cause, detecting JSON + * deserialization failures, and producing the standard JSON error responses. + */ +public abstract class AbstractRestExceptionMapper { + + private static final Logger LOGGER = LoggerFactory.getLogger(AbstractRestExceptionMapper.class.getName()); + + private static final String ERROR_MESSAGE_KEY = "errorMessage"; + + /** + * @return a {@code 400 Bad Request} JSON response: {@code {"errorMessage":"badRequest"}} + */ + protected Response badRequestResponse() { + return jsonErrorResponse(Response.Status.BAD_REQUEST, "badRequest"); + } + + /** + * @return a {@code 500 Internal Server Error} JSON response: {@code {"errorMessage":"internalServerError"}} + */ + protected Response internalServerErrorResponse() { + return jsonErrorResponse(Response.Status.INTERNAL_SERVER_ERROR, "internalServerError"); + } + + private Response jsonErrorResponse(Response.Status status, String errorMessage) { + Map<String, Object> body = new HashMap<>(); + body.put(ERROR_MESSAGE_KEY, errorMessage); + return Response.status(status).header("Content-Type", MediaType.APPLICATION_JSON).entity(body).build(); + } + + /** + * @return {@code true} when the given root cause is a Jackson deserialization failure, i.e. a + * client error (malformed/mistyped request body) rather than a genuine server fault. + */ + protected boolean isJsonDeserializationError(Throwable rootCause) { + return rootCause instanceof JsonMappingException || rootCause instanceof JsonParseException; + } + + protected Throwable getRootCause(Throwable throwable) { + if (throwable == null) { + return null; + } + Throwable cause = throwable.getCause(); + if (cause == null || cause == throwable) { + return throwable; + } + return getRootCause(cause); + } Review Comment: `getRootCause` is implemented recursively without cycle detection. If an exception cause chain ever contains a cycle (e.g., A→B→A via `initCause`), this will recurse indefinitely and can trigger a `StackOverflowError` while handling an error. Converting this to an iterative walk with cycle detection avoids that failure mode. ########## rest/pom.xml: ########## @@ -187,6 +187,13 @@ <artifactId>slf4j-api</artifactId> <scope>provided</scope> </dependency> + + <dependency> + <groupId>org.junit.jupiter</groupId> + <artifactId>junit-jupiter</artifactId> + <version>${junit-jupiter.version}</version> + <scope>test</scope> + </dependency> Review Comment: `unomi-rest` imports `unomi-bom` via `<dependencyManagement>`, so the JUnit Jupiter version is already centrally managed (e.g., `services/pom.xml` declares `junit-jupiter` without a `<version>`). Keeping the version here makes it easier for this module to drift from the BOM and increases upgrade overhead. Consider removing the explicit `<version>` and relying on dependencyManagement. ########## rest/src/main/java/org/apache/unomi/rest/exception/InternalServerErrorExceptionMapper.java: ########## @@ -0,0 +1,81 @@ +/* + * 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.unomi.rest.exception; + +import org.osgi.service.component.annotations.Component; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.ws.rs.InternalServerErrorException; +import javax.ws.rs.core.Response; +import javax.ws.rs.ext.ExceptionMapper; +import javax.ws.rs.ext.Provider; + +/** + * Maps {@link InternalServerErrorException}. When the underlying cause is a Jackson deserialization + * failure (a wrapped client error) the response is downgraded to a {@code 400 Bad Request}; + * otherwise it remains a {@code 500 Internal Server Error} with detailed, sanitized logging. + */ +@Provider +@Component(service = ExceptionMapper.class) +public class InternalServerErrorExceptionMapper extends AbstractRestExceptionMapper + implements ExceptionMapper<InternalServerErrorException> { + + private static final Logger LOGGER = LoggerFactory.getLogger(InternalServerErrorExceptionMapper.class.getName()); + + @Override + public Response toResponse(InternalServerErrorException exception) { + String requestContext = buildRequestContext(); + Throwable rootCause = getRootCause(exception); + + // A wrapped JSON deserialization failure is really a client error -> 400 Bad Request. + if (isJsonDeserializationError(rootCause)) { + String errorMessage = LogSanitizer.forLogging(messageOrType(rootCause)); + LOGGER.warn("Bad request on {} - JSON deserialization error: {} (Set InternalServerErrorExceptionMapper to debug to get the full stacktrace)", + requestContext, errorMessage); + LOGGER.debug("Full JSON mapping exception details for request: {}", requestContext, exception); + return badRequestResponse(); + } + + // Genuine server error -> 500 with detailed context. + LOGGER.error("{} (Set InternalServerErrorExceptionMapper to debug to get the full stacktrace)", + buildServerErrorDetails(requestContext, exception, rootCause), exception); + LOGGER.debug("Full exception details for request: {}", requestContext, exception); Review Comment: The ERROR log statement includes `exception` as the last argument, so SLF4J will emit the full stack trace at ERROR level. That conflicts with the message (“set ... to debug to get the full stacktrace”) and duplicates the subsequent DEBUG log. If the intent is stack traces only at DEBUG (consistent with `InvalidRequestExceptionMapper` / `ValidationExceptionMapper`), remove the throwable from the ERROR log (or adjust the message). -- 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]
