debate: PayPal's REST API likes to throw exceptions -- good design/bad design

Question

User-434868552 posted

for my c# ASP.NET MVC5 website, Paradigm Mentors, i decided to use PayPal's new-ish REST API.

as Rob Conery pointed out in 2009, one should at least learn ASP.NET MVC (1).

Rob Conery and others discussed the RESTful nature of ASP.NET MVC in a number of articles (2).

although the PayPal documentation for its REST API imho leaves much to be desired, i was able to get it working. (notes, 3)

one of my core values:  "Learning only has true value when it’s shared."

i created five free videos, notes, and a free sample demo (c#, ASP.NET MVC5, EF 6 Code First).

While testing the sample demo, i discovered a number of api design flaws ... most of these are related to throwing exceptions when imho exceptions should not be thrown.

FWIW, i suspect that part of this weakness is that PayPal's REST API architect(s) want to leverage the same code base over multiple platforms; imho, for a gazillion dollar company like PayPal, that's unnecessary frugality.

Two examples:

(a) for a PayPal transaction to be successful, the ASP.NET MVC needs to get an access token; in fact, the best practice is to get two access tokens per single transaction cycle; unfortunately, whenever the PayPal library fails to retrieve an access token, it clumsily throws an a exception.

(b) the PayPal documentation defines an amount as "digits" or "digits.cents" but fails to clairify that 1234.56 is valid while for 01234.56 it throws an exception; hmmm ... seems to me that zero is also a digit.

instead of throwing unnecessary exceptions which can kill a production application, imho PayPal needs methods with signatures like this:

public Int32 PayPalRESTapiMethodX(p1, p2, ... , out List<String> errorList);
 // return zero for success; else error code

From my point of view, the best generic design principle is "NEVER, if at all possible, throw an exception".

imho, throwing unnecessary exceptions is an absence of grace.

references:

1.  http://www.wekeroad.com/2009/04/22/i-spose-ill-just-say-it-you-should-learn-mvc/

2. "asp.net mcv restful" http://lmgtfy.com/?q=asp.net+mcv+restful

3. notes&links:  https://g47.org/cash

Answer 1

User-1611549905 posted

No, throwing an exception is the correct thing to do. An exception means that the method that threw it could not do what its name says it does. Whether the method in question was a call to a remote service or something within your own application is irrelevant. There could be all sorts of reasons why the method might fail: loss of network connectivity, out of memory, out of disk space, or invalid input being supplied to the method. Many of these reasons are outside of your (or their) control.

The correct assumption to make in such a case is that unless you knew exactly what caused the exception and how to recover from it, the method's caller can not do what its name says that it does either. Therefore, the exception should be propagated up the call stack.

At the topmost level of your code, you would therefore have a global exception handler that presents an error message to the user informing them that something went wrong.

If you are able to meaningfully recover from the exception, you should handle it in a try...catch block. If you need to roll back transactions or otherwise clean up, you should handle that in a try...finally block.

Returning a status code rather than throwing an exception introduces a different risk: that you don't handle an error condition that you need to know about but just continue regardless. This would result in data corruption or (more likely) incorrect data being saved to your database. Think, someone being charged for something that never arrives, or someone receiving something of high value that they don't get charged for.

A similar error is catching all exceptions:

try {
    DoSomething();
}
catch {
    // do nothing
}

This is called "Pokemon exception handling" (gotta catch them all) and introduces a similar risk. Don't do it.

Answer 2

User-434868552 posted

@jammycakes  i strongly disagree with you.

(a) exceptions should be only a last result, not an easy way out for the method coder.

(b) API design should allow us to avoid unnecessary exceptions; for example, the .NET Framework .TryParse methods.

(c) code should behave as described in the API.

Example:  https://developer.paypal.com/webapps/developer/docs/api/

https://developer.paypal.com/webapps/developer/docs/api/#create-a-payment

https://developer.paypal.com/webapps/developer/docs/api/#transaction-object

https://developer.paypal.com/webapps/developer/docs/api/#amount-object

total     string

Total amount charged from the payer to the payee.

In case of a refund, this is the refunded amount to the original payer from the payee.

10 characters max with support for 2 decimal places. Required.

123.45 meets the above definition.  no problem.

0123.45 (leading zero) also meets the above definition -- the PayPal .dll throws an exception.

This exception is NOT documented by PayPal.

(PayPal has finally updated its code; i've not checked yet whether the newest code is any better.)

(d) API documentation should describe all exceptions that the method throws.  See for example MSDN .NET Framework documention.

the exception should be propagated up the call stack.

No, if MyMethod calls YourMethod and you throw an exception, then it is my responsibility to handle your exception gracefully.  Let me explain.

MyMethod has a purpose that is whatever; when your method throws an exception, it is MyMethod's responsibility to take appropriate action in order to deal with the fact that whatever is not achievable.

PayPal REST API example:  PayPal needs to provide me with a token that allows me to create a payment (actually, an invoice) -- rather than designing its API to return an indication of failure with a signature that returns an Int32 result code (zero == success) and an out token, PayPal's REST API throws an unnecessary exception imho.

https://developer.paypal.com/webapps/developer/docs/api/#errors

imagine you are on my website trying to purchase a life membership; my graceful choice is to display some graceful message like:

"error connecting to PayPal, please try again later"

rather than the inelegant non-choice

Server Error in '/' Application.

Returning a status code rather than throwing an exception introduces a different risk:

that you don't handle an error condition that you need to know about but just continue regardless.

given a well documented API, example Int32.TryParse Method (String, Int32), http://msdn.microsoft.com/en-us/library/f02979c7%28v=vs.110%29.aspx,

Return Value
Type: System.Boolean
true if s was converted successfully; otherwise, false.

it is absolute folly to fail to evalutate the return value.

API Design Obligation:  avoid throwing unnecessary exceptions

API user obligation:  to never ignore documented behaviour, including return codes

jammycakes, on one thing we mostly agree, a "do nothing" response is almost always a poor choice (there are exceptions, but few); imho, "do nothing" likely indicates laziness on the part of the coder.

Answer 3

User-1611549905 posted

This is not about making exceptions an easy way out of anything. This is about establishing exceptions as having a clear, consistent and unambiguous meaning. Your points about documentation are valid, but documentation can and should also take the form of adherence to conventions, and especially to established conventions, wherever possible.

That's why I say that an exception means that your method can not do what its name says that it does. It is clear, consistent, unambiguous, easy to understand, unsurprising, and what well designed APIs do in languages that support structured exception handling anyway.

Methods with names starting with "Try..." do actually follow this pattern, by establishing a convention that "Try" here means "Return false if and only if the method is provided with invalid user input or a request for a resource which does not exist."

The problem with saying things such as "don't throw unnecessary exceptions" or "exceptions should only be a last resort" or "only throw exceptions in cases that are exceptional" is that it is too vague and ambiguous to be practical, for the simple reason that it fails to define what exactly constitutes an exceptional situation. Given that guideline, you will end up with a codebase where one method throws an exception and another method returns null in response to exactly the same failure condition.

Now about handling exceptions: it's important to realise that in a language such as C#, a lack of exception handling code does not mean "do nothing." It means "take the default action" -- abort the method invocation and bubble the exception up to the caller. In 90% of cases, this is the correct action to take. ("Doing nothing" would mean wrapping each statement in a try { } block with an empty catch { } clause.) The counterexample that you gave -- showing a message to the user -- is something you would handle in the UI layer. It would not normally be appropriate to handle this in the business layer because if you did, you may well end up with your method's caller assuming that it had completed correctly when in actual fact it hadn't, and saving incorrect information to the database as a result. The whole point here is that while there will be exceptions to the rule, they will be very much that: exceptions to the rule.

This brings me to another point about APIs. A well designed API will reduce or eliminate the need for repetitive boilerplate code or cross-cutting concerns to support it. With return values, you have to follow every method call with a check on the return code. This isn't about laziness: it's about robustness. The more ceremony and repetition you impose on your callers, the greater the risk that they will make a mistake. Additionally, a well designed API will stick to the idioms of the language and/or framework that it is using. Adopting a different approach, such as returning error codes in a language that uses structured exception handling, is unexpected and will dramatically increase the risk of your users getting things wrong.

Finally, the problem that you are having with PayPal appears to be a bug, pure and simple. Either that, or else it's being overly pedantic about what it can and can't accept, but since that's undocumented, I'd call it a bug anyway.

Answer 4

User753101303 posted

Hi,

Actually returning a status code is what we had first before the general move to exception handling. The main problem for status code is likely that you have then to *explicitely* test for the status code for each and every API call you are doing else the error is ignored and will manifest itself later in the code. And you have no other choice than to handle this status code at each call.

 Witch exceptions, if something is wrong the code fails by default and you can handle those failures at any level you want. Seems cleaner and more flexible to me.

As for those samples :

#1: not sure why it is clumsy. What you asked for doesn't work so this is an error. It dépends rather why it fails. It is some kind of temporary condition?
#2: debatable, on the paypal side it seems they don't accept leading 0s. Not sure how you transmit this value to paypal but it's likely best to always use the native type rahter than a string (ie on your side your PayPal client should accept a decimal rather than a string and then you just couldn't do that). Seems unrelated to the status code vs exception debate.

Try http://nedbatchelder.com/text/exceptions-vs-status.html

If  not done you may want to ask a question about the problem you seems to have to handle those exceptions.

Some of this feedback would be better placed in PayPal support forum as it seems about PayPal code/documentation issues.

Answer 5

User-434868552 posted

@PatriceSc   

Some of this feedback would be better placed in PayPal support forum as it seems about PayPal code/documentation issues.

No, this has nothing to do specifically with PayPal support; in my own case, i've coded around their deficiencies several months ago; for all intents and purposes, i'm simply using PayPal as a vehicle to describe what i call bad design.

For me, although i disagree, i'm sensing different viewpoints than mine.

Actually returning a status code is what we had first before the general move to exception handling

Newer does not always mean better; i'm not sure that newer even applies.

Example:  given a condition f that causes a method to fail, where it makes sense to return a status, let's say Boolean false, that's substantially simpler that causing an unnecessary exception that leads to unnecessary system generated instantiaton of an instance of the Exception class with substantial overhead.

With exceptions, if something is wrong the code fails by default and you can handle those failures at any level you want. Seems cleaner and more flexible to me.

(1) code never fails by default; code behaves as intended by design.

(2) you can not handle code at any level ... it does not work that way; one handles code at the boundary between MyMethod ~~ YourMethod.  OTOH, one can choose to ignore the interaction the occurs at the boundary between MyMethod ~~ YourMethod, frequently with predictable consequences.

A. the action described by my example (a) at http://forums.asp.net/post/5766770.aspx is imho clumsy because it's bad design that throws an unnecessary exception.

B. No, if a leading zero is not acceptable, it needs to be in the documentation.  Also, PatriceSc you have missed that native types, a.k.a. primitive types, do not cross the internet ... these are all managed by text-based protocals.  Analogy:  a restaurant states "No shirt; no shoes; no service" ~~ a customer arrives wearing only clown shoes and a sleeveless undershirt ~~ that customer is compliant ~~ the restaurant management needs to revise its documentation.  Programmers are not pyshic AFAIK; if a leading zero is not acceptable, then the documentation must explicitly state that fact.

Answer 6

User753101303 posted

gerrylowry

i've coded around their deficiencies

It seems a PayPal issue ??

gerrylowry

Newer does not always mean better

Sure but you may still want to wonder why basically ALL languages moved to exception handling.

gerrylowry

unnecessary exception

If unecessary it is a PayPal issue. If something is wrong you should have an exception and the overhead doesn't matter as this is something that should not happen .

#1 sure but if something wrong happens it should fail by default rather than to keep running by default unless you thought to check each and every call

#2  see again how exception are working. You can handle the exception where it makes more sense.

A. Don"t know the PayPal API. The code could actually achieve the expected result even if the PayPal librairies fails to retrieve the token ?

B. IMO their doc should tell they expect a decimal. But you won't have  this problem if on your side you work with numeric types rather than with strings. This is just  a way to workaround this paypal issue and the doc issue is a PayPal issue as well (and in general it's best to always work with the type that makes more sense).

If you wish you can still wrap the PayPal API and transform those exceptions into status code.

Not sure what it is but I would suggest to definitely post about the actual issues you have (maybe in a PayPal forum. You have that much exceptions? Is this because some kind of service is not reliaable and that doing exactly the same operation a bit later will then work ?)

Answer 7

User-1611549905 posted

Regardless of whether you think exceptions are good or bad, you should always follow the idioms and conventions of the language that you are using.

In .NET, the convention is that a method signals failure to do what its name says that it does by throwing an exception. If you return a status code instead, your callers will fail to check it. If you do not throw an exception when your method fails, your callers will assume that it was succesful.

You can shout about laziness, or sloppiness, or absolute folly, or not reading API documentation till you are blue in the face, but that doesn't alter the fact that you are putting requirements on your users that are surprising, unexpected, and easy to miss.

And they will miss them. Just do a Google search for "inattentional blindness." Or watch this video.

Furthermore, the resulting problems will not show up straight away. They will only show up three months down the road when your users find out that they've sent out thousands of pounds worth of orders that haven't been charged for and they've no idea why because they hadn't been logging the fact that your API had been misbehaving.

(TryXXX methods are a special case here. Their name clearly indicates a convention that a return value of false indicates invalid input or a request for a resource that does not exist. However, this is also a well-defined and widely-known convention so it isn't surprising to your API's users. If the method failed for any other reason, it should throw an exception.)

(1) code never fails by default; code behaves as intended by design.

No, code can fail by default. It should fail by default if it is presented with unexpected bad input, or if an external resource is unavailable, or if it has bugs.

(2) you can not handle code at any level ... it does not work that way; one handles code at the boundary between MyMethod ~~ YourMethod.  OTOH, one can choose to ignore the interaction the occurs at the boundary between MyMethod ~~ YourMethod, frequently with predictable consequences.

No, you only handle return codes at the boundary between MyMethod ~~ YourMethod in methods that use that approach.

You do not attempt to handle exceptions at the boundary between MyMethod ~~ YourMethod because the implications of doing so are different. If you don't let an exception bubble up when you can't resolve or work around the condition that caused it, you will cause data corruption.

Answer 8

User-434868552 posted

@PatriceSc   again, it is not a PayPal issue; i'm simply using the PayPal REST API as an example; to clarify, because of imho flaws in the PayPal REST API design, i've had to code around them in order to give my end users a better, more robust experience.

basically ALL languages moved to exception handling.

PatriceSc, not sure what you mean here.  Computer programmers generally need to be prepared for the consequences of hardware exceptions such as zero divide although it's best to avoid dividing by zero.  "ALL languages" is a lot of languages.  q.v. http://en.wikipedia.org/wiki/List_of_programming_languages 

If something is wrong you should have an exception and the overhead doesn't matter as this is something that should not happen

imho, it depends upon what something is wrong.

You can handle the exception where it makes more sense.

PatriceSc, if this were always the case, then would it not be bad design for Microsoft to give us the pseudo-exceptionless  .TryParse methods? (rhetorical)

A. ...The [PayPal REST API] code could actually achieve the expected result even if the PayPal librairies fails to retrieve the token ?

no, the PayPal REST API requires the consumer to provide an appropriate token for security reasons to continue the payment workflow.

. ... their doc should tell they expect a decimal. But you won't have  this problem if on your side you work with numeric types rather than with strings.

PatriceSc, please remember that this is the internet, a text-based world; the PayPal REST API expects strings ... the design flaw is that insignificant leading zeros still are valid strings that meet the requirements of the PayPal REST API documentation; the PayPal REST API either needs to change to documentation or handle the insignificant leading zeros properly.

PatriceSc,, again, this thread is not about PayPal, this thread is about design.  my subject is good design/bad design and is about design philosophy which is the reason that i posted the O.P. in http://forums.asp.net/16.aspx/1?Architecture which is clearly described as "Discuss and debate ASP.NET application designs.".

@jammycakes  James, i appreciate your input and promise to get back to your posts in this thread in a timely fashion; your points are interesting and because we appear to disagree somewhat i am doing some research before responding.

Answer 9

User753101303 posted

Ok so to be more accurate...

To me, it makes sense to use exceptions if the code you are calling "fails in an unexpected manner" to do what you asked for. You can still avoid exceptions (ie testing the divider, testing if a file exists, using tryparse etc...) if this is a situation that you expect to meet under normal circumstances (ie the user could enter a bad value and you expect that this value will be bad sometimes so you want to test for this, not to get an exception because you attempted to blindly use this value).

I do agree that it depends exactly what is wrong. For example if creating a user fails because this account already exists, this is something that is expected under normal circumstances. So here it should return some kind of status to tell why it failed (existing account, password not complex enough, etc...)

So to me this is about using both depending on the situation,that is an exception if the failure is not expected at all under normal circumstance and a status code if the failure is expected to happen under a "normal" situation. And you would avoid exception by doing tests (file exists whatever etc...) ie I never thought about that before but basically in my ideal world having an exception would mean that the root cause almost always need to be investigated and fixed (be it just a server being down or whatever).

So it would depend exactly why those PayPal calls are failing.

For the leading 0 issue, I do agree that they should be able to deserialize this to a numeric value but still from a design point of view, I would say the usual "use the more restrictive type that fit your needs" should still apply. That is if a numeric value is expected here, the client facing API should IMO use the restrictive datatype ie a numeric value. Incidentally you would likely not run then into this issue. Being "transported" as a string is IMO irrelevant here (for example this is not because you'll use the JSON format or whatever that your web API will use only strings. You just use the type that make sense at the API level).

Answer 10

User-1611549905 posted

The important thing is that whether you are using return codes or exceptions, you follow a clearly-defined set of conventions so that your callers know exactly which to expect. These conventions will normally be determined mostly if not entirely by your programming language.

In .NET, for instance, you should only indicate failure by a return code if your method's name starts with "Try". All other cases should indicate failure by throwing an exception.

Almost every exception-related problem that I've ever seen in code has been a result of developers not sticking to the conventions of their language.