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

[Chris Travers <..hidden..>]

On Mon, Jun 22, 2015 at 2:32 PM, Erik Huelsmann <..hidden..> wrote:
>
> 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?]

Agreed.  I have a fix I am testing which moves the error handling to a
dedicated error class.

It also makes the default error status be 500, but anything coming
through LedgerSMB::Request::required() returns a 422.
>
>>
>>   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.

Did they come up as stack traces with the red Error at the top?  Or as
something else?

I am wondering where the stack traces came from.  The problem with
Carp::Always is that  it chances every "die" into a Carp "Confess" so
the stack traces get appended to the end of the text string.  This is
convenient for debugging, but not very convenient for the user.
>
>>>
>>> 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.

Of course.  Not disputing that.
>
>>>
>>> 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.

Ah.... stack traces there.... that's a Moose thing.....

LedgerSMB::Request is for preconditions and input handling of requests btw.

Moose dies with a stack trace when required attributes are not
provided.  I agree that's a bad thing.

I need to think about this.  I think we can fix that as well, but will
take some effort and careful design work.  What happens when Moose
requirements are stricter than the db for one reason or another?  We
probably don't want to display an error message telling the user that
they didn't enter the right data, when that data came from the db.

Again, solvable, just need to think how.  I am thinking that the local
keyword indicating we are in db processing may do wonders.

(For those who don't know, local sets a package-global variable for a
stack frame and descendant stack frames, allowing for module
configuration to be overridden only while specific functions are being
called.)
>
> 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?

I think where we put response objects or how we handle this requires
just a little more thought.  Perl frameworks tend to approach this
differently than PHP frameworks.   I do agree having a response class
of some sort is probably a good idea.  Just a question of how we use
it.
>
>
> --
> Bye,
>
> Erik.
>
> http://efficito.com -- Hosted accounting and ERP.
> Robust and Flexible. No vendor lock-in.
>
> ------------------------------------------------------------------------------
> Monitor 25 network devices or servers for free with OpManager!
> OpManager is web-based network management software that monitors
> network devices and physical & virtual servers, alerts via email & sms
> for fault. Monitor 25 devices for free with no restriction. Download now
> http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
> _______________________________________________
> Ledger-smb-devel mailing list
> ..hidden..
> https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel
>

------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors 
network devices and physical & virtual servers, alerts via email & sms 
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
Ledger-smb-devel mailing list
..hidden..
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel