Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpException's message does not return a proper PayoutError POJO #14

Closed
hadjiski opened this issue Dec 20, 2020 · 8 comments · Fixed by #20
Closed

HttpException's message does not return a proper PayoutError POJO #14

hadjiski opened this issue Dec 20, 2020 · 8 comments · Fixed by #20

Comments

@hadjiski
Copy link
Contributor

hadjiski commented Dec 20, 2020

I see the API deserializes json strings into POJOs (e.g CreatePayoutResponse) using a custom com.paypal.http.annotations.SerializedName annotation and simple java reflections technique.

OK, but then could you also make the thrown com.paypal.http.exceptions.HttpException also return a deserialized POJO, when .getMessage() is called, because an auto deserialization via Jackson for example is not working, since POJO field names and serialized json string key names are not matching to each other - ok it is possible indirectly by extending PayoutError and PayoutErrorDetails and annotating the fields with the Json deserialization library of choice, but not so nice:

@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_NULL)
public class JsonDeserializablePayoutError extends PayoutError {

    @JsonProperty
    private String name;

    @JsonProperty
    private String message;

    @JsonProperty("details")
    private List<JsonDeserializablePayoutErrorDetails> payoutErrorDetails;

...

@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_NULL)
public class JsonDeserializablePayoutErrorDetails extends PayoutErrorDetails {

    @JsonProperty
    private String field;

    @JsonProperty
    private String issue;

So it looks like the suitable POJO is the com.paypal.payouts.PayoutError

These type of messages of HttpException I saw till now and they seem to fit exactly the PayoutError POJO:

{
  "name": "INSUFFICIENT_FUNDS",
  "message": "Sender does not have sufficient funds. Please add funds and retry.",
  "debug_id": "6d22bad65aefc",
  "information_link": "https://developer.paypal.com/docs/api/payments.payouts-batch/#errors",
  "links": []
}
{
  "name": "VALIDATION_ERROR",
  "message": "Invalid request - see details",
  "debug_id": "9a66b8363a86f",
  "information_link": "https://developer.paypal.com/docs/api/payments.payouts-batch/#errors",
  "details": [
    {
      "field": "items[0].receiver",
      "location": "body",
      "issue": "Receiver is invalid or does not match with type"
    }
  ],
  "links": []
}
@rajkumarpalani
Copy link
Contributor

rajkumarpalani commented Feb 10, 2021

@hadjiski paypalhttp allows only a success type to be deserialised, for errors the Encoder from the paypalhttp can be used to deserialise the error type.

       try { 
           HttpResponse<CreatePayoutResponse> response = client.execute(request);  
        } catch (HttpException e) {  
            String error = e.getMessage();  
            Encoder encoder = new Encoder();  
            Error payoutError = encoder.deserializeResponse(new ByteArrayInputStream(error.getBytes(StandardCharsets.UTF_8)), Error.class, e.headers());  
            System.out.println(payoutError);  
        }  

will update samples and readme to reflect the same

@hadjiski
Copy link
Contributor Author

hadjiski commented Feb 10, 2021

@rajkumarpalani thank you for sharing this possible solution. From it I derive that the PayoutError class is indeed the right POJO to use when deserializing the com.paypal.http.exceptions.HttpException.
Then why the need to write such a not so elegant try/catch code block, when this can be provided from the paypalhttp:
Either via enhancing the API to also deserialize errors
Or via providing a helper method to pass the exception and get the POJO, instead of passing these input streams to the deserializeResponse method etc.

The current workaround solution from above, which I am currently using is a bit more elegant I would say:
PayoutError payoutError = new ObjectMapper().readValue(httpException.getMessage(), JsonDeserializablePayoutError.class)

@rajkumarpalani
Copy link
Contributor

@hadjiski updated comment to reflect that Error has to be used instead of PayoutError. I do agree that it would be nice to have that readily available in a deserialised format from the HttpException.

Created an issue on paypalhttp, will try to raise a PR to address the same - paypal/paypalhttp_java#5

@hadjiski
Copy link
Contributor Author

hadjiski commented Feb 10, 2021

Error has to be used instead of PayoutError

@rajkumarpalani currently both are identical from content perspective, but good to know for the future.
(1) Btw. what is the semantic difference between both?
My assumption was, that when calling PayoutsPostRequest I can expect the HttpException to be from a payout error type.
(2) If you are saying the api exception would hold the more general Error type, then what is the purpose of the PayoutError, in which use case one can expect it?
Currently I can see it as part of the CreatePayoutResponse, but you said there it is actually deprecated in a successful case and in an error case, we get the exception thrown, so back to question (2)?

@rajkumarpalani
Copy link
Contributor

rajkumarpalani commented Feb 11, 2021

@hadjiski Yes, both PayoutError and Error looks similar right now with minor difference in PayoutError.ErrorDetails.links which is missing in the Error.

Going to cleanup the error classes and will provide a much simpler version. The current Error structure is missing a field location in the ErrorDetails which you can notice in the current API error response. Expect a new release with all the changes sometime next week

@rajkumarpalani rajkumarpalani linked a pull request Feb 15, 2021 that will close this issue
@rajkumarpalani
Copy link
Contributor

@hadjiski Payouts SDK v1.1.0 released with most of the errors cleaned up. Check out the changelog - https://github.com/paypal/Payouts-Java-SDK/blob/master/CHANGELOG.md

@hadjiski
Copy link
Contributor Author

@rajkumarpalani thanks a lot for the quick release, btw. do you still plan to further simplify the error handling skipping the explicit de-serialization part or is this kind of the final solution now?

@rajkumarpalani
Copy link
Contributor

rajkumarpalani commented Feb 26, 2021

@hadjiski I started working on a PR to get typed entity out of HttpException and realised that exceptions cannot be generic in Java. This restricts the possibility of getting a typed entity out of the HttpException as expected out of the issue paypal/paypalhttp_java#5

The options are to emit exception as a return type such as,

HttpResponse<Pair<Optional<CreatePayoutResponse>, Optional<Error>>> response = client.execute(request);

This allows the client to populate the entity as per the result, but this involves huge changes for existing integrations and also doesn't align with the current pattern that paypalhttp library adopts across all supported languages. The problem seems to be specific on strongly typed languages like Java.

Am hoping to catch up with the paypalhttp team and bring up the issue and update here on the proceedings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants