Earizon refactoring exceptions

[earizon]

Refactor ILP Exceptions according to the latest RFC: https://github.com/interledger/rfcs/blob/master/0003-interledger-protocol/0003-interledger-protocol.md

I noticed the existing Exception/s use a hierarchy InterledgerException <-> child classes, but this could be overkilling now since there can be (pottencially) tens or hundreds of differnt errors and the public Enum InterledgerException.ErrorCode serves to have a compliant implementation with the RFCs. Also, it's not expected for code to extend the behaviour and/or data of the base exception (most probably the code will limit to serialize and send back the wire to other connectors/ledger/clients)

I leave as TODO comments some doubts I still have:

• Must ILQP Exception be treated as an "embeded" exception inside the InterledgerException.data ?
• how the exception be serialized?

[sappenin]

@earizon, you asked:

how the exception be serialized?

Do you mean Java serialization (i.e., java.io.Serializable) or do you mean via some encoding like JSON, Protobuf, etc?

[earizon]

@sappenin

Do you mean Java serialization (i.e., java.io.Serializable) or do you mean via some encoding like JSON, Protobuf, etc? I mean the final encoding. I think is well defined now on the RFC, but I had no time to check it properly. I think it must be "symetrical" to the ILP packet and must be OER encoded before sending it back in any transport (HTTP, WebSocket, ...). Again not 100% sure.

[sappenin]

I spent some more time thinking about this PR last night, working through some of the implementation's constructor cases for when a developer might use this class.

It occurred to me that we should consider splitting this class into two classes: One, an Interface that defines the contract for what's defined in RFC-3 (I'll call that InterledgerError.java for now); and Two, a class that actually extends RuntimeException (I'll call that InterledgerException.java).

In this potential new structure, InterledgerException.java could provide constructors that mirror those in RuntimeException and/or Throwable, in addition to taking a single extra property of type InterledgerError.

My rationale for this change is that these two classes will be used in very different contexts.

• InterledgerError.java is a Java representation of the error object being passed around on the wire in some format -- likely JSON or Protobuf, or some standardized ASN.1 OER encoding. This object (an Interface so developers can make their own implementations) should have no knowledger of things like a Throwable or a message or anything else in the Java exception heirarchy -- none of these details should leak into serialization formats (like JSON, OER, etc). Think of this as an object that models ILP base objects in the same way InterledgerAddress.java does.

• InterledgerException.java, on the other hand, is a formal Java exception (extending RuntimeException) that developers can "throw" and "catch" in Java code and has a "has-a" relationship with InterledgerError (meaning, an InterledgerException has an optionally-present InterledgerError)

In this way, Java programs can throw InterledgerExceptions all day long, handle them per-usual, but when it comes time to actually send something "on the ILP wire", like to a Connector, then the InterledgerError interface should be used.

Thoughts?

[earizon]

@sappenin From the RFCs looks to me that errors are always considered exceptions, since they are supposed to stop normal flow, trigger a different message response and message format to clients, and they are supposed to be treated in a completly different way to standard non-exception flow controls. I think the best (or the most conservative) approach would/will be to do what the JS reference implementation does. I couldn't find any related code in the interledgerjs repos yet.

[sappenin]

From @earizon: From the RFCs looks to me that errors are always considered exceptions, since they are supposed to stop normal flow, trigger a different message response and message format to clients, and they are supposed to be treated in a completely different way to standard non-exception flow controls.

@earizon No disagreements from me about InterledgerErrors being exception-cases. Instead, I was referring to a difference between modeling the ILP-layer and the Java exception layer, and that we probably want those two layers to be different classes.

For example, consider this typical API flow:

Hypotehtical HTTP API

1. Internal code throws a Java Exception.
2. Internal code catches the exception, looks at it's type or message, or whatever, and decides to map that to an HTTP status code and JSON payload.
3. API emits JSON that looks like this:

   {
   "code": 400,
   "message": "You can't do that because we don't allow it"
   }

Notice that the payload emitted by the API in this "exception case" doesn't include anything from the java.lang.RuntimeException class hierarchy. There's no message or stackTrace or cause or suppressedExceptions or anything like that. So, in my mind, we should model both things - 1.) the payload that will actually be returned to an ILP client in exceptional cases; and 2.) An exception class that can be used in Java code to try/catch.

