Re: [gnome-db] Minor API change and MySQL [update/delete]_row

[Rodrigo Moya <rodrigo gnome-db org>]

On Sun, 2003-10-26 at 03:18, Paisa Seeluangsawat wrote:
> Here's an alpha code for MySql updating and deleting row--only work
> when there's a unique, non-null key (ahem ;-).  There are still a few
> FIXME's in the code.  It also prints out the sql query for your
> debugging pleasure.  These will go away in a few weeks.  I'm just
> going by 'commit early, commit often' :-).
> 
> No one replied to my last e-mail about the 'const' issue, and adding
> gda_data_model_get_updatable_row().  So, taking silence as
> consent...I...err...I...  Don't be mad at me, OK?

> 
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/libgda/ChangeLog,v
> retrieving revision 1.581
> diff -u -r1.581 ChangeLog
> --- ChangeLog	18 Oct 2003 23:10:39 -0000	1.581
> +++ ChangeLog	26 Oct 2003 00:49:25 -0000
> @@ -1,3 +1,45 @@
> +2003-10-25  Paisa Seeluangsawat <paisa users sf net>
> +	
> +	*gda-blob.c: fixed return type mismatch in gda_blob_free_data()
> +
format is wrong here, use full relative path to the file, that is:

	* libgda/gda-blob.c:...

>  /**
> + * gda_data_model_get_updatable_row
> + * @model:	a #GdaDataModel object.
> + * @row:	row number.
> + *
> + * Retrieves an editable row from a data model.
> + *
> + * Returns: a #GdaRow object.  #NULL if fail.
> + */
> +GdaRow *
> +gda_data_model_get_updatable_row (GdaDataModel *model, gint row)
> +{
> +	g_return_val_if_fail (GDA_IS_DATA_MODEL (model), NULL);
> +	g_return_val_if_fail (CLASS (model)->get_updatable_row != NULL, NULL);
> +
> +	return CLASS (model)->get_updatable_row (model, row);
> +}
> +
>
thinking about all this and the 'begin_edit' stuff, I think you are
right, and we really need to allow providers to do per-row
initialization, if they need to. So, having a begin_edit, where the
provider does not know which row has to be locked/saved/whatever, is not
a good idea. So, yes, let's go with this get_updatable_row.

The only thing that doesnt gets solved is how to do process batch of
updates?

>  /**
>   * gda_data_model_foreach
>   * @model: a #GdaDataModel object.
> @@ -634,118 +631,23 @@
>  			GdaDataModelForeachFunc func,
>  			gpointer user_data)
>  {
> -	gint cols;
>  	gint rows;
> -	gint c, r;
> -	GdaRow *row;
> +	gint r;
> +	const GdaRow *row;
>  
>  	g_return_if_fail (GDA_IS_DATA_MODEL (model));
>  	g_return_if_fail (func != NULL);
>  
>  	rows = gda_data_model_get_n_rows (model);
> -	cols = gda_data_model_get_n_columns (model);
> -
> -	for (r = 0; r < rows; r++) {
> -		row = gda_row_new (model, cols);
> -		for (c = 0; c < cols; c++) {
> -			GdaValue *value;
> -			value = gda_value_copy (gda_data_model_get_value_at (model, c, r));
> -			memcpy (gda_row_get_value (row, c), value, sizeof (GdaValue));
> -		}
> -
> +	gboolean more = TRUE;
>
this is C99, so wont compile in some platforms. Make sure all variable
definitions are always at the beginning of code blocks, not in between.
 
> +// FIXME: These hopefully will go into *_impl.h ================
> +
why?

> +void                gda_data_model_changed (GdaDataModel *model);
> +void                gda_data_model_row_inserted (GdaDataModel *model, gint row);
> +void                gda_data_model_row_updated (GdaDataModel *model, gint row);
> +void                gda_data_model_row_removed (GdaDataModel *model, gint row);
> +
> +void                gda_data_model_set_column_title (GdaDataModel *model, gint col, const gchar *title);
> +
> +void                gda_data_model_set_command_text (GdaDataModel *model, const gchar *txt);
> +void                gda_data_model_set_command_type (GdaDataModel *model, GdaCommandType type);
> +
>  
>  G_END_DECLS
...

>  
>  	row = gda_row_new (model, g_list_length ((GList *) values));
>  	for (i = 0, l = values; l != NULL; l = l->next, i++) {
> -		const GdaValue *value = (const GdaValue *) l->data;
> +		GdaValue *value = (GdaValue *) l->data;
>  
>  		if (value)
> -			gda_value_set_from_value (gda_row_get_value (row, i), value);
> +			gda_value_set_from_value (gda_row_get_updatable_value (row, i), value);
>
hmm, this looks wrong to me. Why do we want a
gda_row_get_updatable_value? What, apart from not returning const will
it do?

>  GdaDataModel *
> -gda_row_get_model (GdaRow *row)
> +gda_row_get_model (const GdaRow *row)
>  {
>
hmm, again, this looks bad to me. I know it's not easy to come up with a
good const/non-const setting, but this totally breaks GNOME standards.
You've got always, in other GNOME libs, the first argument being the
class object, with no const. Doing this now breaks that standard.

The same for all other similar functions. We should just use const for
return values, I guess. If not, we are looking a lot different than
other GNOME libs.

>  
> +static GdaRow *
> +gda_mysql_recordset_get_updatable_row (GdaDataModel *model, gint row)
> +{
> +	g_return_val_if_fail (GDA_IS_MYSQL_RECORDSET (model), NULL);
> +	GdaMysqlRecordset *recset = (GdaMysqlRecordset*) model;
> +
>
C99 again

> +	find_key_columns (recset);
> +	if (recset->key[0]==0)  return NULL;  // not updatable
>
please use our coding guidelines, which should make the above line look:

	if (recset->key[0] == 0)
		return NULL;

that is, spaces between identifiers, values, operators, etc, and the
code block in its own line.

> +	GdaRow *gda_row = (GdaRow*) gda_mysql_recordset_get_row (model, row);
>
again, this is C99 :-)

> +	if (gda_row && !gda_row_get_id (gda_row))
> +		set_id (recset, gda_row);
> +	return gda_row;
> +}
> +
hmm, one of the things I think providers could do in this function is to
store the old row in some cache, so that if editing is cancelled, they
just put back the old row.

> -gda_mysql_recordset_remove_row (GdaDataModel *model, const GdaRow *row)
> +gda_mysql_recordset_remove_row (GdaDataModel *model, GdaRow *row)
>  {
> -	return FALSE;
> +	GString *str;
> +	const gchar *id;
> +	gint rc;
> +
> +	g_return_val_if_fail (GDA_IS_MYSQL_RECORDSET (model), FALSE);
> +	g_return_val_if_fail (model == gda_row_get_model(row), FALSE);
> +
> +	GdaMysqlRecordset *recset = (GdaMysqlRecordset*) model;
>
more C99

> +	id = gda_row_get_id(row);
>
this sould be:

	id = gda_row_get_id (row);

with a space after the function name.

> +	g_return_val_if_fail (id != NULL, FALSE);
> +
> +	str = g_string_sized_new (200);
> +	g_string_sprintf (str, "DELETE from %s WHERE %s", recset->table_name, id);
> +
this looks wrong, isn't it missing the correct WHERE clause?

> 	
> +	g_printerr ("\nquery:  %s\n", str->str);  //FIXME: just debugging
>
it's ok to use FIXME's in the code, but do so with standard C comments
(/* .. */), not C++. This is to avoid compilation failures in some
systems.

