r15094 - gnucash/branches/gda-dev - Initial commit of initial gda backend framework. See GDA_STATUS.

[Phil Longstaff]

On Tue, 2006-07-11 at 14:01 -0500, Derek Atkins wrote:
> Phil,
> 
> First, thanks for this work so far.  It's looking like we're making
> progress, which is good.  But I've got a few comments about this work.
> Comments are inline..
> 
> Phil Longstaff <plongstaff at cvs.gnucash.org> writes:
> 
> >    gnucash/branches/gda-dev/src/backend/gda/ddl/business.ddl
> 
> The business ddl should live unser src/business/business-core/gda
> (along with any other business-specific GDA code).

I agree. I created that ddl file by looking at the xml code and creating
a ddl file from it.  I haven't even tried to see if mysql or any other
db will accept the ddl.  Eventually it will be moved (or even moved
internally into the code in some fashion).

> 
> > +Building:
> > +- I am currently building with libgda 1.99.1 installed in /opt/libgda-1.99.1.
> > +This location is hard wired into src/backend/gda/Makefile.am as LIBGDA_DIR
> 
> You should just add a configure test to find libgda and use that.  You
> could add a --with-gda-prefix= so you could bypass the specific
> pkgconfig checks.  But you shouldn't hard-code the path in the
> Makefile.  The configure script should export at least two macros:
> GDA_CFLAGS and GDA_LIBS (or LIBGDA_, and it could be _INCS instead of
> _CFLAGS) -- but you get the point.

I agree.  However, I took at look at configure.in to see how to do this
and saw lots of AC_xxx macros which I didn't understand.  I didn't see
another --with-xxx-prefix option that I could use as a template.  I did
what worked for me in the short term, and have it on the TODO list to
configure it properly.

> > +Execution:
> > +- A basic GDA backend framework now exists.  This framework accepts URLs of
> > +the form gda://DSN:USERNAME:PASSWORD.  "gda" is required.  "DSN" represents
> > +a dataset configured in ~/.libgda/config.  USERNAME and PASSWORD are not
> > +required but can be specified.
> 
> How would this work with SQLite?

It will work with SQLite if a DSN is configured.  Another TODO item is
to switch file:// over to use SQLite which will just take a file name.

> > +- The backend assumes that the database already exists and tables are set up
> > +according to src/backend/gda/ddl/gnucash.ddl.  Note that
> > +src/backend/gda/ddl/business.ddl has not been tried yet, so there are no
> > +guarantees that any database will accept it.
> 
> I assume this comment is really a translation of a todo item: Get
> Gnucash to build the DB table.  This should probably be changed in the
> File -> New File function -- the File -> New File subsystem should ask
> the user for a Filename (or other DSN) and then do all the
> initialization for the user.

Yes, this is a translation from a TODO item into a "how it currently
works" item.

BTW, the file UI is built around files.  Save As, for example, doesn't
allow me to save as gda://gnucash.

> > +- The backend will save commodities and load them on startup.  However, they
> > +will not have the correct GUIDs.
> 
> I'm not convinced that Commodidies need to have (or SHOULD have)
> GUIDs.  In fact, the XML backend doesn't store the commodity GUID,
> either.  Everything is referenced PURELY by the Commodity
> <Namespace,ID> tuple.  The database should mirror this.

The problem is ran into is how to handle it when a commodity's name or
namespace is changed.  If a guid is used as the commodity ID, I just
update the commodity.  If I use the tuple, I need to update every
account, transaction, ... that uses that commodity.  The XML backend
doesn't have this problem because internally, the name can be changed,
and the XML backend can write the new tuple with the new namespace or
ID.

> > +++ gnucash/branches/gda-dev/src/backend/gda/ddl/slots.ddl	2006-11-07 15:56:39 UTC (rev 15094)
> > @@ -0,0 +1,23 @@
> > +-- Slots table to store all slots
> > +CREATE TABLE slots (
> > +	slot_id int NOT NULL,
> > +
> > +	-- What type of object, and what guid, does this slot belong to
> > +	object_type int NOT NULL,
> 
> I dont think we need the object type here.  The application is always
> looking into this table from an object; it never looks from here back
> up into the object tables.  So you don't need to differentiate.

Possibly not.

> > +	obj_guid char(32) NOT NULL,
> > +
> > +	-- Full name
> > +	name text NOT NULL,
> > +
> [snip]
> > +	PRIMARY KEY(slot_id)
> 
> Actually, I think the primary key would be the <obj_guid,name> tuple.

Could be, since as far as I can tell, the slot info really belongs to
the owning object.

> > Added: gnucash/branches/gda-dev/src/backend/gda/gnc-account-gda.c
> [snip]
> > +/** @file gnc-account-gda.c
> > + *  @brief load and save data to SQL 
> > + *  @author Copyright (c) 2000 Gnumatic Inc.
> > + *  @author Copyright (c) 2002 Derek Atkins <warlord at MIT.EDU>
> > + *  @author Copyright (c) 2003 Linas Vepstas <linas at linas.org>
> > + *  @author Copyright (c) 2006 Phil Longstaff <plongstaff at rogers.com>
> 
> I dont think this copyright notice is correct.  Gnumatic, myself,
> and Linas have nothing to do with this file, so you can remove it.
> Yeah, I know you copied the file backend source, but really this
> is very different.

I think all that is left of the original is the header comment and list
of include files.  I'll fix.

> > + * This file implements the top-level QofBackend API for saving/
> > + * restoring data to/from an SQL db using libgda
> > + */
> > +
> > +#include "config.h"
> > +
> > +#include <glib.h>
> > +#include <glib/gi18n.h>
> > +#include <libintl.h>
> > +#include <locale.h>
> > +#include <stdio.h>
> > +#include <fcntl.h>
> > +#include <limits.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <dirent.h>
> > +#include <time.h>
> > +#include <libgda/libgda.h>
> > +
> > +#include "qof.h"
> > +#include "qofquery-p.h"
> > +#include "qofquerycore-p.h"
> > +#include "TransLog.h"
> > +#include "gnc-engine.h"
> > +#include "Group.h"
> > +#include "AccountP.h"
> > +
> > +#include "gnc-filepath-utils.h"
> > +
> > +#include "gnc-backend-gda.h"
> > +#include "gnc-gconf-utils.h"
> > +
> > +#include "gnc-account-gda.h"
> > +
> > +#ifndef HAVE_STRPTIME
> > +# include "strptime.h"
> > +#endif
> 
> Are you sure you need all these headers?  Or did you just copy it?

Just copied.

> > +static Account*
> > +load_account( QofBook* pBook, GdaDataModel* pModel, int row )
> > +{
> [snip]
> > +	for( col = 0; col < numCols; col++ ) {
> > +		val = gda_data_model_get_value_at( pModel, col, row );
> > +		
> > +		switch( col ) {
> > +			case 0:	/* guid */
> > +				s = g_value_get_string( val );
> > +				string_to_guid( s, &guid );
> > +				break;
> 
> I'm not sure we really want to hard-code the column number to the
> column data like this.  Is there any way we could make this more
> data driven?  Also, I'd like to try to see this tie into the QOF
> definitions, maybe some QofParam tie-in?
> 
> For example, I could see how you could define the following mappings:
> 
>   column -> QofParam
>   QofType -> GdaType
> 
> and then use this abstraction to say something like:
> 
> AccountColumns[] = { QOF_PARAM_GUID, ACC_TYPE, ACC_CMDTY, ... };
> 
> Then you can use the QofParam Getters/Setters...

This was partly to experiment with Qof and with Gda and GValues and ...
to see how things actually work.  Unfortunately, not all db fields can
be handled via a mapping (e.g. Account parent GUID).

> 
> [snip]
> > +			default:	/* too many cols */
> > +				*(char*)0 = 0;
> 
> This is a SEGV waiting to happen.  If you want to abort the
> application you should use g_assert().  I presume this is just
> a placeholder for you -- I see this construct in a lot of places.
> I also think these should be marked:  // XXX: FIXME: <note>

Yes, it's a placeholder.  I'll start adding FIXMEs

> > +	pAccount = xaccMallocAccount( pBook );
> 
> You should begin_edit here...

except that all I want to do is create a new account structure.  I
really don't want any background stuff.  I certainly don't want to
commit (and I've already added a flag turning off commits while doing an
initial load).

> 
> > +	xaccAccountSetGUID( pAccount, &guid );
> > +	xaccAccountSetName( pAccount, name );
> > +	xaccAccountSetType( pAccount, type );
> > +	xaccAccountSetCode( pAccount, code );
> > +	xaccAccountSetDescription( pAccount, description );
> > +	xaccAccountSetCommodity( pAccount, pCommodity );
> > +
> > +	if( pParent != NULL ) {
> > +		xaccAccountInsertSubAccount( pParent, pAccount );
> > +	}
> 
> And commit_edit here...

See above.

> > +	return pAccount;
> > +}
> 
> [snip]
> 
> > +static void
> > +commit_account( GncGdaBackend* be, QofInstance* inst )
> > +{
> > +	Account* pAcc = (Account*)inst;
> > +	Account* pParent = xaccAccountGetParentAccount( pAcc );
> > +	gnc_commodity* c;
> > +	const GUID* guid = xaccAccountGetGUID( pAcc );
> > +	char guid_buf[GUID_ENCODING_LENGTH+1];
> > +	char commodity_guid_buf[GUID_ENCODING_LENGTH+1];
> > +	const GUID* parent_guid;
> > +	char parent_guid_buf[GUID_ENCODING_LENGTH+1];
> > +	char cmdbuf[300];
> 
> These should be gchar, not char.  Also, the cmdbuf is a buffer
> overrun waiting to happen..
> 
> [snip]
> > +	if( inst->do_free ) {
> > +		sprintf( cmdbuf, "DELETE FROM accounts WHERE guid='%s';", guid_buf );
> 
> You should use snprintf(), or better yet, g_strdup_printf()..  In the
> latter case be sure the g_free() the result.
> 

I'm not familiar with some of the glib functions.  I'll look at them. 

> [snip]
> > +			sprintf( cmdbuf, "UPDATE accounts set name='%s',account_type_id=%d,commodity_guid='%s',parent_guid='%s',code='%s',description='%s' WHERE guid='%s';\n",
> > +				name, type, commodity_guid_buf, parent_guid_buf, code, description,
> > +				guid_buf );
> 
> This could easily overwrite the buffer size.  Also, what happens if
> the user puts parens into the data?  E.g., what happens if I have an
> account name of something like "Foo's Account" -- that would cause a
> SQL error.  Worse, this could cause a user to be able to potentially
> execute arbitrary SQL.
> 
> Doesn't GDA have a feature to pull variables from an argument array
> instead of printing them into standard SQL String?  E.g., something like:
> 
>    gda_build_query("UPDATE accounts set name=%1,account_id=%2,...",
>                    name, type, ...);
> [snip]
> 
> > +static void
> > +handle_and_term( QofQueryTerm* pTerm, char* sql )
> > +{
> > +	GSList* pParamPath = qof_query_term_get_param_path( pTerm );
> > +	QofQueryPredData* pPredData = qof_query_term_get_pred_data( pTerm );
> > +	gboolean isInverted = qof_query_term_is_inverted( pTerm );
> > +	GSList* name;
> > +	char val[33];
> > +
> > +	strcat( sql, "(" );
> > +	if( isInverted ) {
> > +		strcat( sql, "!" );
> > +	}
> > +
> [snip]
> > +	} else {
> > +		*(char*)0 = '\0';
> > +	}
> > +
> > +	strcat( sql, ")" );
> > +}
> 
> Again, you've got a huge potential for buffer overrun here.   We should
> probably use g_strdup_printf() here, again, or at least be amenable to
> arbitrary string sizes.   Also, notice this SEGV waiting again?  ;)

Again, the segv is a FIXME.  I need to look more at how to handle error
conditions.

> 
> Pretty much the rest of the file I dont have any comments that I
> didn't say above.  We probably DO want to think about how to GModule
> gets loaded so that gnc-engine.c doesn't need to know about it..  And
> we also probably want the URI handlers to get registered (plugged in)
> by the backend handlers.
> 
> Anyways, that's all for now.   Good Luck, and thanks so far..
> 
> I really suggest thinking about ways to reduce the hard-coding of
> the tables and mappings of the objects.  The more you can abstract
> the easier it'll be to make it general.
> 
> Also, the way your queries are set up you're tying your table names to
> the Qof ID names.  This is fine, but just keep in mind that that's
> what you've done, so you need to make sure that your actual tables DO
> match your Qof ID name strings.
> 
> That's all for now, I'm sure there will be more discussion.

Thanks for the comments.
Phil