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

[Derek Atkins]

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).

> +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.

> +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?

> +- 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.

> +- 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.

> +++ 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.

> +	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.

> 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.

> + * 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?

> +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...

[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>

> +	pAccount = xaccMallocAccount( pBook );

You should begin_edit here...

> +	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...

> +	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.

[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?  ;)

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.

-derek
-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available