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@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³
From a4019f8c23ac2942cf30be956a2819808b5fb553 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Thu, 23 Feb 2012 07:29:47 -0500
Subject: [PATCH 15/17] Only split CSV in the client
Neither the Web UI nor other XML-RPC or JSON clients other than the CLI
need CSV handling (which includes escaping values properly, creating an
additional burden for frontend writers -- and worse: if this is not
done, common cases will work).
Only parse CSV in the CLI client, where it's needed.
Also, document Param's csv arguments, and update tests.
https://fedorahosted.org/freeipa/ticket/2227
https://fedorahosted.org/freeipa/ticket/2417
---
ipalib/cli.py | 1 +
ipalib/frontend.py | 20 ++++++++++
ipalib/parameters.py | 50 ++++++++++++++++++++-----
tests/test_ipalib/test_parameters.py | 24 ++++++------
tests/test_xmlrpc/test_delegation_plugin.py | 6 ++--
tests/test_xmlrpc/test_privilege_plugin.py | 2 +-
tests/test_xmlrpc/test_role_plugin.py | 9 ++---
tests/test_xmlrpc/test_selfservice_plugin.py | 4 +-
8 files changed, 82 insertions(+), 34 deletions(-)
diff --git a/ipalib/cli.py b/ipalib/cli.py
index 737ae001573af0f614783fe69add5711362da21e..8c3199094af9e94f3c258d0eb3873abec2f234ab 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -1028,6 +1028,7 @@ class cli(backend.Executioner):
kw['version'] = API_VERSION
if self.env.interactive:
self.prompt_interactively(cmd, kw)
+ kw = cmd.split_csv(**kw)
self.load_files(cmd, kw)
try:
result = self.execute(name, **kw)
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 028e17e7945c5b193ccbfd84a2f2214e56d2a5fa..a3133808d47cb16c048d3059721ac7a909b3cec0 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -557,6 +557,26 @@ class Command(HasParam):
if name in params:
yield(name, params[name])
+ def split_csv(self, **kw):
+ """
+ Return a dictionary of values where values are decoded from CSV.
+
+ For example:
+
+ >>> class my_command(Command):
+ ... takes_options = (
+ ... Param('flags', multivalue=True, csv=True),
+ ... )
+ ...
+ >>> c = my_command()
+ >>> c.finalize()
+ >>> c.split_csv(flags=u'public,replicated')
+ {'flags': (u'public', u'replicated')}
+ """
+ return dict(
+ (k, self.params[k].split_csv(v)) for (k, v) in kw.iteritems()
+ )
+
def normalize(self, **kw):
"""
Return a dictionary of normalized values.
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index b1525b4d59e8c95125f424be94917bcb2bd9285e..741ab3a8c45a7b9d35e517dc5a1b771e802c219b 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -345,11 +345,16 @@ class Param(ReadOnly):
is not `required`. Applied for all crud.Update based commands
* req_update: The parameter is `required` in all crud.Update based
commands
- - hint: This attribute is currently not used
+ - hint: this attribute is currently not used
- alwaysask: when enabled, CLI asks for parameter value even when the
parameter is not `required`
- sortorder: used to sort a list of parameters for Command. See
`Command.finalize()` for further information
+ - csv: this multivalue attribute is given in CSV format
+ - csv_separator: character that separates values in CSV (comma by
+ default)
+ - csv_skipspace: if true, leading whitespace will be ignored in
+ individual CSV values
"""
# This is a dummy type so that most of the functionality of Param can be
@@ -692,6 +697,40 @@ class Param(ReadOnly):
# decode UTF-8 back to Unicode, cell by cell:
yield [unicode(cell, 'utf-8') for cell in row]
+ def split_csv(self, value):
+ """Split CSV strings into individual values.
+
+ For CSV params, ``value`` is a tuple of strings. Each of these is split
+ on commas, and the results are concatenated into one tuple.
+
+ For example::
+
+ >>> param = Param('telephones', multivalue=True, csv=True)
+ >>> param.split_csv((u'1, 2', u'3', u'4, 5, 6'))
+ (u'1', u'2', u'3', u'4', u'5', u'6')
+
+ If ``value`` is not a tuple (or list), it is only split::
+
+ >>> param = Param('telephones', multivalue=True, csv=True)
+ >>> param.split_csv(u'1, 2, 3')
+ (u'1', u'2', u'3')
+
+ For non-CSV params, return the value unchanged.
+ """
+ if self.csv:
+ if type(value) not in (tuple, list):
+ value = (value,)
+ newval = ()
+ for v in value:
+ if isinstance(v, basestring):
+ csvreader = self.__unicode_csv_reader([unicode(v)])
+ newval += tuple(csvreader.next()) #pylint: disable=E1101
+ else:
+ newval += (v,)
+ return newval
+ else:
+ return value
+
def normalize(self, value):
"""
Normalize ``value`` using normalizer callback.
@@ -718,15 +757,6 @@ class Param(ReadOnly):
if self.multivalue:
if type(value) not in (tuple, list):
value = (value,)
- if self.csv:
- newval = ()
- for v in value:
- if isinstance(v, basestring):
- csvreader = self.__unicode_csv_reader([unicode(v)])
- newval += tuple(csvreader.next()) #pylint: disable=E1101
- else:
- newval += (v,)
- value = newval
if self.multivalue:
return tuple(
self._normalize_scalar(v) for v in value
diff --git a/tests/test_ipalib/test_parameters.py b/tests/test_ipalib/test_parameters.py
index ad8d8404496f1631cfdfc2db4e6f849ecdd09afe..83c33ddfce073d3aa90691b62cab30f30ffcee1e 100644
--- a/tests/test_ipalib/test_parameters.py
+++ b/tests/test_ipalib/test_parameters.py
@@ -635,44 +635,44 @@ class test_Param(ClassChecker):
assert o._convert_scalar.value is default
assert o.normalizer.value is default
- def test_csv_normalize(self):
+ def test_split_csv(self):
"""
- Test the `ipalib.parameters.Param.normalize` method with csv.
+ Test the `ipalib.parameters.Param.split_csv` method with csv.
"""
o = self.cls('my_list+', csv=True)
- n = o.normalize('a,b')
+ n = o.split_csv('a,b')
assert type(n) is tuple
assert len(n) is 2
- n = o.normalize('bar, "hi, there",foo')
+ n = o.split_csv('bar, "hi, there",foo')
assert type(n) is tuple
assert len(n) is 3
- def test_csv_normalize_separator(self):
+ def test_split_csv_separator(self):
"""
- Test the `ipalib.parameters.Param.normalize` method with csv and a separator.
+ Test the `ipalib.parameters.Param.split_csv` method with csv and a separator.
"""
o = self.cls('my_list+', csv=True, csv_separator='|')
- n = o.normalize('a')
+ n = o.split_csv('a')
assert type(n) is tuple
assert len(n) is 1
- n = o.normalize('a|b')
+ n = o.split_csv('a|b')
assert type(n) is tuple
assert len(n) is 2
- def test_csv_normalize_skipspace(self):
+ def test_split_csv_skipspace(self):
"""
- Test the `ipalib.parameters.Param.normalize` method with csv without skipping spaces.
+ Test the `ipalib.parameters.Param.split_csv` method with csv without skipping spaces.
"""
o = self.cls('my_list+', csv=True, csv_skipspace=False)
- n = o.normalize('a')
+ n = o.split_csv('a')
assert type(n) is tuple
assert len(n) is 1
- n = o.normalize('a, "b,c", d')
+ n = o.split_csv('a, "b,c", d')
assert type(n) is tuple
# the output w/o skipspace is ['a',' "b','c"',' d']
assert len(n) is 4
diff --git a/tests/test_xmlrpc/test_delegation_plugin.py b/tests/test_xmlrpc/test_delegation_plugin.py
index db5f7186527d2e0c6567dd5a727e878144bd3020..e687571f7966795aab88cc66154442ee5fc437f2 100644
--- a/tests/test_xmlrpc/test_delegation_plugin.py
+++ b/tests/test_xmlrpc/test_delegation_plugin.py
@@ -72,7 +72,7 @@ class test_delegation(Declarative):
desc='Try to create %r for non-existing member group' % delegation1,
command=(
'delegation_add', [delegation1], dict(
- attrs=u'street,c,l,st,postalCode',
+ attrs=[u'street', u'c', u'l', u'st', u'postalCode'],
permissions=u'write',
group=u'editors',
memberof=u'nonexisting',
@@ -87,7 +87,7 @@ class test_delegation(Declarative):
desc='Create %r' % delegation1,
command=(
'delegation_add', [delegation1], dict(
- attrs=u'street,c,l,st,postalCode',
+ attrs=[u'street', u'c', u'l', u'st', u'postalCode'],
permissions=u'write',
group=u'editors',
memberof=u'admins',
@@ -111,7 +111,7 @@ class test_delegation(Declarative):
desc='Try to create duplicate %r' % delegation1,
command=(
'delegation_add', [delegation1], dict(
- attrs=u'street,c,l,st,postalCode',
+ attrs=[u'street', u'c', u'l', u'st', u'postalCode'],
permissions=u'write',
group=u'editors',
memberof=u'admins',
diff --git a/tests/test_xmlrpc/test_privilege_plugin.py b/tests/test_xmlrpc/test_privilege_plugin.py
index 58dbff85bfd824d3d7e45877d99fc10e14871d80..944225585227a677e4915d4ddbada2dedaa5524c 100644
--- a/tests/test_xmlrpc/test_privilege_plugin.py
+++ b/tests/test_xmlrpc/test_privilege_plugin.py
@@ -87,7 +87,7 @@ class test_privilege(Declarative):
command=(
'permission_add', [permission1], dict(
type=u'user',
- permissions=u'add, delete',
+ permissions=[u'add', u'delete'],
)
),
expected=dict(
diff --git a/tests/test_xmlrpc/test_role_plugin.py b/tests/test_xmlrpc/test_role_plugin.py
index f871e2683a993b4bd65223fb0ae1c20af977e047..6f6de32147e0fe969cfc63ba3ae42bffdf64eb8a 100644
--- a/tests/test_xmlrpc/test_role_plugin.py
+++ b/tests/test_xmlrpc/test_role_plugin.py
@@ -47,9 +47,6 @@ privilege1 = u'r,w privilege 1'
privilege1_dn = DN(('cn', privilege1), DN(api.env.container_privilege),
api.env.basedn)
-def escape_comma(value):
- return value.replace(',', '\\,')
-
class test_role(Declarative):
cleanup_commands = [
@@ -184,7 +181,7 @@ class test_role(Declarative):
dict(
desc='Add privilege %r to role %r' % (privilege1, role1),
command=('role_add_privilege', [role1],
- dict(privilege=escape_comma(privilege1))
+ dict(privilege=privilege1)
),
expected=dict(
completed=1,
@@ -465,7 +462,7 @@ class test_role(Declarative):
dict(
desc='Remove privilege %r from role %r' % (privilege1, role1),
command=('role_remove_privilege', [role1],
- dict(privilege=escape_comma(privilege1))
+ dict(privilege=privilege1)
),
expected=dict(
completed=1,
@@ -486,7 +483,7 @@ class test_role(Declarative):
dict(
desc='Remove privilege %r from role %r again' % (privilege1, role1),
command=('role_remove_privilege', [role1],
- dict(privilege=escape_comma(privilege1))
+ dict(privilege=privilege1)
),
expected=dict(
completed=0,
diff --git a/tests/test_xmlrpc/test_selfservice_plugin.py b/tests/test_xmlrpc/test_selfservice_plugin.py
index 2ddff50ea1c0a9c6dae3e15e6bb4af35b225878e..c7d1124fde92d449bb1098fdbabe5e5c3e418b22 100644
--- a/tests/test_xmlrpc/test_selfservice_plugin.py
+++ b/tests/test_xmlrpc/test_selfservice_plugin.py
@@ -74,7 +74,7 @@ class test_selfservice(Declarative):
desc='Create %r' % selfservice1,
command=(
'selfservice_add', [selfservice1], dict(
- attrs=u'street,c,l,st,postalCode',
+ attrs=[u'street', u'c', u'l', u'st', u'postalCode'],
permissions=u'write',
)
),
@@ -95,7 +95,7 @@ class test_selfservice(Declarative):
desc='Try to create duplicate %r' % selfservice1,
command=(
'selfservice_add', [selfservice1], dict(
- attrs=u'street,c,l,st,postalCode',
+ attrs=[u'street', u'c', u'l', u'st', u'postalCode'],
permissions=u'write',
),
),
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
