On Mon, Jun 22, 2015 at 5:59 PM, Chris Travers <chris.trav...@gmail.com>
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 <e...@efficito.com> 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.
------------------------------------------------------------------------------
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
Ledger-smb-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel
