[Home]

Summary:ASTERISK-06524: [patch] [post-1.4] Empty strings in database is not returned as a variable value
Reporter:Ove Aursand (aurs)Labels:
Date Opened:2006-03-11 19:27:54.000-0600Date Closed:2007-01-07 10:21:51.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Applications/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20060926__bug6701.diff.txt
( 1) bug6701-asterisk-1.2.diff
( 2) bug6701-asterisk-addons-1.2.diff
( 3) bug6701-asterisk-trunk.diff
Description:Database table:
mysql> select * from testdb;
+-------+------+
| col1  | col2 |
+-------+------+
| test1 | test |
| test2 |      |
+-------+------+
2 rows in set (0.00 sec)

Asterisk CLI:
sip*CLI> realtime load testdb col1 test1
                  Column Name  Column Value
         --------------------  --------------------
                         col1  test1
                         col2  test
That is good...
But what happens on test2?
sip*CLI> realtime load testdb col1 test2
                  Column Name  Column Value
         --------------------  --------------------
                         col1  test2
col2 is not returned.
I got help from Qwell on the #asterisk-bugs irc channel. He looked through the code and adviced me to change line 151 in res_config_mysql.c (asterisk-addons-1.2.2)
from if(chunk && !ast_strlen_zero(ast_strip(chunk))) {
to
if(chunk /*&& !ast_strlen_zero(ast_strip(chunk))*/) {

This solved the problem. The same problem and solution applies to the res_config_odbc.c file.
Comments:By: Jason Parker (jparker) 2006-03-11 19:37:29.000-0600

I completely agree that this is a bug, but I'm hesistant to upload that patch as a "solution" to this problem.  I imagine that other functions that rely on ast_load_realtime will have problems with "empty" fields - which is probably why this ast_strlen_zero check is there in the first place.

This also affects res_config_odbc, and all others, I assume.

I have a few ideas on how this can be done.  Some are probably more "proper".

1) Create a new function ast_load_realtime_full, which will, as the name implies, return ALL results, regardless of their value (or, at least, null/empty ones).

2) Add a flag to ast_load_realtime, "int full", which does the same as above, if "full" is true.

3) Make it always return all results - and see what happens to other functions (yes, I'm {mostly} joking).


If somebody can give some input on this, I can write the code and upload the patch.

Also, can anybody think of other places besides res_config_mysql and res_config_odbc that might have these calls?  I recall hearing of some ldap and pgsql drivers...a) have those been put into svn?, b) would those have the same res_config_* files?
By: Tilghman Lesher (tilghman) 2006-03-11 22:31:18.000-0600

I vote for solution 3.  The problem appears to be a symptom of an optimization that wasn't completely thought all the way through.  Let's remove the optimization and let the system set variables for all columns, zero-length or no.
By: Ove Aursand (aurs) 2006-03-12 04:37:39.000-0600

Won't solution 3 destroy other functions? Like sip. In that case, the correct behaviour is not to return empty fields as variables. (Have not tested sipusers/sippeers after "fixing" this bug, but I suppose there might be a problem there.)
In the dialplan, this is a problem if you do several calls to the RealTime() funktion in the same call. Typically in a IVR menu. If the application stumbles upon a blank value in the database, then this particular variable keeps the value from the previous RealTime() call. To continue on north's ideas... Could this be solved by adding an extra argument to the RealTime() command?
RealTime(<family>|<colmatch>|<value>[|<prefix>[|0/1]]) where 1 gives you everything, including empty fields, and 0 is the same as before?
By: Tilghman Lesher (tilghman) 2006-03-12 08:26:41.000-0600

aurs:  I'm not understanding your point.  Please give me an example where this would be undesireable behavior.
By: Ove Aursand (aurs) 2006-03-12 08:54:43.000-0600

Hi Corydon76.

Example:
[macro-menugotoif]
exten => s,1,RealTime(testdb|family|${ARG1}/ivr${ARG2})
IVR choices are saved in table as this:
family        value
exten/ivr001  destination

Destination can be another IVR menu. So if I in the first IVR make a choice that puts me trhough to another IVR, and then in the next IVR menu choose a value that is blank in col2, then the variable $value sill has the value from the previous choice that was made. In this case that would only bring us back to the same IVR again.
We have tried to prevent this, using unique prefixes in the RealTime() call, but still, it would be nice if the variable was set, even if the field is empty.
Did that description help? This can also be a problem in other places. Like when a incoming call is placed, we check RealTime(testdb|family|exten/HN). And (testdb|family|exten/CFIM). And lots of other stuff. The first RealTime() could get $value="22222222" or similar. Then if we check exten/CFIM, that one could be empty, thus $value will still be "22222222", and The call is forwarded to extension 22222222. We are avoiding that particular issue with unique prefix, but that is not always an option, and it probably shouldn't be necessary.

Hope this cleared things up.
aurs
By: Tilghman Lesher (tilghman) 2006-03-12 10:49:44.000-0600

No, you just made the case for option 3.  I asked for a case where overwriting the previous values would be bad, and you gave me the case where overwriting the previous values would be GOOD.  That case is not in dispute.
By: Ove Aursand (aurs) 2006-03-12 11:52:25.000-0600

I see. I was just saying that with sip peers in realtime, it is probably a bad idea to set variables if the field is empty. The sip peer table have things like username varchar(xx), qualify varchar(xx) and so on.

