Re: Proposed framework changes after 1.4 branches off

Hi Chris,

On Sat, Jan 18, 2014 at 12:24 PM, Chris Travers <..hidden..> wrote:

Hi everyone;

I want to propose the following framework changes after 1.4 branches off:

Having struggled with connection handling myself a bit yesterday evening (and ending up not solving the problem), I feel we do need action indeed. While I trust you to have made the remarks below from a design view, I'd like to take the opportunity to discuss a bit, hopefully making the design a bit more apparent to those who don't have the current situation top of mind. We could also use the discussion as documentation for a future implementation.

So, currently, there are a number of files/classes involved in connection setup:

* lsmb-request.pl - bridges the basic CGI request into the LedgerSMB application, by creating request, user, session and database connection objects

* LedgerSMB.pm - the "new code" version of the request object

* LedgerSMB::Session.pm - used to determine session validity

* LedgerSMB::Auth.pm - used to parse auth credentials

* LedgerSMB::App_State.pm - used to hold app state (such as a database connection)

* LedgerSMB::Database.pm - used to encapsulate database connection setup and database schema construction

The problem that I had to find my way around is that it's not clear which file does what. To give you an example: all of lsmb-request, LedgerSMB::new() and LedgerSMB::Session::check() sort through more or less "raw" connection data (e.g. HTTP Auth, CGI variables, etc) each to do their task and setup. But what role does each have? The database handle for example is cached all over the place.

One of the consequences of this lack of clarity is that there are two very similar concepts: login and username. Both are stored in the request object and used at different times, but they are used seemingly interchangeable during request construction.

1. Database connections should be handled by a separate class so we can get rid of duplicate code between LedgerSMB.pm and LedgerSMB::Form.pm.

2. LedgerSMB->new() needs to be simplified and modularized. Right now it is 170 lines long and rather hard to follow.

Agreed. Basically, what I saw there are lots of special cases, each with its own order of processing, while still maintained in the same function.

3. Split open/check/close_form into a separate module for handling form id's.

Well, my thoughts are that it really depends on your view if this is true or not. This is my view of the world:

Requests (operations) are grouped into sessions.

Sessions carry transitory state from login to logout -- typically, one would implement logic to verify requests being part of a session in the session object. Multiple sessions can be handled by an application. Depending on your performance requirements and scaling; one could associate database connections with requests (single operations) or sessions. [Resource (memory/tcp stack) scaling would normally mean association with the request level.]

The application contains non-transient configuration and state: database configuration, company address, etc.

In this model and given the statelessness of the HTTP protocol, I would bootstrap the CGI request (script being "called") into an application request (LedgerSMB request object being created) and from there promoting it to be associated with a session (including authentication).

After a valid session has been established, the request (operation) can proceed to do what it needs to do, which may include asking the application for application or session state. Reading the last sentence myself, I would argue for a stronger role of the session: having the database handle in the session, it would easy to handle multiple requests (especially old code does this, by "spawning off" an internal request), without the need to copy over all the state from one request to another. The session would also have the user details and possibly open (and closed) forms.

We currently have three types of requests:

* The "regular" request in the course of the application flow (forms being submitted, processed and results returned); these are part of a "session flow; from login to logout"

* The "authentication" request: the request which creates a session - the sole purpose is to validate user credentials and kick off a new session (ie initiate a series of requests)

The above two requests are associated with what I'd call "regular application users": employees which have database logins as well as application user objects. The third type of requests comes from the DBA level:

* Database superuser requests created through setup.pl;

A refactoring of the login code would IMO be really an exponent of the view we have on handling each of these situations in the final application.

4. Move to PGObject::Simple (for 1.3 code) and PGObject::Simple::Role (for 1.4 code). This requires some very simple changes to db call routines and allows a lot more flexibility if we need it.

Right. This is a technical detail and part of the move to putting more modules out on CPAN for other projects to depend upon, if they want to.

5. LedgerSMB->get_user_info should be moved to LedgerSMB::DBObject::User

This on the other hand has a lot to do with my point re (1)-(3): it's really about how we think we should model the application responsibilities. Whether that's through LedgerSMB::DBObject::User or LedgerSMB::UserNewCode is an implementation detail which will "automatically" follow, I think.

6. I would like to remove the following methods:
LedgerSMB->debug

LedgerSMB->escape (this is better handled by the Template macros)

LedgerSMB->is_blank (use LedgerSMB->required(...) instead)
LedgerSMB->redirect(). We shouldn't need this at all.
LedgerSMB->format_amount(). Not needed with new code.

LedgerSMB->parse_amount(). Use LedgerSMB->numbers instead.
LedgerSMB->round_amount(). Just round the number using ffround instead.
LedgerSMB->call_procedure() would be gone (in favor of PGObject interface).

LedgerSMB->is_allowed_role()
LedgerSMB->date_to_number()-- do we even need this?
Move web error handling to LedgerSMB::Request for code sharing
LedgerSMB->get_default_value_by_key should be removed in favor of LedgerSMB::Setting->get

Well, along the lines of my earlier thinking: if we have LedgerSMB::Request which models request state and manages the request/response cycle, we have LedgerSMB::Session to manage session state and we have LedgerSMB::Sysconfig to access application configuration -- what's left for LedgerSMB.pm??

Eventually I would like the LedgerSMB module to go away.

Ah :-) Sorry, didn't read this bit until I split it off from the list above :-)

I would also like to rewrite our current auth modules as they have accumulated a fair bit of cruft in 1.4. I would also like to rewrite LedgerSMB::App_State to use singletons and accessors instead of direct variable access.

To be honest, I'm not really all that clear on what the App_State is about, if we have transient state in Request and Session and non-transient state in Sysconfig, what more state is there to be managed?

Some of the above (in particular removing unused LedgerSMB functions) I might get to before the branch.

I think the removal is fine. I'd like to take a bit of discussion like this before we proceed to refactoring though. I hope you don't mind and can find the time to address some of my points.

Bye,

Erik.

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

Robust and Flexible. No vendor lock-in.