[Home]

Summary:ASTERISK-14077: [patch] Fetching a NULL value from database returns "NULL" string
Reporter:Chris Maciejewski (chris-mac)Labels:
Date Opened:2009-05-06 15:25:18Date Closed:2009-05-20 16:48:10
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Addons/app_mysql
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090506_bug15045.diff.txt
( 1) 20090507_bug15045.diff.txt
( 2) 20090520_bug15045.diff.txt
Description:How to reproduce (asumming SQL query "SELECT some_null_col
FROM table WHERE where id = 1" returns NULL):

extensions.conf:
exten => 601,1,MYSQL(Connect connid1 ${DBHOST} ${DBUSER} ${DBPASS} ${DBNAME})
exten => 601,n,MYSQL(Query resultid1 ${connid1} SELECT some_null_col
FROM table WHERE where id = 1)
exten => 601,n,MYSQL(Fetch fetchid1 ${resultid1} X-DB-null)
exten => 601,n,MYSQL(Clear ${resultid1})
exten => 601,n,MYSQL(Disconnect ${connid1})
exten => 601,n,NoOp(X-DB-null: ${X-DB-null})
exten => 601,n,NoOp(ISNULL = ${ISNULL(${X-DB-null})})

console:
  -- Executing [601@test:1] MYSQL("SIP/OpenSER-082cf100", "Connect
connid1 mysql-ro asterisk pass1 test") in new stack
  -- Executing [601@test:2] MYSQL("SIP/OpenSER-082cf100", "Query
resultid1 1 SELECT some_null_col FROM table WHERE where id = 1") in
new stack
  -- Executing [601@test:3] MYSQL("SIP/OpenSER-082cf100", "Fetch
fetchid1 2 X-DB-null") in new stack
  -- Executing [601@test:4] MYSQL("SIP/OpenSER-082cf100", "Clear 2")
in new stack
  -- Executing [601@test:5] MYSQL("SIP/OpenSER-082cf100",
"Disconnect 1") in new stack
  -- Executing [601@test:6] NoOp("SIP/OpenSER-082cf100", "X-DB-null:
NULL") in new stack
  -- Executing [601@test:7] NoOp("SIP/OpenSER-082cf100", "ISNULL =
0") in new stack

So looks like NULL value fetched from a MySQL table is converted into a 'NULL' string and ISNULL function returns 0.
Comments:By: Chris Maciejewski (chris-mac) 2009-05-06 16:05:17

Patch attached: Don't convert values returned from DB to "NULL" string, but return them as they are. If they are NULL return NULL, if they are an empty string '' return an empty string etc.
By: Chris Maciejewski (chris-mac) 2009-05-07 03:14:45

Patch 20090507_bug15045.diff.txt  attached: previous diff file wasn't created in a correct directory.
By: Atis Lezdins (atis) 2009-05-07 10:22:57

The problem is that there is no distinction within Asterisk variables
between empty and NULL. So, Your patch would just return empty string
which could be perfectly valid string response (not NULL). I believe
that it's more uncommon to have actual data in MySQL to contain string
"NULL" than empty string, so it seems a good choice. Changes in this
could break some dialplans, which would lead to unsatisfied users.

The total solution of course would be to have distinction in Asterisk
between string not set and empty string.
By: Dmitry Andrianov (dimas) 2009-05-07 12:46:05

null=>"" is definitely much better that null=>"null".
It does not really matter how common is to have column value set to "null" - returning ANY non-empty text in case of NULL-value was a bad idea at the first place.
By: Chris Maciejewski (chris-mac) 2009-05-08 08:20:28

Yes, I completely agree that "" is much better idea than "NULL". Perhaps the patch should look like:

pbx_builtin_setvar_helper(chan, s5, s6 ? s6 : "");

instead of

pbx_builtin_setvar_helper(chan, s5, s6);

but the current version:

pbx_builtin_setvar_helper(chan, s5, s6 ? s6 : "NULL");

is just a really bad idea.
By: Atis Lezdins (atis) 2009-05-08 08:29:07

Well, it is very inconvenient to have string "NULL" (it took me 15 minutes for the first time to figure it out), but if it would be "" there would be no way how to distinguish between NULL and empty string. Also that change would break some existing dialplans.


By: Dmitry Andrianov (dimas) 2009-05-08 08:29:36

what is wrong with
 pbx_builtin_setvar_helper(chan, s5, s6);

?
setting var to null basically deletes it. And it is even better than empty string. if you attempt to access the variable as ${VAR} it will be the same as empty string while by executing core show channel XXX you will be able to see if given variable actually present (non-null) or not.
By: Dmitry Andrianov (dimas) 2009-05-08 08:33:08

atis, do YOU really differentiate in your dialplans betwenn NULLs and "" ?
Will your dialplan really break if you receive "" ?
Please be honest.

Every dialplan i saw account for the fact that field can be empty.
Even if you have special handling for "NULL" you usually still check for "" as well.
By: Chris Maciejewski (chris-mac) 2009-05-08 08:35:09

atis: what if it happens there is a "null" string stored in a database which should NOT be treated as NULL. Possibly in non-English languages there is a word "null" mining something completely different?
By: Atis Lezdins (atis) 2009-05-08 08:40:54

dimas, that un-setting might be a good start, but i guess ISNULL should be modified (or some new function created) to distinguish in dialplan between "" and not set.

Of course, my dialplan won't break, i somehow added or check for empty string :)

chris-mac - that's what i meant in initial comment - having database contain string "NULL" is much less likely than having "".
By: Chris Maciejewski (chris-mac) 2009-05-08 08:48:47

Ideally we would need one more Dialplan function called ISSET, which would check if variable is initialized. ISNULL would return 1 only if variable is set and it's value is NULL. So Asterisk behaves as follow:

exten => s,1,MYSQL(Connect connid1 ${DBHOST} ${DBUSER} ${DBPASS} ${DBNAME})
... here we fetch NULL from database which is assigned to X-NULL channel var ...
exten => s,n,Set(X-String=Some String)
exten => s,n,Set(X-Empty-String=)
exten => s,n,NoOp(ISSET(${X-NULL}) => 0
exten => s,n,NoOp(ISSET(${X-String}) => 1
exten => s,n,NoOp(ISSET(${X-Empty-String}) => 1
exten => s,n,NoOp(ISSET(${X-Not-Set-Yet}) => 0
exten => s,n,NoOp(ISNULL(${X-NULL}) => 1
exten => s,n,NoOp(ISNULL(${X-String}) => 0
exten => s,n,NoOp(ISNULL(${X-Empty-String}) => 0
exten => s,n,NoOp(ISNULL(${X-Not-Set-Yet}) => 1

this would be an equivalent of PHP is_null http://uk.php.net/manual/en/function.is-null.php and isset http://uk.php.net/manual/en/function.isset.php


By: Dmitry Andrianov (dimas) 2009-05-08 09:11:06

That is very easy to create function like DEFINED or ISDEF. The function code would be just:

static int defined(struct ast_channel *chan, char *cmd, char *data,
                 char *buf, size_t len)
{
       const char *x = ast_strlen_zero(data) ? NULL : pbx_builtin_getvar_helper(s->chan, data);

       snprintf(buf, len, "%d", x != 0);

       return 0;
}

the usage will be DEFINED(varname) not DEFINED(${varname})
By: Atis Lezdins (atis) 2009-05-08 09:36:26

dimas, you're quick :)

chris-mac: Ideally Yes - and there should be also way to check for equals empty string (ISEMPTY?). Currently You can just do $["${var}"=""], but it would be true also on NULL etc.

However, I'm quite skeptical about doing those changes anywhere in 1.6.x code.

So, there are two things that will change - MySQL not returning "NULL" and function ISNULL.

But generally, a good concept. I was thinking that it would be more complex :)
By: Tilghman Lesher (tilghman) 2009-05-11 11:49:09

I think this needs a configuration option, to avoid breaking existing dialplans.  Related issue implements an initial configuration file, since this module previously had none.
By: Tilghman Lesher (tilghman) 2009-05-18 17:41:54

Patch still needs an update.
By: Chris Maciejewski (chris-mac) 2009-05-19 02:42:33

I will upload an updated patch by the end of this week. My idea is to use a new configuration file like:

mysql.conf:
[general]
nullasstring = yes  ;Return NULL and '' values as 'NULL' string (default behaviour), or as NULL when set to no

Could you please let me know if the above will be OK?
By: Dmitry Andrianov (dimas) 2009-05-19 03:40:40

Hm... If it was not addon, the normal place for this kind of parameter would be [compat] section of asterisk.conf and it would look like:

addon_sql_mysql=1.6  ; for NULLs to be returned as "" (default)
addon_sql_mysql=1.4  ; to return NULLs as "NULL"

Maybe we still can do the same for addons too...


By: Tilghman Lesher (tilghman) 2009-05-19 10:14:34

chris-mac:  I think that's the right way to go, with the exception that blank values should remain to be returned as blank.  Note that you can verify the difference, by using a function to test whether the result is the NULL value (as opposed to the "NULL" string).

dimas:  No, given that we already have 2 settings, a separate file is better.  Also, I disagree on your default.  The default setting should be the CURRENT behavior in any released version (so that nothing changes for people upgrading to a bug-fix release).  We might consider changing the default in an unreleased version, but that discussion needs to be held on the -dev list, not here.


By: Chris Maciejewski (chris-mac) 2009-05-20 09:01:05

Updated patch uploaded. The behaviour of MYSQL addon is as follows:

1. If there is a mysql.conf file:

[general]
nullasstring = no

  NULL is returned as NULL

2. If there is no mysql.conf or nullasstring = yes NULL is returned as 'NULL' string.
By: Dmitry Andrianov (dimas) 2009-05-20 09:32:17

To me personally "nullasstring" name is meaningless.
I would think about something like:

nullvalue = null       ; real C NULL
nullvalue = empty      ; empty string
nullvalue = nullstring ; "NULL"
By: Tilghman Lesher (tilghman) 2009-05-20 12:45:15

chris-mac: I think I like dimas's suggestion for the option better.  It leaves the possibility for more options in the future.
By: Chris Maciejewski (chris-mac) 2009-05-20 13:30:11

I agree "nullasstring" is maybe not the best name. But in my opinion whatever name of this option will be it should be boolean true/false (for current broken incorrect behaviour, and when NULL is NULL and '' is '').

I suggest it is *much* better to return the actual value from DB, rather than temper with it again.

So it should be (as it is in the latest patch):

if (somenicename) {
 /* Current broken behaviour - return NULL and '' as 'NULL' */
 pbx_builtin_setvar_helper(chan, s5, s6 ? s6 : "NULL");
} else {
 /* New behaviour - return whatever is in DB, so if it is NULL return NULL, if it is '' return '' */
 pbx_builtin_setvar_helper(chan, s5, s6);
}

And it should *NOT* be (as suggested by dismas):

if (nullvalue == 'null') {
 /* Current broken behaviour - return NULL and '' as 'NULL' */
 pbx_builtin_setvar_helper(chan, s5, s6 ? s6 : "NULL");
} else if (nullvalue == 'empty') {
 /* New broken behaviour!!! - return NULL and '' as '' */
 pbx_builtin_setvar_helper(chan, s5, s6 ? s6 : "");
} else {
 /* New behaviour - return whatever is in DB, so if it is NULL return NULL, if it is '' return '' */
 pbx_builtin_setvar_helper(chan, s5, s6);
}
By: Tilghman Lesher (tilghman) 2009-05-20 14:06:40

You seem to be under the mistaken impression that "" evaluates to false in C.  It does not.  It evaluates to true.  Only NULL evaluates to false.

So the code works out to:

pbx_builtin_setvar_helper(chan, s5, s6 ? s6 :
   nullvalue == NULLSTRING ? "NULL" :
   nullvalue == EMPTYSTRING ? "" :
   NULL);

A value which is an empty string in the database is COMPLETELY unaffected.


By: Chris Maciejewski (chris-mac) 2009-05-20 14:11:22

Yes, "" is true in C, but not in Asterisk dialplan. In Asterisk dialplan NULL and "" evaluates to false (AFAIK)? And at the end of the day, we need MySQL addon to return values which will be interpreted correctly in dialplan, and not in C.
By: Tilghman Lesher (tilghman) 2009-05-20 14:17:44

chris-mac:  please try it, instead of telling us both what we know it will not do.
By: Dmitry Andrianov (dimas) 2009-05-20 14:31:20

tilghman,
I think you have miss-communication with chris-mac here. He is not stating that " if (value) " will evaluate to false in C for value="". He just thinks that nullvalue=empty option is broken anyway and should not be used - this is because there will be no way to know later if the value in the DB was really empty or it was it NULL. So we just replace one ambiguous value ("NULL") with another ("").

I can agree that single boolean option may be enough instead of configurable nullvalue. But "nullasstring" is still not the best name for it.


By: Chris Maciejewski (chris-mac) 2009-05-20 14:36:36

dismas: thanks! I couldn't explain my point better :-)

"nullvalue=empty option is broken anyway and should not be used - this is because there will be no way to know later if the value in the DB was really empty or it was it NULL"

This is exactly what I wanted to communicate.
By: Tilghman Lesher (tilghman) 2009-05-20 14:41:24

dimas:  No, he's saying that this option will affect how blank values in the database will manifest in the dialplan, which is manifestly false.  His comment says "Current broken behaviour - return NULL and '' as 'NULL'".  This is FALSE.  NULL is currently returned as "NULL", but "" is currently returned as "".  Each of the options specified does not affect how "" is returned.

In fact, I would argue against setting variables to the NULL value because of how this affects Gosub and LOCAL() variables.  I'm open to the idea of letting people shoot themselves in the foot, but unless blank is an option for NULL, I have to veto any such patch.
By: Atis Lezdins (atis) 2009-05-20 14:45:06

Dialplan still needs a way to distinguish between NULL and ""

Perhaps this nullstring=empty could be made global compatibility option (default), modifying also function ISNULL to act correctly. Then just add another function ISEMPTY for empty strings as replacement of ISNULL.
By: Dmitry Andrianov (dimas) 2009-05-20 14:58:16

tilghman,
ah, I see what you mean. However I would consider this comment in his code a sort of typo :) The code is still correct:

if (oldnulls) {
 /* Old broken behaviour - return NULL as 'NULL' */
 pbx_builtin_setvar_helper(chan, s5, s6 ? s6 : "NULL");
} else {
 /* New behaviour - return whatever is in DB*/
 pbx_builtin_setvar_helper(chan, s5, s6);
}

Could you please explain what kind of problems NULL produce for GoSub and LOCAL?

atis,
unfortunately you can not change behavior of ISNULL because some dialplans may depend on it. So you can not change ISNULL and add ISEMPTY - you need to keep ISNUL and add something ISDEF/ISDEFINED instead...
By: Tilghman Lesher (tilghman) 2009-05-20 15:33:17

dimas:  sure.  The way LOCAL() variables work is that they are pushed onto the stack of variables, hiding any previously set variables with the same name.  When the stack is pulled (via StackPop or Return), the variables are similarly pulled.  When you set a variable to NULL, that is the same operation.  So if MYSQL set NULL values to NULL within a Gosub routine, it could potentially screw up variables at higher levels.
By: Atis Lezdins (atis) 2009-05-20 15:51:56

dimas: that's why i was proposing nullstring option global in compat section. It would affect both app_addon_mysql and ISNULL.

The ISNULL is another bad naming chosen in past, compatibility of course should be maintained. If somebody choses to fix mysql return values, he can also check for all uses of ISNULL.

The ISDEFINED seems to be good naming, but often you have to check for opposite. It could work in combination with UNDEFINED function :)

Btw, how are NULL values returned from res_odbc?
By: Tilghman Lesher (tilghman) 2009-05-20 15:54:45

atis: from res_odbc or func_odbc?  res_odbc doesn't return values, but func_odbc does.  In func_odbc's case, NULL values are returned as empty strings.
By: Tilghman Lesher (tilghman) 2009-05-20 15:57:45

"nullvalue=empty option is broken anyway and should not be used - this is because there will be no way to know later if the value in the DB was really empty or it was it NULL"

This is not negotiable.  This must be one of the options.  If you want, I'll add it to your patch, but for the reasons of Gosub and LOCAL() variables, the option of setting NULL values as empty MUST be available.
By: Chris Maciejewski (chris-mac) 2009-05-20 16:13:02

tlighman: Yes, please modify my patch as you see fit.

Thanks
Chris
By: Digium Subversion (svnbot) 2009-05-20 16:48:08

Repository: asterisk-addons
Revision: 916

U   trunk/apps/app_addon_sql_mysql.c
A   trunk/configs/mysql.conf.sample

------------------------------------------------------------------------
r916 | tilghman | 2009-05-20 16:48:07 -0500 (Wed, 20 May 2009) | 7 lines

Allow the NULL value in a mysql database to be retrieved as different values.
(closes issue ASTERISK-14077)
Reported by: chris-mac
Patches:
      20090520_bug15045.diff.txt uploaded by chris-mac (license 506)
      with additional modifications by myself

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk-addons?view=rev&revision=916