If the qualify field is empty in the database, that will be the equal to putting nothing in sip.conf. If the qualify field in database has value "yes", that would be equal to putting qualify=yes in sip.conf. So the 3) solution mentioned here will probably make empty qualify field in database be equal to putting "qualify=" in sip.conf. And that is probably not a good thing? The same applies for realtime queues, and probably other realtime families as well.

That was what you were asking for, right?
aurs
By: Tilghman Lesher (tilghman) 2006-03-12 16:09:43.000-0600

An empty in the database would indicate that we should do whatever the default is (in the case of qualify, the default is no).  That is the expected behavior, and I think that's exactly what what we should do.
By: Ove Aursand (aurs) 2006-03-13 07:13:33.000-0600

Update: Have done some more testing. To continue on the qualify example; the fields in the database is not passed as variables if their values is NULL (mysql). Good thing. If they have a empty string as value, they are passed as variables. So what I feared about the realtime sip peers did not happen, since all the empty fields in that table were set to NULL.
So this fix does exactly what I wanted it to. I also tried with a similar fix on the res_config_odbc.c file, tested it, and it behaved the same way as res_config_mysql.
aurs
By: Ove Aursand (aurs) 2006-04-18 17:18:44

Hello north. Here is a reminder, as requested on #asterisk
By: Serge Vecher (serge-v) 2006-05-08 09:17:46

* ping *
By: Serge Vecher (serge-v) 2006-05-25 20:54:09

* tracert north
By: Jason Parker (jparker) 2006-06-02 14:59:20

Patches uploaded for the various repositories/versions.

asterisk-addons 1.2 patch can also be applied directly to asterisk-addons trunk, with no modifications.
By: Serge Vecher (serge-v) 2006-07-10 12:37:49

aurs: any feedback on north's patches?
By: Ove Aursand (aurs) 2006-07-11 09:59:18

The patch solved our problem. Have used it since december without problems. However, we are only using voicemail and a custom family in realtime (extconfig.conf).

Did some testing with the sipusers/sippeers at one time in the winter. It worked fine. Fields in the database with NULL value are still not parsed as variables (good thing), but fields with an empty string as value are parsed (also a good thing).

From my point of view; this patch solves the problem. Thanks :)
By: Jason Parker (jparker) 2006-07-13 18:38:52

Discussed this bug with Kevin and Russell at Digium.  They suggested we use option 1.

We need to go through and figure out which functions need to be changed to use the new function call.

Edit: This will be going into 1.2.


By: Tilghman Lesher (tilghman) 2006-07-13 21:34:11

After discussion between kpfleming, north, and myself, it was decided that this approach should rule.

aurs:  please test this patch by using res_config_odbc, instead of res_config_mysql and let us know how it works out.
By: Tilghman Lesher (tilghman) 2006-07-13 21:39:06

Actually, your change to addons will work with this, as well.
By: Ove Aursand (aurs) 2006-07-17 05:46:57

Hello. I've tested both res_config_mysql and res_config_odbc with this solution. Right now, we are running with the odbc driver on a postgresql database.

At the time I reported the bug, we were running the mysql driver. But after changing from mysql to pgsql, we swapped to odbc, and have had no problems with it.

Comment to norths comment on "approach 1)":
Just want to specify again that with this patch, NULL values from DB are NOT returned as variables, but VARCHAR fields with value "" (empty string) ARE returned as variables.
By: Tilghman Lesher (tilghman) 2006-07-17 12:43:57

Patch updated.  Please retest.
By: Serge Vecher (serge-v) 2006-09-01 14:21:50

aurs: need your feedback here
By: jmls (jmls) 2006-11-01 06:20:55.000-0600

aurs: need your feedback here
By: Ove Aursand (aurs) 2006-11-01 12:58:42.000-0600

Downloaded http://svn.digium.com/svn/asterisk/trunk and patched:
# patch -p0 <realtime.patch
patching file funcs/func_realtime.c
patching file include/asterisk/config.h
patching file main/config.c
Hunk #1 succeeded at 1301 (offset 156 lines).
patching file res/res_config_odbc.c
patching file res/res_realtime.c
#
Will compile and test as soon as possible.

Edit:
Asterisk has successfully been built, and can be installed by running: make install
Will try to test this before the end of the week.


By: Serge Vecher (serge-v) 2006-11-14 08:34:22.000-0600

aurs: what are the test results?
By: Ove Aursand (aurs) 2006-11-14 15:21:27.000-0600

vm01*CLI> realtime load test1 family hello
                  Column Name  Column Value
         --------------------  --------------------
                       family  hello
                        value  world
vm01*CLI>
running sql: update astdb set value='' where family='hello'
vm01*CLI> realtime load test1 family hello
                  Column Name  Column Value
         --------------------  --------------------
                       family  hello
                        value
vm01*CLI>

It works! Now trying to query a row with NULL values:
vm01*CLI> realtime load extensions extension test
                  Column Name  Column Value
         --------------------  --------------------
                    extension  test
                  reseller_id
               numbergroup_id
        primary_extension_ref
vm01*CLI>
(reseller_id, numbergroup_id and primary_extension_ref all have NULL value in database).

With the first fix, the fields with NULL values in db was not parsed as variables, so that would have given something like this:
vm01*CLI> realtime load extensions extension test
                  Column Name  Column Value
         --------------------  --------------------
                    extension  test
vm01*CLI>
Have not tested this in a "live" environment.

EDIT: Oh, yes. And the version I did the testing on: Asterisk SVN-trunk-r46782M built by root


By: Tilghman Lesher (tilghman) 2007-01-07 10:21:51.000-0600

Committed, rev 49801.