Re: Proposal to differentiate between program and processing(precondition) errors

On Mon, Jun 22, 2015 at 5:59 PM, Chris Travers <..hidden..> wrote:

Just to add to this for sake of discussion

First, for history's sake, we inherited the undifferentiated error case from sql-ledger.

Right. The fact that it's undifferentiated is the reason I'm bringing it up now. [I'm not sure if it really matters at this point whether or not we got it from SL -- it's something we should do something about - that I *want* to do something about -- but the question is What?]

We have actually started decoupling htis in some ways but that is very far from complete. Discussion and thoughts on this matter are very welcome.

On Sun, Jun 21, 2015 at 1:41 PM, Erik Huelsmann <..hidden..> wrote:

Currently, all errors generated by LedgerSMB are being raised through 'die'. It's an effective way, but my feeling is that it bleeds through too many details of the internals. This is especially true when the error is really nothing more than a reported "missing configuration".

Yeah, we could do a better job of handling this.

Taking this from the chat we had earlier: there are several ways to deal with identifying incomplete configurations. One is what we do now: report that a specific request doesn't make sense until some specific configuration is available. The other thing we talked about is to have some kind of tool to verify the system setup: check for currencies, check for latex, xetex, pdflatex, etc. and report all that to the user and which system functionalities will be blocked by not having that dependency available.

E.g. we currently generate an error in case there's a field's value missing. The generated error includes a stack dump and all!

It can include the stack trace. I don't think we do by default though with Starman you can turn this on and off by using -MCarp::Always but if this is happening without, it is a bug.

Ok. On my master-fixup branch, I got stack traces all the way. I should search for the included Carp::Always module, apparently. However, even with Carp::Always, we might want to write the errors to the log files with stack traces, but maybe we want to return a minimal erorr description to the client, as John suggests. That way at least there's no leaking of system configuration data.

My proposal is that in this type of error case, we're checking the existence of the required fields and their values. If these don't exist, we should be reporting a nicely formatted error to the client -- most definitely without a stack trace.
Agreed. We should also use the required field in html as well.

True, however, as we're moving to a services oriented model more and more, reporting correct and parseable responses gets increasingly important. Preventing the field from being forgotten is nicely handled by the 'required' HTML elements. However, I think we can't really depend on *the* client being our own clietn when we move to services.

Also, the HTTP status code for each error-with-stacktrace is currently "500" -- Internal Server Error. When we *understand* the request, but can't process it, we should be generating a 422 (Unprocessable entity) response or alike.
Agreed here too.

Ok. so, then the next step is to set up a structure to handle missing parameters if and when it occurs. Then the second step can be to search the code base for possible reports of missing parameters and replace them by whatever construct we come up with.

Now for the question: what would be the best way to achieve this? (I think this applies equally to HTML page responses as it does to web service api calls, so this is a general question that needs a general solution which is applicable for a long time.)

Ok, so currently we have a couple mechanisms we could use for this.

1) On the HTML side we really should set the required attribute where appropriate. As a bonus we can use css to highlight what is required.
2) On the server/service side, we now have LedgerSMB/Request.pm which allows us to specify required fields. We could modify that and the global error handling, to allow http statuses to be included. Remember we can die $hashref, and intercept/handle variables on that hashref. I could probably have a patch in to route from the required functions there to have appropriate http statuses fairly quickly.

Ok. Good you point me to LedgerSMB/Request.pm. I hadn't seen it's existence before. I have to think how we can fit this into the broader picture. If possible, I'd like to leverage the knowledge encoded in the Moose class definitions. I'm not sure if and how that works out with Request.pm.

Given that we probably want to handle missing fields from a lot of places as we probably want to handle it from both newer and older code, I like the idea of using "die $hashref" and processing the response from there. Combining this with John's idea of having a separate Response object, it'd probably a nice construct to "die $response" to shortcut further processing?

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.