To illustrate that using an Interledger example, consider a flow like this (again, this is just an example to illustrate a point):

Hypothetical ILP Ledger API

1. Internal code tries to execute a transfer, but the amount is negative.
2. Internal API throws a MyCoolInvalidAmountException. This will have an internal stackTrace, potentially a message, and let's say some internal attributes that should not be exposed to the outside world.
3. An exception-mapper catches the MyCoolInvalidAmountException, and then needs to marshal it to a payload that looks like what's defined in RFC-3. It transforms MyCoolInvalidAmountException into an instance of InterledgerError (see my previous comment), and returns this to the API layer.
4. API emits JSON that looks like this:

   {
   "code":"F03",
   "name":"Invalid Amount",
   "triggeredBy":"g.foo.bar",
   ... // other RFC-3 fields.
   }

Key point to notice is that the final payload has no mention of anything Java-Exception-related, because it's not a Java Exception. It's something else. And because it's something else, it should not extend from anything related to java.lang.Throwable.

This will allow any implementor to use the Java-Exception we provide (InterledgerException.java) or create their own exceptions -- but ultimately, return a standard ILP payload object (InterledgerError.java).

Thoughts?

[sappenin]

All, to illustrate what I was talking about in my previous comments, I submitted an alternate PR here.

While I do prefer my split-design, I am open if the group feels otherwise, so we can certainly close my PR in favor of this design.

Sometimes illustrating with code is easier to explain than written words trying to explain code.

[earizon]

@sappenin I noticed you are thinking about a different way of handling exception to the one I had in mind:

Internal code tries to execute a transfer, but the amount is negative. "An exception-mapper catches the MyCoolInvalidAmountException... It transforms MyCoolInvalidAmountException into an instance of InterledgerError "

I see we diverge in the way exceptions must be handled.

The way I see it, the code must not create a MyCoolInvalidAmountException, then later on map it to an InterledgerError. I think It's much safer (and IMHO, simpler) that the code detecting the error (amount is negative) will be forced to use a well defined InterledgerError at throw time. Allowing developers to be "lazy" when launching exceptions can produce buggy code. As Adrian once said "protect developers from themselves". Don't let them do something as "launch it know, think later how to map it to an ILP error" or "something/someone will (magically) know how to handle this exception and what it must do with it".

If the API just let developers report exceptions in an unique way (throw an InterledgerException with well defined InterlederErrors provided by the RFCs or ask someone else with more experience what to do) there will not be missunderstanding about how to use it.

The main controller listening for incomming requests must be limited to serialize exceptions "comming back" in the catch(InterledgerException e) block and send them back to previous nodes. It must have NO "mapping" logic or allow different implementations to "interpret" the flow of ILP errors. Different implementations can choose where is the best place to place the "catch(InterledgerException e)" (a custom dispatcher, a common controller, a filter,...), the details inside the catch block (logging, ...) before serializing and sending it back to clients.

Other non-ILP related exceptions must be procesed in any proper way by the implementation and finally re-launch a well-defined ILP Error "T00 Internal Error".

[earizon]

@adrianhopebailie , @sappenin I just merged. Last changes split InterledgerException<->InterledgerError and change array by List. Note: InterledgerError is a simple POJO in last commit. (no interfaces + impl)

[adrianhopebailie]

@sappenin, @earizon sorry I got a little lost in PRs and email threads on this and didn't have a chance to dig into the details.

I also hadn't realized @sappenin had proposed another PR and logged a number of related issues too.

@earizon Was the consensus that this was the best solution or did you modify it "against your will" 😄

My thinking is; let's use this as is for now and start building some implementations. We'll quickly discover if the design we have settled on is good or needs to change.

[earizon]

@adrianhopebailie I have not a hard opinion. Spliting in Error and Exception makes the code slightly more readable and clean anyway.