You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2022/06/01 13:39:12 UTC
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia opened a new pull request, #59: SLING-11366: Provide more exception error context on the JSON API responses
jose-correia opened a new pull request, #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59
The API is currently responding with a very generic message when packages fail to be distributed.
```
{
"message": "an unexpected error has occurred"
}
```
This should be improved since there are some errors that are due to bad usage, eg: packages that are too big, and we should to provide more context to the client:
```
{
"message": "an unexpected error has occurred",
"details": "Failed to create content package for requestType=ADD, paths=[/content]. Error=Can't distribute package with size greater than 1 Byte, actual 4053"
}
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887647886
##########
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:
@cschneider Its seems like implementing the error codes pattern is looking more and more useful. This could be solved by having something like:
```java
public enum Error {
PACKAGE_SIZE(0, "Package size exceeds the limit threshold.", 400),
AUTHORIZATION(1, "Not authorized to perform action", 401);
INTERNAL_ERROR(2, "An error has occurred", 500);
....
}
public class DistributionException extends Exception {
public Error errorCode;
}
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r888746447
##########
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 decided to implement an initial approach to a more structured way of having well-defined external error codes and improve how we propagate the exception metadata. What do you think?
[https://github.com/apache/sling-org-apache-sling-distribution-core/pull/60](https://github.com/apache/sling-org-apache-sling-distribution-core/pull/60)
--
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
[GitHub] [sling-org-apache-sling-distribution-core] tmaret commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
tmaret commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1143693721
Good point. It makes sense to replace the generic error message with the specific one, using the existing `message` property.
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887636626
##########
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:
Agree that the status code is not ideal. However, I think Christian is right. I will track this in a JIRA issue, to be assessed later, since it falls outside of the main purpose of this PR.
Regarding the exception narrowing, even though I agree, the current implementation makes it very hard to do such a thing. If you look at the [code that we are executing](https://github.com/apache/sling-org-apache-sling-distribution-journal/blob/master/src/main/java/org/apache/sling/distribution/journal/impl/publisher/DistributionPublisher.java#L267) it is catching a generic `Exception` and raising this `DistributionException`, which can contain any type of error message, and we cannot be sure which.
We need to discuss how can we include maybe more properties in this `DistributionException` which would help an upper module decide the severity and type of error.
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887976156
##########
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:
Created Jira - [Wrong 503-unavailable status code in Distribution API responses](https://issues.apache.org/jira/browse/SLING-11371)
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887636626
##########
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:
Agree that the status code is not ideal. However, I think Christian is right. I will track this in a JIRA issue, to be assessed later, since it falls outside of the main purpose of this PR.
Regarding the exception narrowing, even though I agree, the current implementation makes it very hard to do such a thing. If you look at the [code that we are executing](https://github.com/apache/sling-org-apache-sling-distribution-journal/blob/master/src/main/java/org/apache/sling/distribution/journal/impl/publisher/DistributionPublisher.java#L267) it is catching a generic exception and raising this DistributionException, which can contain any type of error message, and we cannot be sure which.
We need to discuss how can we include maybe more metadata in this `DistributionException` which would help an upper module decide the severity and type of error.
--
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
[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
Posted by 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
[GitHub] [sling-org-apache-sling-distribution-core] cschneider commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
cschneider commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887553854
##########
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 currently we have no means of determining if the error is because of bad data sent by the client or because of some internal problem.
So in my opinion 500 is ok. I also think current client would not be prepared to distinguish different status codes.
Do you have a concrete proposal how to improve the code?
--
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
[GitHub] [sling-org-apache-sling-distribution-core] cschneider commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
cschneider commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1143683365
I don't think this makes sense.
In the old code "message" is always exactly the same string that has zero information. "an unexpected error has occurred" does not help the user at all.
--
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
[GitHub] [sling-org-apache-sling-distribution-core] tmaret commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
tmaret commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1143656095
I think it makes sense to keep the message and the cause in two distinct properties. To align naming with java exceptions we may use
```
{
"message": "an unexpected error has occurred",
"cause": "Failed to create content package for requestType=ADD, paths=[/content]. Error=Can't distribute package with size greater than 1 Byte, actual 4053"
}
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] sonarcloud[bot] commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1143632852
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [sling-org-apache-sling-distribution-core] tmaret commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
tmaret commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887645261
##########
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:
> Also I don't think if it's a good idea to expose the exception message potentially containing data which should not be exposed.
+1. We can make the error more specific. In this case, I think that ideally the message should be
```
Failed to create content package: Can't distribute package with size greater than 1 Byte, actual 4053
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887636626
##########
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:
Agree that the status code is not ideal. However, I think Christian is right. I will track this in a JIRA issue, to be assessed later, since it falls outside of the main purpose of this PR.
Regarding the exception narrowing, even though I agree, the current implementation makes it very hard to do such a thing. If you look at the [code that we are executing](https://github.com/apache/sling-org-apache-sling-distribution-journal/blob/master/src/main/java/org/apache/sling/distribution/journal/impl/publisher/DistributionPublisher.java#L267) it is catching a generic `Exception` and raising this `DistributionException`, which can contain any type of error message, and we cannot be sure which.
We need to discuss how can we include maybe more metadata in this `DistributionException` which would help an upper module decide the severity and type of error.
--
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
[GitHub] [sling-org-apache-sling-distribution-core] cschneider commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
cschneider commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887551410
##########
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 agree. Statuscode 503 is not suitable. On the other hand this is returning 503 for a long time now. We might break clients if we change it.
--
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
[GitHub] [sling-org-apache-sling-distribution-core] cschneider commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
cschneider commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r889025653
##########
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",
Review Comment:
One very small change request: There should be a space in front of %s I think.
--
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
[GitHub] [sling-org-apache-sling-distribution-core] cschneider merged pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
cschneider merged PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887647886
##########
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:
@cschneider Its seems like implementing the error codes pattern you mentioned is looking more and more useful. This could be solved by having something like:
```java
public enum Error {
PACKAGE_SIZE(0, "Package size exceeds the limit threshold.", 400),
AUTHORIZATION(1, "Not authorized to perform action", 401);
INTERNAL_ERROR(2, "An error has occurred", 500);
....
}
public class DistributionException extends Exception {
public Error errorCode;
}
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887647886
##########
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:
@cschneider Its seems like implementing the error codes pattern you mentioned is looking more and more useful. This could be solved by having something like:
```java
public enum Error {
PACKAGE_SIZE(0, "Package size exceeds the limit threshold.", 400),
AUTHORIZATION(1, "Not authorized to perform action", 401);
INTERNAL_ERROR(2, "An error has occurred", 500);
....
}
public class DistributionException extends Exception {
public Error errorCode;
....
}
```
##########
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:
@cschneider Its seems like implementing the error codes pattern you mentioned is looking more and more useful. This could be solved by having something like:
```java
public enum Error {
PACKAGE_SIZE(0, "Package size exceeds the limit threshold.", 400),
AUTHORIZATION(1, "Not authorized to perform action", 401);
INTERNAL_ERROR(2, "An error has occurred", 500);
....
}
public class DistributionException extends Exception {
public Error errorCode;
....
}
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] sonarcloud[bot] commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1146059113
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [sling-org-apache-sling-distribution-core] sonarcloud[bot] commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1143892206
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-core&pullRequest=59&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia closed pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia closed pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59
--
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
[GitHub] [sling-org-apache-sling-distribution-core] jose-correia commented on a diff in pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
jose-correia commented on code in PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#discussion_r887647886
##########
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:
@cschneider Its seems like implementing the error codes pattern is looking more and more useful. This could be solved by having something like:
```java
public enum Error {
PACKAGE_SIZE(0, "Package size exceeds the limit threshold."),
AUTHORIZATION(1, "Not authorized to perform action");
INTERNAL_ERROR(2, "An error has occured");
....
}
public class DistributionException extends Exception {
public Error errorCode;
}
```
--
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
[GitHub] [sling-org-apache-sling-distribution-core] cschneider commented on pull request #59: SLING-11366: Provide more exception error context on the JSON API responses
Posted by GitBox <gi...@apache.org>.
cschneider commented on PR #59:
URL: https://github.com/apache/sling-org-apache-sling-distribution-core/pull/59#issuecomment-1146048874
Let's separate the status codes from this change. Jose plans to work on that in a separate jira.
--
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