[asterisk-dev] [Code Review] 3335: [res_config_odbc] Fix for nullable integer columns and keyfield existence check in update_odbc

[wdoekes]

> On March 13, 2014, 3:52 p.m., Mark Michelson wrote:
> > Excellent job fixing the problem!
> > 
> > A note to whoever ends up committing this: This bug also exists in 1.8, 12, and trunk. The 1.8 fix is exactly the same as this one. The 12 and trunk fixes will be slightly different since the update_odbc() callback takes an ast_variable list instead of va_args. The fix is still very similar though.
> 
> zvision wrote:
>     I think this patch could rise some regression issues. While for PostgreSQL backend it is perfectly fine, for MySQL it can change the current behavior.
>     With MySQL backend, setting an integer column to an empty string will result in storing a zero integer value - with this patch, if the column is nullable,
>     it will store a NULL value instead.
>     Maybe it is not a big deal, but some third-party applications reading MySQL database may start finding unexpected NULL values instead of zeros.

> I think this patch could rise some regression issues.

Very true. That should not be taken lightly.


- wdoekes


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


On March 14, 2014, 4:32 p.m., zvision wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3335/
> -----------------------------------------------------------
> 
> (Updated March 14, 2014, 4:32 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23459
>     https://issues.asterisk.org/jira/browse/ASTERISK-23459
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch fixes setting nullable integer columns to NULL instead of an empty string, which fails for PostgreSQL, for example.
> The current code is supposed to do so, but the check is broken. The patch also allows the first column in the list to be a nullable integer.
> Also, the check for existence of a mandatory column checked for the first column in the list instead of the key field lookup column.
> 
> 
> Diffs
> -----
> 
>   http://svn.asterisk.org/svn/asterisk/branches/12/res/res_config_odbc.c 410554 
> 
> Diff: https://reviewboard.asterisk.org/r/3335/diff/
> 
> 
> Testing
> -------
> 
> Tested by me. Use case scenario: Asterisk + res_odbc + PostgreSQL backend, SIP realtime peers + regs.
> When a 'port' column in SIP regs (I assume this also applies when using sippeers only) is a nullable integer,
> Asterisk tries to write an empty string here during SIP endpoint deregistration.
> 
> 
> Thanks,
> 
> zvision
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140317/b06ed742/attachment-0001.html>