Created attachment 185855 [details] [review]
libgda_null_type.patch

This patch changes libgda to use a custom GdaNull GType, but I'm getting these errors during gda_connection_update_meta_store():

(+5): Wrong Holder value type, expected type 'string' when value's type is 'null'

I guess that some code somewhere depends on a sql-null GValue's GType being 0 or G_TYPE_INVALID, or on !G_IS_VALUE().

Created attachment 185857 [details]
libgda_null_type2.patch

And this one makes many changes that fix that warning but still leave me with these warnings while moving the row in a GdaDataModel

sys:1: Warning: g_value_type_compatible: assertion `G_TYPE_IS_VALUE (dest_type)' failed
sys:1: Warning: g_value_get_string: assertion `G_VALUE_HOLDS_STRING (value)' failed

That happens at this point:

+ Trace 226710

This patch is sure to contain many errors.

At least some of these changes are necessary because libgda seems to have used GDA_TYPE_NULL as
1) A test for a valid GValue. G_TYPE_INVALID is better, though G_IS_VALUE() would be even better.
2) A flag that a GType is not yet set. G_TYPE_INVALID seems more appropriate here. And because G_TYPE_INVALID is 0, like the old GDA_TYPE_NULL, then g_new0() will still initialize them as expected.

I was the responsible of your problems. I ported GdaValue to GValue, creating most of functions to use GValue instead of a custom GdaValue holder.

At design process found no GType definition for a null value. In a database, AFAIR, a null value is still a value but undefined type 

I desired to use an invalid GValue (not set) to represent a NULL value in a database. This is why gda_value_set_null, calls l_g_value_unset (value); defined as:

#define l_g_value_unset(val) G_STMT_START{ if (G_IS_VALUE (val)) g_value_unset (val); }G_STMT_END

A NULL value in a database is different from an INVALID one, as GObject documentation stats:

G_TYPE_INVALID

#define G_TYPE_INVALID			G_TYPE_MAKE_FUNDAMENTAL (0)

An invalid GType used as error return value in some functions which return a GType. 

Then in order to avoid to use 0 as NULL type for NULL values in a database, because GObject Introspection declare it as invalid, may be we need to define a new GdaNull type, as follow:

typedef struct {
    gshort cero;
} GdaNull;

and define a BOXED_TYPE to use it as a valid GType.

Then define:

#define GDA_VALUE_NULL gda_value_null_get_type() 

will be used. 

Any comments before to create a patch?

