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

[Chris Travers <..hidden..>]

I just realized LedgerSMB::Request has a require_from('Moose::Class')
API which may be helpful here.  I don't know how well this one is
tested, so will probably spend some time over the next week or so
adding test cases.

On Mon, Jun 22, 2015 at 8:07 PM, Chris Travers <..hidden..> wrote:
> Hi John;
>
> Good thoughts!
>
> On Sun, Jun 21, 2015 at 10:29 PM, John Locke <..hidden..> wrote:
>> One more time!
>>
>>
>> On 06/21/2015 01:41 PM, Erik Huelsmann 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".
>>>
>>> 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!
>>
>> This does seem absurd at this point -- why not just change those to
>> throw exceptions? Then we can catch them at the appropriate point and
>> bubble those up to the interface.
>
> In Perl the difference between croak and die is saying "you gave me
> bad data" and "I messed up."
>
> I think we would do well to at least allow a difference between a 4xx
> and a 5xx error to provide something similar.  I have a patch to
> commit tonight to do that.  Preprocessing errors should be 4xx errors,
> processing and post-processing errors should be 5xx errors, I think.
> This allows service consumers to determine how to handle based on the
> status.
>
> But this leads to some issues.  A required field that is missing from
> a db entry should be a 5xx error but a required field missing from a
> field on request is a 4xx error.
>>
>>>
>>> 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.
>>>
>>> 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.
>>>
>>>
>>> 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.)
>>
>> Exceptions. Without question.
>
> Right now that's what we are using.   I don't see us moving away from
> them.  They are convenient, if quick and dirty.  They are generally
> accepted in the Perl community (we aren't programming in, say, Scala
> where referential transparency is a lot more important).  However I
> could see an argument for moving away from them for preprocessing
> conditions.  See below in discussion of response objects.
>>
>> We do need separate response objects that get instantiated right at the
>> start of the request. These should have some sort of ending method --
>> something like ->send() -- to actually output the response back to the
>> requestor, in the appropriate format.
>
> Ok, so in 1.5 we effectively only support Plack.  We pretend to do
> this via CGI but the code is moving to a point where we could very
> easily change this.  In other words right now we "print" but that
> actually goes to a wrapper, which returns the Plack response, which
> then gets converted back to whatever gateway interface is actually
> happening behind the scenes.  PSGI servers would just convert to HTTP
> responses.  CGI environmnets would print out things in relevant ways,
> etc.  So far so good.
>
> The fundamental assumption of Plack is that a response is a function
> of the request.  Eventually we should be getting to a point where
> instead of printing a response, we simply return it.  Exceptions
> should go to an exception handler that returns a response.
>
> In this view, I think we'd do best to do something like:
>
> my $validation_err = LedgerSMB::Request->requires('foo', 'bar', 'baz');
> return $validation_err if $validation_err;
>
> This would allow some additional things that exceptions wouldn't
> allow.  For example, we could check *all* required fields and return a
> full set of them rather than dying on first error.  Now, I know this
> could still be done with exceptions (.e. delaying them and then dying
> later) but I think in that case, this may be cleaner.  One could also
> allow for multiple validation checks and errors to be merged and so
> forth (which exceptions can't readily do since by definition they
> break program flow unless we put them in eval{} )..
>
> If $validation_err implements a response interface, then we are in
> good shape.  We could also handle CLI tools by making our exit codes
> for non-2xx/3xx responses into the http status codes.
>
>>
>> Server side, we need a global exception handler to catch anything not
>> caught during actual processing, which can attach the appropriate error
>> data to the response header along with the appropriate error code. The
>> response object should generally return an appropriate http response
>> code, and can then pass the stack trace (if appropriate) and any other
>> data necessary, on up to the browser.
>
> Yep, we have that, or rather we have 2, one for old code and one for new code.
>>
>> The response object should be smart enough to know how much detail to
>> transmit -- perhaps some other service can filter messages if we want to
>> keep very sensitive stuff away from the browser, and only transmit it
>> all if the site is set to 'debug'...
>
> Actually the problem in handling Moose instantiation and stack traces
> goes a bit deeper.
>
> The stack traces are appended to the exception text by Moose, so my
> thinking is we want to avoid sending those at all back to the browser.
> I think we are much better off with a wrapper that can be told "we are
> getting this from the database and not the UI" and appropriately
> transform user-visible error messages in that context.  Again Perl has
> some useful features in this regard.
>
> But basically we need to catch the exception, process it in the
> context we are actually running at the moment, and then then handle
> that.  That's doable, but just needs some thought.  We don't have too
> many places we need to do that so it should be possible at least for
> new code.
>
>>
>>
>> So really what I've described so far is how to get an exception up to
>> the browser. We haven't really handled it, just transmitted it. If we're
>> going single page, we really need to handle that in the browser, so most
>> of the actual graceful handling of the exceptions will end up being
>> Javascript.
>
> Right and to my mind that means being able to receive errors in html
> or json, and being able to differentiate based on http status.
>>
>>
>> I think one area we might want to explore further is what kinds of
>> exceptions we want to handle, where. Do we need specialized exception
>> classes? Do we catch some kinds of exceptions and simply log them, then
>> resume processing?
>
> 100% agreed here.
>>
>> Cheers,
>> John
>> http://www.freelok.com
>>
>>
>> ------------------------------------------------------------------------------
>> 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