The implementation of utils.str2dict fails to convert a dict with more than 2 elements

[Jianing Yang]

from neutron.common import utils
print utils.str2dict('inside_addr=10.0.1.2,inside_port=22,outside_addr=172.16.0.1,outside_port=2222,protocol=tcp')

returns
{'inside_addr': '10.0.1.2', 'inside_port': '22,outside_addr=172.16.0.1,outside_port=2222,protocol=tcp'}

expected value should be

{'outside_port': '2222', 'inside_addr': '10.0.1.2', 'protocol': 'tcp', 'inside_port': '22', 'outside_addr': '172.16.0.1'}

The reason is that in the third line of the implementation below, string.split(',', 1) only splits out two key-value pairs.

quote from neutron/common/utils.py:181:

def str2dict(string):
    res_dict = {}
    for keyvalue in string.split(',', 1):
        (key, value) = keyvalue.split('=', 1)
        res_dict[key] = value
    return res_dict

a quick fix might be remove ",1" from string.split. But it turns out that str2dict/dict2str may also fail when input values containing characters like '=' or ','. A better fix might be using json encode/decode to deal with it.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/60195

[Mark McClain (markmcclain)]

Switching to json encoding can break compatibility with the callers. Have you verified that none of the calls to encode/decode can the API responses?

[Jianing Yang (jianingy)]

I am not sure I understand this compatibility problem. Would you give an example about this?

[DennyZhang (denny-6)]

string.split(',', 1) is incorrect indeed.

As described in the review comments:
1. Current patch change utils.str2dict function to take a valid json as input parameter.
2. But the input parameter would be a string like "inside_addr=10.0.1.2,inside_port=22,outside_addr=172.16.0.1,outside_port=2222,protocol=tcp".

I would suggest to fix by: string.split(',', 1) --> string.split(',')

[Jianing Yang (jianingy)]

There is still a potential error if we use string.split(',') as mention in the error description. Please refer to the test case in my commit.

[Jianing Yang (jianingy)]

@mark I see the problem. it seems that currently changing string.splt(',', 1) to string.split(',') as Denny mentioned is a safe way.
I'll look into if other callers other than diff_list_of_dict depends on the result of these functions.

[DennyZhang (denny-6)]

Jianing, below code explain your concern, right:
 "side_addr=10.0.1.2,inside_port=22,23,45".split(',')
----------------------------------------------
def str2dict(string):
    res_dict = {}
    for keyvalue in string.split(','): <--- We may probably don't have value of key pair containing ','
        (key, value) = keyvalue.split('=', 1) <-- If we do have that problem, this line will raise an exception, to make the error apparently.
        res_dict[key] = value
    return res_dict

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/60195
Committed: http://github.com/openstack/neutron/commit/c011b7668f83b25f1b8e026a909243af96f8f85c
Submitter: Jenkins
Branch: master

commit c011b7668f83b25f1b8e026a909243af96f8f85c
Author: Jianing Yang <email address hidden>
Date: Thu Dec 5 17:34:31 2013 +0800

    Fix str2dict and dict2str's incorrect behavior

    Closes-bug: #1258065

    Change-Id: Idf14c077eeeda6f18f534ad8f62cd741d0c0a2eb