> +	rc = mysql_real_query (recset->mysql, str->str, str->len);
> +	g_string_free (str, TRUE);
> +	if (rc!=0 || mysql_affected_rows(recset->mysql)!=1) {
> +		gda_connection_add_error (
> +			recset->cnc, gda_mysql_make_error (recset->mysql_res->handle));
> +		return FALSE;
> +	}
> +	// FIXME what to do with the record now?
>
of course, remove it from the model :-) and notify clients of the
change, via the gda_data_model_row_removed function.

>  static gboolean
> -gda_mysql_recordset_update_row (GdaDataModel *model, const GdaRow *row)
> +gda_mysql_recordset_update_row (GdaDataModel *model, GdaRow *row)
>  {
> -	return FALSE;
> +	GString *str;
> +	const gchar *id;
> +	gint n;
> +	gint rc;
> +	gint *all;
> +
> +	g_return_val_if_fail (GDA_IS_MYSQL_RECORDSET (model), FALSE);
> +	g_return_val_if_fail (model == gda_row_get_model(row), FALSE);
> +
> +	GdaMysqlRecordset *recset = (GdaMysqlRecordset*) model;
>
more C99

> +	id = gda_row_get_id (row);
> +	g_return_val_if_fail (id != NULL, FALSE);
> +
> +	str = g_string_sized_new (200);
> +	g_string_sprintf (str, "UPDATE %s SET ", recset->table_name);
> +	n = gda_row_get_length (row);
> +	all = g_new (gint, n+1);
> +	all[0] = n;
> +	int i;
> +	for (i=0; i<n; i++)  all[i+1] = i;
> +	printfa_cols (str, recset, row, all, FALSE);
> +	g_free (all);
> +	g_string_sprintfa (str, " WHERE %s", id);
> +
> +	g_printerr ("\nquery:  %s\n", str->str);  //FIXME: just debugging
> +	rc = mysql_real_query (recset->mysql, str->str, str->len);
> +	g_string_free (str, TRUE);
> +	if (rc!=0 || mysql_affected_rows(recset->mysql)!=1) {
> +		gda_connection_add_error (
> +			recset->cnc, gda_mysql_make_error (recset->mysql_res->handle));
> +		return FALSE;
> +	}
> +	
> +	// In NOT NULL columns, MySQL quietly converts NULL to 0, 0.0, or "".
> +	// Our column is UNIQUE so this probably won't happen often, but handle
> +	// it anyway.
> +	for (i=1; i<=recset->key[0]; i++) {
> +		gint c = recset->key[i];
> +		GdaValue *value = gda_row_get_updatable_value (row, c);
> +		if (value->type != GDA_VALUE_TYPE_NULL) continue;
> +		MYSQL_FIELD *field = mysql_fetch_field_direct (recset->mysql_res, i);
> +		GdaValueType type = gda_mysql_type_to_gda (field->type);
> +		if (!gda_value_set_from_string (value, "", type) &&
> +		    !gda_value_set_from_string (value, "0", type))
> +			// should never get to here, but reluctant to use g_assert()
> +			g_printerr ("You've just found a bug.  Please let us know how to get to this message.  -- gnome-db-list gnome org");
> +		// note that we'd never get TIMESTAMP in our key[] since it's always
> +		// a NULL-ok column.
> +	}
> +
> +	// FIXME: should we refetch this record for the new AUTO INCREMENT, TIMESTAMP values?
> +	set_id (recset, row);	// our key column might have changed
>
you have to notify the model has changed, via gda_data_model_row_updated
function.

as you see, there are a lot of things missing, so please, continue
working on this, taking into account all we've been talking about it
these days.

thanks for your work