kevdoran commented on a change in pull request #230: NIFIREG-321 Integrate revision concept into UI, REST API, and service… URL: https://github.com/apache/nifi-registry/pull/230#discussion_r334152158
########## File path: nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/mapper/InvalidRevisionExceptionMapper.java ########## @@ -0,0 +1,48 @@ +/* + * 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.nifi.registry.web.mapper; + +import org.apache.commons.lang3.StringUtils; +import org.apache.nifi.registry.revision.api.InvalidRevisionException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.ws.rs.core.Response; +import javax.ws.rs.ext.ExceptionMapper; +import javax.ws.rs.ext.Provider; + +/** + * Maps invalid revision exceptions into client responses. + */ +@Provider +public class InvalidRevisionExceptionMapper implements ExceptionMapper<InvalidRevisionException> { + + private static final Logger logger = LoggerFactory.getLogger(InvalidRevisionExceptionMapper.class); + + @Override + public Response toResponse(InvalidRevisionException exception) { + // log the error + logger.info(String.format("%s. Returning %s response.", exception, Response.Status.BAD_REQUEST)); + + if (logger.isDebugEnabled()) { + logger.debug(StringUtils.EMPTY, exception); + } + + return Response.status(Response.Status.BAD_REQUEST).entity(exception.getMessage()).type("text/plain").build(); Review comment: Poking around with this I noticed a couple things 1. Minor, but the response status of `409 Conflict` might be more useful than `400 Bad Request`, as the client could check for 409 and know that they need to pull for revision to retry their update or whatever vs 400 which could be due to a lot of reason that may not allow for auto retry. This is based on the usage I saw in `NaiveRevisionManager`. It was unclear to me if the `InvalidRevisionException` thrown in `JDBCRevisionManager` would always be for the same reason (did not match current) or could be for other types of validation failure. I suppose if there are multiple cases we might need to use 400 or introduce multiple exception types. 2. Related to 1, the error message sent to the client _"Invalid Revision for entity..."_ is difficult to understand. Invalid to me sort of implies malformed request object. If it is stale/out-of-date/old, and we know that is the cause of the exception, I would prefer we say that rather than invalid. If we cannot be sure (ie, the exception could be due to invalid or stale, than I'm ok with leaving as-is. 3. Do we need an exception mapper for `ExpiredRevisionClaimException`? Based on the use of it I saw in deleteRevision(), it looks like it would probably map to a 404? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
