[ZEPPELIN-6356] Fix ZeppelinClientIntegrationTest

[tbonelee]

What is this PR for?

After #5090, we started hiding unhandled error details from clients, which caused some tests that depended on those details to fail. The afffected cases are note imports/creations that should fail when the target note path already exists.

This PR introduces a ConflictException and updates NotebookRestApi to return HTTP 409 (Conflict) when NotebookService throws or returns NotePathAlreadyExistsException.

I chose to do this exception-to-HTTP mapping in NotebookRestApi because NotebookService is also used outside REST contexts, and assigning HTTP status codes belongs in the REST layer.

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-6356

How should this be tested?

• Check if zeppelin-integration-test and other tests pass

Screenshots (if appropriate)

Questions:

• Does the license files need to update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? No

[tbonelee]

@Reamer When you have a moment, could you please take a look at this?

[Reamer]

I thought long and hard about how to solve this problem.
There are various exceptions in org.apache.zeppelin.rest.exception that inherit from WebApplicationException, and there are mappings in the code in various places. In my opinion, this is somewhat unnecessary, as we have an ExceptionMapper. It currently has an unattractive name, WebApplicationExceptionMapper, but it is a mapper for exceptions from the code to return a response.

I would rather solve the problem this way.

--- a/zeppelin-server/src/main/java/org/apache/zeppelin/rest/exception/WebApplicationExceptionMapper.java
+++ b/zeppelin-server/src/main/java/org/apache/zeppelin/rest/exception/WebApplicationExceptionMapper.java
@@ -22,18 +22,22 @@ import jakarta.ws.rs.core.Response;
 import jakarta.ws.rs.ext.ExceptionMapper;
 import jakarta.ws.rs.ext.Provider;
 
+import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
+import org.apache.zeppelin.utils.ExceptionUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 @Provider
 public class WebApplicationExceptionMapper implements ExceptionMapper<Throwable> {
-    
+
   private static final Logger LOGGER = LoggerFactory.getLogger(WebApplicationExceptionMapper.class);
 
   @Override
   public Response toResponse(Throwable exception) {
     if (exception instanceof WebApplicationException) {
       return ((WebApplicationException) exception).getResponse();
+    } else if (exception instanceof NotePathAlreadyExistsException) {
+      return ExceptionUtils.jsonResponseContent(Response.Status.CONFLICT, exception.getMessage());
     } else {
       LOGGER.error("Error response", exception);
       // Return generic error message to prevent information disclosure

[tbonelee]

Understood. So you prefer letting exceptions be thrown at the source and then handling them centrally in WebApplicationExceptionMapper wherever possible? I'm generally used to preventing service-layer exceptions from bubbling past the controller (except in unexpected cases), which is why I initially did it that way. But I don't have a strong preference.
I've updated the code as you suggested. Thanks for the thoughtful feedback!

[Reamer]

All that's missing is the removal of an unnecessary class, otherwise it looks good to me. This change is, of course, a breaking change and will therefore only be merged into the master.

[Reamer]

be thrown at the source and then handling them centrally in WebApplicationExceptionMapper wherever possible?

Yes, I hope this simplifies the code and leads to a uniform API response.

[tbonelee]

@Reamer I removed that class. I also realized I merged the previous PR, which caused the test failure, into branch-0.12. Since it is a breaking change, should I revert that commit to keep the 0.12 line stable? If you agree, I will open a revert PR targeting branch-0.12. Sorry for the oversight.

[tbonelee]

I noticed the tests aren't fixed yet. I'll correct them and re-request review. Sorry for the confusion.

[Reamer]

@Reamer I removed that class. I also realized I merged the previous PR, which caused the test failure, into branch-0.12. Since it is a breaking change, should I revert that commit to keep the 0.12 line stable? If you agree, I will open a revert PR targeting branch-0.12. Sorry for the oversight.

No, that's not necessary. We'll merge this change as well and accept the breaking change.

[Reamer]

LGTM

[tbonelee]

Thansk, I merged this into master and also the branch-0.12 as you suggested