[asterisk-dev] [Code Review] extra ast_strlen_zero() check in cel_odbc prevents integer eventtype in database

[mlehner]

> On 2011-03-26 17:12:24, Tilghman Lesher wrote:
> > This change reverts revision 236847, which was reported on the -dev list by Nic Colledge.  You need to address his comments, before this can go forward:
> > http://lists.digium.com/pipermail/asterisk-dev/2009-December/041362.html
> 
> Tilghman Lesher wrote:
>     It's probably better for you to specify a default in your table, instead of making a source code change.  A blank is almost never a good option for an enum value (especially as it applies to Asterisk).
> 
> mlehner wrote:
>     The checks I removed caused the eventtype field to always be NULL when the column was an integer. This is not an issue of a default value, nor does this patch revert revision 236847. The extraneous checks for integer values were added (to the best of my searching) when the file was renamed from cel_adpative_odbc.c to cel_odbc.c. There is no comment in the SVN history as to why the extra checks were added at that time. My patch removes these checks and actually brings the code closer to revision 236847.
> 
> Tilghman Lesher wrote:
>     No, the checks you removed cause the field, if unspecified, to take the default value.  In your case, it seems that the default value is NULL.  If you change the default value, the insert will insert that value, instead.
>     
>     I've already pointed you to the exact contents of revision 236847.  I'll link to the log here, since you seem to be unwilling to accept that your patch reverts that revision:
>     
>     $ svn pg --revprop -r 236847 svn:log http://svn.asterisk.org/svn/asterisk
>     When the field is blank, don't warn about the field being unable to be coerced, just skip the column.
>     (closes http://lists.digium.com/pipermail/asterisk-dev/2009-December/041362.html)
>     Reported by Nic Colledge on the -dev list, fixed by me.
>     
>     SVNView is having trouble this evening, but the info can usually be found here:  http://svnview.digium.com/svn/asterisk/trunk/cel/cel_adaptive_odbc.c?view=log&pathrev=236847
> 
> mlehner wrote:
>     You are not understanding the problem at all.
>     
>     If eventtype column is of type integer, it will *always be null* without my patch. Please refer to lines 404 through 441 where the various values to be inserted are converted to strings for use in an SQL query. The eventtype field is *skipped* in these lines, and thus colptr is an empty string (the last else branch of that if statement). Inside the switch statement is where eventtype value is converted to a string for the SQL query. However, without my patch all the code to convert eventtype to a string is *skipped* because the value is first incorrectly checked if it's the empty string.. and as I previously pointed out it is a empty string because of lines 404-441. So eventtype is always skipped if the column is an integer.
>     
>     Setting or not setting a default value will not resolve this problem. This problem has zero to do with the post by Nic Colledge. Yes you fixed his problem with revision 236847, and yes that revision has the proper working code. But the latest version of cel_odbc.c from trunk (or 1.8.3.2) has the broken version of cel_odbc.c.

The ast_strlen_zero() check I removed from lines 574 - 576 exist at lines 580 - 582.
The ast_strlen_zero() check I removed from lines 593 - 595 exist at lines 599 - 601.
The ast_strlen_zero() check I removed from lines 612 - 614 exist at lines 618 - 620.
The ast_strlen_zero() check I removed from lines 631 - 633 exist at lines 637 - 639.
The ast_strlen_zero() check I removed from lines 650 - 652 exist at lines 656 - 658.
The ast_strlen_zero() check I removed from lines 672 - 674 exist at lines 678 - 680.
The ast_strlen_zero() check I removed from lines 693 - 695 exist at lines 699 - 701.

As I already stated, the checks I removed were redundant, overzealous, extraneous and caused the code that filled in the value for eventtype (iff column type was an integer) to be skipped.


- mlehner


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1149/#review3253
-----------------------------------------------------------


On 2011-03-26 11:43:38, mlehner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1149/
> -----------------------------------------------------------
> 
> (Updated 2011-03-26 11:43:38)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> event_type could not be an integer datatype in a ODBC database because an extra ast_strlen_zero() check was wrapped around the code that did the work. ast_strlen_zero() check is done in the second branch of the if statement, after the check for eventtype is done. Changes are very simple, just removing the "overzealous" ast_strlen_zero() check, while keeping braces to prevent compilation errors and warnings with variable re-definitions. 
> 
> 
> Diffs
> -----
> 
>   /tags/1.8.3.2/cel/cel_odbc.c 311686 
> 
> Diff: https://reviewboard.asterisk.org/r/1149/diff
> 
> 
> Testing
> -------
> 
> Tested on a local install of Asterisk 1.8.3.2. Compiles and creates the expected records in the database.
> 
> 
> Thanks,
> 
> mlehner
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110329/98b2f720/attachment-0001.htm>