[asterisk-dev] [Code Review] 3923: CallerID: Fix parsing of malformed callerid strings

[Mark Michelson]

> On Aug. 19, 2014, 6:05 p.m., rmudgett wrote:
> > branches/1.8/tests/test_callerid.c, line 608
> > <https://reviewboard.asterisk.org/r/3923/diff/1/?file=66615#file66615line608>
> >
> >     Rather than a bunch of redundant tests checking minor variations in the string.  How about one test that works through an array of input string variatons and the expected broken out strings:
> >     struct expect {
> >       char *orig;
> >       char *name;
> >       char *number;
> >     };
> >     struct expect array[] = {
> >       { "name <number>", "name", "number" },
> >       { "\"name\" <number>", "name", "number" },
> >       { "name", "name", NULL },
> >       { "1234", NULL, "1234" }
> >     }
> >     
> >     The last two entries above are also valid possibilities for the input string to ast_callerid_parse().

Hm, I see that this has a check mark as being resolved, so I may be wasting my breath here. I want to weigh in and say that I like the idea of having separate tests rather than a single one. That way, if there is a regression, it's eaiser to see that cases A, B, F, and H pass while cases C, D, E, and G fail. They're redundant, yes, and it may be possible to factor out some test logic into common functions, but having them as separate tests is a positive thing if you ask me.

If you've already combined these into a single test, though, then nevermind, just leave it like it is.


> On Aug. 19, 2014, 6:05 p.m., rmudgett wrote:
> > branches/1.8/main/callerid.c, line 1111
> > <https://reviewboard.asterisk.org/r/3923/diff/1/?file=66611#file66611line1111>
> >
> >     I am wondering if ast_callerid_parse() should be escaping the quotes at all.  The job of ast_callerid_parse() is to take the string and separate it into a name and number string.  Escaping characters seems wrong.  If anything, the routine should be unescaping character sequences.  The consumers likely don't know how to handle escaped quotes or may need to escape characters differently.
> 
> opticron wrote:
>     You make a good point. This is going to take a bit to rework since I need to identify consumers of callerid information and have them escape quotes where necessary.

Remember also that depending on the context you're in, escaping quotes may need to be done in different ways. In URLs, quotes are escaped as %22, whereas in XML they're escaped as " . So I agree that a core function like this shouldn't be trying to escape quotation marks on its own.


- Mark


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


On Aug. 19, 2014, 1:55 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3923/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2014, 1:55 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This allows the callerid parsing function to handle malformed input strings and strings containing escaped and unescaped double quotes. This also adds a unittest to cover many of the cases where the parsing algorithm previously failed.
> 
> 
> Diffs
> -----
> 
>   branches/1.8/tests/test_callerid.c PRE-CREATION 
>   branches/1.8/res/res_agi.c 421326 
>   branches/1.8/main/privacy.c 421326 
>   branches/1.8/main/manager.c 421326 
>   branches/1.8/main/callerid.c 421326 
>   branches/1.8/include/asterisk/callerid.h 421326 
>   branches/1.8/channels/chan_unistim.c 421326 
>   branches/1.8/channels/chan_misdn.c 421326 
>   branches/1.8/apps/app_voicemail.c 421326 
>   branches/1.8/apps/app_dial.c 421326 
> 
> Diff: https://reviewboard.asterisk.org/r/3923/diff/
> 
> 
> Testing
> -------
> 
> Ran the unittest.
> 
> 
> Thanks,
> 
> opticron
> 
>

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