[GitHub] [sling-org-apache-sling-distribution-core] joerghoh commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses

[GitBox <gi...@apache.org>]

joerghoh commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887051860


##########
src/main/java/org/apache/sling/distribution/servlet/DistributionAgentServlet.java:
##########
@@ -71,7 +73,7 @@ protected void doPost(SlingHttpServletRequest request, SlingHttpServletResponse
                 log.debug("distribution response : {}", distributionResponse);
             } catch (Throwable e) {
                 log.error("an unexpected error has occurred", e);
-                ServletJsonUtils.writeJson(response, 503, "an unexpected error has occurred", null);
+                ServletJsonUtils.writeJson(response, 503, e.getMessage(), null);

Review Comment:
   I don't think that this statuscode is correct, the service is not unavailable. There was just an (unqualified) exception, and we don't know what exactly happened just based on the exception type. Is it possible to narrow down the exception type and then provide a better matching status code?
   
   Also I don't think if it's a good idea to expose the exception message potentially containing data which should not be exposed.



##########
src/main/java/org/apache/sling/distribution/servlet/DistributionPackageImporterServlet.java:
##########
@@ -126,8 +127,10 @@ protected void doPost(SlingHttpServletRequest request, SlingHttpServletResponse
             ServletJsonUtils.writeJson(response, SC_OK, "package imported successfully", null);
 
         } catch (final Throwable e) {
-            ServletJsonUtils.writeJson(response, SC_INTERNAL_SERVER_ERROR, "an unexpected error has occurred during distribution import", null);
-            log.error("Error during distribution import", e);
+            String msg = format("an unexpected error has occurred during distribution import. Error:%s",
+                    e.getMessage());
+            log.error(msg, e);
+            ServletJsonUtils.writeJson(response, SC_INTERNAL_SERVER_ERROR, msg, null);

Review Comment:
   I think that a statuscode 500 is wrong here, because it's not an internal server error. It's a situation which is handled by this code, therefor controlled. 
   As above: Can we make the exception more narrow and just capture a specific type, and therefor find a better statuscode for it? Same for the exception 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: dev-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org