[Freeipa-devel] [PATCH] 0015 Only split CSV strings once (updated)

[Petr Viktorin]

On 02/29/2012 07:13 PM, Petr Vobornik wrote:
> On 02/27/2012 02:01 PM, Petr Viktorin wrote:
>> It seems I didn't communicate the problem and my solution clearly
>> enough, so let me try again. (Also, I learned from the discussions!)
>>
>> Currently, both the client and the server parse CSV options. The client
>> does *not* re-encode the CSV before sending; the parsing is really done
>> twice. This means e.g. that you need 3 backslashes to escape a literal
>> comma: after the client-side split, '\\\,' becomes '\,'; which after the
>> server-side split becomes ','.
>>
>>
>> Since CSV is specific to the command-line, and the client is responsible
>> for translating command-line input to XML-RPC (which has its own syntax
>> for lists), the ideal fix will be to move CSV processing entirely to the
>> client.
>> This will be a rather invasive change, mainly because some parts of the
>> UI now expect the server-side parsing (but they don't escape CSV, so
>> values containing commas or backslashes are broken). So it won't make it
>> to the upcoming release. My patch provides a quick fix: when a call
>> comes from the command-line client, disable the server-side parsing.
>
> I investigated all occurrences of CSV creation in Web UI. I removed them
> and UI is working fine. The patch is on the list: pvoborni 099. So your
> patch shouldn't affect UI if my patch is applied.
>
>>
>> I can't get away from moving split_csv() (which is not idempotent) out
>> of normalize() (which is, and gets called lots of times); this is the
>> patch's major change in therms of LOC.
>>
>>
>> I'll note again that this only affects values with backslashes or double
>> quotes. Exactly these options are currently broken (=need double
>> escaping). The "normal" uses of CSV are completely unaffected.
>>
>>
>> Attaching updated patch; the change vs. the original is that the "don't
>> parse again" flag is now only set at the server, when a XMLRPC call is
>> received, making the client fully forward-compatible (the flag doesn't
>> get sent through the wire).
>>
>>
>> The ticket is https://fedorahosted.org/freeipa/ticket/2227, but this
>> patch is only the first step in fixing it.
>>
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
>

This updated patch moves the CSV handling entirely to the client. This 
means the web UI (and other XMLRPC/JSON clients that don't use CSV) now 
works with values containing commas and backslashes.

The CLI works too, using the Python CSV parser which is only run once 
now. It still has some surprising behavior which my patch 18 will try to 
simplify.

I updated the tests. Now that CSV parsing is outside the server, it 
doesn't fit into the xmlrpc testing framework.
I think we'd benefit from tests that would verify whether particular 
command lines result in correct XML-RPC calls. I'll look into making 
this more testable (and tested).

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0015-03-Only-split-CSV-in-the-client.patch
Type: text/x-patch
Size: 13363 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120305/622eb4f3/attachment.bin>