adobe / aio-lib-java-cloudmanager

Java wrapper to the Adobe Cloud Manager API.
https://opensource.adobe.com/aio-lib-java-cloudmanager/
Apache License 2.0
3 stars 9 forks source link

Expose more metadata from CloudManagerApiException #16

Closed kwin closed 3 years ago

kwin commented 3 years ago

Currently all meta information being passed to the constructor is only used to construct an error message (https://github.com/adobe/aio-lib-java-cloudmanager/blob/923e6718cd72941707ea98de800e61ad2877efc3/src/main/java/io/adobe/cloudmanager/CloudManagerApiException.java#L52). Sometimes it would be useful for consumers to get the http status code after the exception has been caught. I propose to also store at least the HTTP status code as field and expose it via a getter. Otherwise error handling apart from logging will be difficult to achieve (because parsing of the error message is necessary then).

bstopp commented 3 years ago

I disagree with this. The error codes for the underlying requests are an implementation detail that shouldn't be a concern of the caller.

If the error messages do not provide enough information to indicate what has occurred and what the remediation steps are, another approach would be needed. Possibly using different type of exception sub-classes would be the proper way to communicate what is "recoverable" vs "unrecoverable".

I'd like @justinedelson to weigh in though.

kwin commented 3 years ago

For me the response status codes are part of the API contract and therefore crucial to expose in the exception. (They are actually part of the swagger API documentation). Leave it up to the caller to decide which failure is recoverable and unrecoverable. That is easy to do once you know the status code.

To give a concrete example: CloudManagerApi.startExecution might return a 412 (https://www.adobe.io/apis/experiencecloud/cloud-manager/api-reference.html#/Pipeline_Execution/startPipeline) in case the pipeline is already running. In this case the caller could try to first stop the current pipeline with another API call.

bstopp commented 3 years ago

Right, but 412 is the implementation detail that you, as the caller, shouldn't be concerned with.

What you'd rather handle is an PipelineRunningException. Because the error code could change, the exception would not.

kwin commented 3 years ago

Again, 412 is part of the API spec. Adobe cannot change it without breaking the ReST API contract. Http status codes are integral part of each ReST API!

Quote from https://www.infoworld.com/article/3401920/how-to-make-your-rest-apis-backward-compatible.html#:~:text=An%20API%20is%20backward%20compatible,future%20versions%20of%20the%20API.

Never change the behavior of HTTP response codes

or https://zubialevich.blogspot.com/2018/09/backward-compatibility.html

Never delete or modify existing HTTP Response code behavior

justinedelson commented 3 years ago

AFAICT, the status code (and other info from the response) is already gettable as the original ApiException is available via getCause().

bstopp commented 3 years ago

Only downside to that is you have to do a blind cast to convert the method result to an ApiException

justinedelson commented 3 years ago

Doesn't have to be blind :) But yes, certainly could be improved.

kwin commented 3 years ago

What speaks against exposing just the http status code as additional getter?