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

[Tilghman Lesher]

> 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.

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


- Tilghman


-----------------------------------------------------------
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/153a137b/attachment.htm>