(In reply to comment #3)

> maybe we need to define a new GdaNull type

Yes, that's what my patch above does, though there are many more changes that need to be made to use that type. I don't think any struct declaration is actually necessary.

I have tried this a few times and have not got very far in the limited time that I have. Please do try.

Created attachment 188190 [details] [review]
Sustitute invalid GValues by a valid ones using a GdaNull type

I've created a patch to add a GdaNull type to use a valid type for database null types.

I need to check it in a system different than a 64 bits, because I have problems with the providers.

Please test it! I'll do it as soon as I can have a descent test machine.

Thanks, but that patch contains multiple commits, at least one of which in an unrelated thing about introspection. I can't easily get it to apply. 
Could you turn it into just one commit/patch?

Created attachment 189426 [details] [review]
Adding GdaNull type, fixing SQLite provider

Ok I have something working now.

This patch add GdaNull data type to handle any database NULL types. It is defined as G_TYPE_BOXED derived type and always returns a valid GType.

Fixes the use of Gda's null Value representation. In most cases changes the way you must handle internally.

This patch affects any providers implementation that doesn't use GDA's value API and assumes invalid GValues to represent NULL values in the database. Please see at SQLite provider modifications. Fix other providers is required in order to finish the transition to GdaNull.

I've tested this patch using PyGObject+GObject Introspection using the following code:

$ python
Python 2.7.1 (r271:86832, Apr 12 2011, 16:15:16) 
[GCC 4.6.0 20110331 (Red Hat 4.6.0-2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from gi.repository import Gda;
>>> cnn = Gda.Connection.open_from_dsn("test", None, Gda.ConnectionOptions.READ_ONLY);
>>> cnn.is_opened()
True
>>> m = cnn.execute_select_command("SELECT * FROM test")
>>> print m.dump_as_string()
id     | description | notes 
-------+-------------+-------
_null_ | Text1       | _null_
(1 row)

>>> v = m.get_value_at(0,0) 
>>> v
<GdaNull at 0x26ef040>
>>> Gda.value_stringify(v)
'_null_'
>>>

Patch attachment (id=189426), was created against origin/LIBGDA_4.2

GdaBrowser fails with a SQLite provider.

Works on PostgreSQL!!!

Review of attachment 189426 [details] [review]:

::: libgda/gda-value.c
@@ -1953,3 @@
 			}
 			else
-				return g_strdup ("");

Is this change really related to the null type change?

Created attachment 189447 [details] [review]
0001-null-type-patch.patch-

This is an API change that should only be made on master, not LIBGDA_4.2. Here is my attempt to port your patch to master.

Note that this should not be pushed until "make check" succeeds again.

Comment on attachment 189447 [details] [review]
0001-null-type-patch.patch-

This is an API change that should only be made on master, not LIBGDA_4.2. Here is my attempt to port your patch to master.

At the end this must be ported to master, but LIBGDA_4.2' API have no changes, AFAIT. The problem is ABI, but for providers.

As stated by comment (In reply to comment #10)
> Review of attachment 189426 [details] [review]:
> 
> ::: libgda/gda-value.c
> @@ -1953,3 @@
>              }
>              else
> -                return g_strdup ("");
> 
> Is this change really related to the null type change?

You are right. Is unrelated and is a mistake to modify the return value. Feel free to change it.

(In reply to comment #12)
> (From update of attachment 189447 [details] [review])
> This is an API change that should only be made on master, not LIBGDA_4.2. Here
> is my attempt to port your patch to master.

First to make the master port of this patch.

Why do you think it is an API change? I try to not modify API for NULL types. If you see at comment #9, PostgreSQL provider works with no modifications. The actual problems with SQLite provider is related with the assumptions on what a GDA's null means, if it uses public API this must not be the case (see at gda-holder.c, it defines its own gda_value_is_null function of internal use).

I would love to see this to happend on 4.2, because I would like to see a transition from 4.x to 5.x, the same way as 2.x to 3.x of Gtk, with a good support of bindings using GObject Introspection.

(In reply to comment #14)
> Why do you think it is an API change? I try to not modify API for NULL types.

It's API addition, at the least, which doesn't strictly belong in a stable version.

But more importantly, it's a change to how GValue should be used for null types. There's a fairly strong chance that someone is checking the GValue's GType for NULL.

Even more importantly, it's an ABI change because you are changing the definition of GDA_TYPE_NULL from 0 to the result of a function call. Application code must be recompiled against the newer header to use that newer macro definition. There may be other ABI breaks, but that one is obvious. We can't do ABI breaks in a stable version, or it's not stable.
 
> If you see at comment #9, PostgreSQL provider works with no modifications. The
> actual problems with SQLite provider is related with the assumptions on what a
> GDA's null means, if it uses public API this must not be the case (see at
> gda-holder.c, it defines its own gda_value_is_null function of internal use).

Yes, that code is no fun to fix, but we must fix it. "make check" must succeed.

Maybe you would like to put the work so far  in a branch of master if it lots more work is needed.

> I would love to see this to happend on 4.2, because I would like to see a
> transition from 4.x to 5.x, the same way as 2.x to 3.x of Gtk, with a good
> support of bindings using GObject Introspection.

Use of libgda 4.2 implies use of GTK+ 2 anyway (because libgda 4.2 requires GTK+ 2, due to unwise bundling of libgda and libgda-ui together), and people using GTK+ 2 will have no big reason to switch from pygtk to PyGObject+Introspection.

So I don't think it's worth the risk, unless someone is really asking for it.

(In reply to comment #15)
> (In reply to comment #14)
> > Why do you think it is an API change? I try to not modify API for NULL types.
> 
> It's API addition, at the least, which doesn't strictly belong in a stable
> version.
> 
> But more importantly, it's a change to how GValue should be used for null
> types. There's a fairly strong chance that someone is checking the GValue's
> GType for NULL.
> 
> Even more importantly, it's an ABI change because you are changing the
> definition of GDA_TYPE_NULL from 0 to the result of a function call.
> Application code must be recompiled against the newer header to use that newer
> macro definition. There may be other ABI breaks, but that one is obvious. We
> can't do ABI breaks in a stable version, or it's not stable.
> 
> > If you see at comment #9, PostgreSQL provider works with no modifications. The
> > actual problems with SQLite provider is related with the assumptions on what a
> > GDA's null means, if it uses public API this must not be the case (see at
> > gda-holder.c, it defines its own gda_value_is_null function of internal use).
> 
> Yes, that code is no fun to fix, but we must fix it. "make check" must succeed.
> 
> Maybe you would like to put the work so far  in a branch of master if it lots
> more work is needed.
> 
> > I would love to see this to happend on 4.2, because I would like to see a
> > transition from 4.x to 5.x, the same way as 2.x to 3.x of Gtk, with a good
> > support of bindings using GObject Introspection.
> 
> Use of libgda 4.2 implies use of GTK+ 2 anyway (because libgda 4.2 requires
> GTK+ 2, due to unwise bundling of libgda and libgda-ui together), and people
> using GTK+ 2 will have no big reason to switch from pygtk to
> PyGObject+Introspection.
> 
> So I don't think it's worth the risk, unless someone is really asking for it.

I agree with all.

Then this changes must be on master. I haven't access to a branch in order to fix all issues, most of them are for UI extension (I've tested on console and bindings and it works).

At the end I've tested your patch for master and works as the one for 4.2 branch.

I'll investigate about "make check" in order to make it success.

But now we need:

a) State that 4.x series is incompatible with GObject-Introspection due to its manage of GType for null values

b) State in documentation how to deal with Null values on bindings when using Gda 4.x series, maybe adding a try/catch in order to catch GType Error

Review of attachment 189426 [details] [review]:

This patch is against LIBGDA_4.2 branch, witch never gets GdaNull functionality in order to keep API/ABI stable. Then it is rejected.

The GDA_TYPE_NULL is now a boxed type, see http://git.gnome.org/browse/libgda/commit/?id=3f7456ef94b1b0f539267d02f32d8563dc9bb541
Tell me if you find any bug with that change.
Vivien

Excelent patch!!! Thanks.

When try to see the contents in a table called 'test' in a SQLite database defined as:

CREATE TABLE test (id integer, id text, notes, text);
INSERT INTO test (description) VALUES ("Text1");

table 'test' contents:

id   | description | notes
-----+-------------+------
NULL | Text1       | NULL 

Python bindings work well:

$ python
Python 2.7.1 (r271:86832, Apr 12 2011, 16:15:16) 
[GCC 4.6.0 20110331 (Red Hat 4.6.0-2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from gi.repository import Gda
>>> cnn = Gda.Connection.open_from_dsn("test", None, Gda.ConnectionOptions.READ_ONLY)
>>> m = cnn.execute_select_command("SELECT * FROM test")
>>> print m.dump_as_string()
id   | description | notes
-----+-------------+------
NULL | Text1       | NULL 
(1 row)

>>>  null = m.get_value_at(0,0)
>>> print null
None
>>> v = m.get_value_at(1,0)
>>> print v
Text1
>>> 

But I found errors on GdaBrowser, it fails to execute a SELECT * FROM test command and when you try to see the table contents.

Back trace:

** (gda-browser-5.0:2105): WARNING **: Failed to find the <parameter> tag
Trying to load plugins in /usr/lib64/libgda-5.0/plugins...
Cargando archivo /usr/lib64/libgda-5.0/plugins/libgda-ui-plugins.so…
  - loaded filesel (File selection entry): Entry
  - loaded cird (Entry to hold an IPv4 network specification): Entry
  - loaded text (Multiline text entry): Entry
  - loaded rtext (Rich text editor entry): Entry
  - loaded picture (Picture entry): Entry Cell
  - loaded picture_as_string (Picture entry for data stored as a string): Entry Cell
[New Thread 0x7ffff0039700 (LWP 2108)]
[New Thread 0x7fffef838700 (LWP 2109)]

** (gda-browser-5.0:2105): CRITICAL **: gdaui_entry_none_new: assertion `type != GDA_TYPE_NULL' failed

** (gda-browser-5.0:2105): CRITICAL **: gdaui_data_entry_set_value: assertion `GDAUI_IS_DATA_ENTRY (de)' failed

** (gda-browser-5.0:2105): CRITICAL **: gdaui_data_entry_set_reference_value: assertion `GDAUI_IS_DATA_ENTRY (de)' failed

** (gda-browser-5.0:2105): CRITICAL **: gdaui_data_entry_set_attributes: assertion `GDAUI_IS_DATA_ENTRY (de)' failed

(gda-browser-5.0:2105): GLib-GObject-CRITICAL **: g_object_ref_sink: assertion `G_IS_OBJECT (object)' failed

** (gda-browser-5.0:2105): CRITICAL **: gdaui_data_entry_set_editable: assertion `GDAUI_IS_DATA_ENTRY (de)' failed

(gda-browser-5.0:2105): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(gda-browser-5.0:2105): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(gda-browser-5.0:2105): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(gda-browser-5.0:2105): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(gda-browser-5.0:2105): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(gda-browser-5.0:2105): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

** (gda-browser-5.0:2105): CRITICAL **: gdaui_data_entry_set_attributes: assertion `GDAUI_IS_DATA_ENTRY (de)' failed

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff79d4ffe in gdaui_basic_form_set_entries_auto_default (form=<optimized out>, auto_default=0) at gdaui-basic-form.c:1978
1978			if (g_object_class_find_property (G_OBJECT_GET_CLASS (((SingleEntry*) list->data)->entry),
Missing separate debuginfos, use: debuginfo-install adwaita-gtk3-theme-3.0.2-1.fc15.x86_64 gtk3-3.0.10-1.fc15.x86_64 libgcc-4.6.0-9.fc15.x86_64 libgnome-keyring-3.0.2-2.fc15.x86_64
(gdb) bt

+ Trace 227481

I don't have the problem. Did you make sure you ran "make install" to remove any of the previous plugin DLLs?

I you'll rigth. There's no problem. I had to delete and recreate the test SQLite data base and the problem gone.

It's time to close this bug.

Ok, closing the bug then.
Thanks!

Thanks. This seems to be working well.

Unfortunately this is causing one problem, I think: Bug #647633

(In reply to comment #24)
> Unfortunately this is causing one problem, I think: Bug #647633

Are bug number currect? It's the same of this bug, could you point to the correct one please.

Thanks.

Sorry, I mean bug #652702.

That's fixed now. Thanks.

make check still fails one one test (check_threaded_cnc), but that didn't work before your change either. Closing.

It looks like this change was unintended :
http://git.gnome.org/browse/libgda/commit/libgda/gda-column.c?id=3f7456ef94b1b0f539267d02f32d8563dc9bb541
GDA_TYPE_NULL (a SQL NULL) does not seem like a suitable default database column type.

But it's probably too late (and too risky) to change it back now. I'm just mentioning it because I noticed it.