Antonio, sure I will review the patch. But please make sure that API
backwards compatibly is intact, otherwise the fix won¹t be accepted.
-Alena.
On 3/2/14, 4:31 PM, "Antonio Fornié Casarrubios"
<antonio.for...@gmail.com> wrote:
>Hi Alena,
>
>The reasons for this strange format? I don't know. There doesn't seem to
>be
>one. After asking on my team and in the dev list I thought perhaps you
>could know. It seems we all see it strange and nobody knows why. But of
>course, if it is for reasons I will stop the change.
>
>
>
>And about the DB, you are right, in the DB is not like I said. But you can
>have this in a table row field:
>{0={value=Toronto,key=City}}
>for some tables. I think there are two cases:
>
>1- params in wich the get method fixes the params on the fly. In these of
>course the strange format is not propagated anymore. But this is still
>wrong: the format itself before the get is invoked, the time spent on
>fixing something that should be a normal Map from the begining (each time
>the get method is invoked) and mainly the fact that these get methods that
>fix the map on the fly are copies of each other: instead of fixing the
>structure in one method, the are plenty of methods almost identical
>copying
>and pasting the same lines. Some times the same method twice in the same
>cmd class for two Map params (look CreateNetworkOfferingCmd
>#getServiceCapabilities and #getServiceProviders).
>
>2- params in which the get method returns the map as it is. With the
>strange format. For example,
>Cloudmonkey command
>create networkoffering ... tags[0].key="City" tags[0].value="Toronto"
>
>You store in the table network_offeringstags, field tags, the String:
>{0={value=Toronto,key=City}}
>(including brackets and all)
>
>So knowing all this I guess you agree this should be refactored... unless
>at some point the strange format is needed. But after looking for it
>everywhere I didn't find any place where it was. I already did the change
>and tested most of the cases and it all seems to work.
>
>
>It would be great if once I upload the patch somebody could help me double
>check that it doesn't brake anything, not only reviewing to code. I did
>plenty of tests of many kinds, but I cannot be sure that I am covering
>enough. Further, there seem to be several places where the code expects
>the
>strange format.
>->ConfigurationManagerImpl line 1545
>
>
>Thanks. Cheers
>Antonio
>
>
>2014-02-28 18:44 GMT+01:00 Alena Prokharchyk
><alena.prokharc...@citrix.com>:
>
>>
>>
>> From: Antonio Fornié Casarrubios <antonio.for...@gmail.com>
>> Date: Friday, February 28, 2014 at 2:09 AM
>> To: Rohit Yadav <rohityada...@gmail.com>, cloudstack <
>> dev@cloudstack.apache.org>, Alena Prokharchyk <
>> alena.prokharc...@citrix.com>
>> Subject: Re: [PROPOSAL][QUESTION] Map parameters in API Commands
>>
>> Hi Alena,
>>
>> I would like to know your opinion on this change. Mainly consists on:
>> 1- Change the way we store the Map params after unpackParams in order to
>> have, for each Map param, a Map<String, String> instead of Map<String,
>> Map<String, Object>>.
>>
>>
>> -Antonio, what was the reason for storing the parameter in the old
>> format to begin with? Where there any case where we actually needed a
>>map
>> of map parameters?
>>
>>
>
>>
>> 2- There are many commands that fix this strange format on demand on
>> their getters, so they do the conversion there. Since I already have the
>> final format I replace these getters with just
>> getTags(){ return this.tags;}
>>
>> 3- Persistence of these Map params. This last change is more tricky and
>> error-prone but the previous two would brake the functionality without
>>it.
>> Actually it doesn't seem that I should change this for all the cases,
>>given
>> that for some commands the current behavior is storing in the DB the
>>Map as
>> it comes, so after the change it will just do the same and thus
>>retrieve it
>> with the right format. So, although in the tables we move from
>> ------
>> key | City
>> ------
>> value | The Hague
>> ------
>>
>> to
>> ------
>> City | The Hague
>> ------
>>
>> then in memory, after DB read, we will just have the proper format
>>again
>> (Map<String, String>). Is that right?
>>
>>
>>
>> - in what table do you see key name being a field name? I've looked
>>at
>> various *_details tables, as well as resource_tag table, everywhere
>>we have
>> key/value fields where we store key and the value respectfully:
>>
>> mysql> desc user_Vm_details;
>>
>>+---------+---------------------+------+-----+---------+----------------+
>> | Field | Type | Null | Key | Default | Extra
>>|
>>
>>+---------+---------------------+------+-----+---------+----------------+
>> | id | bigint(20) unsigned | NO | PRI | NULL | auto_increment
>>|
>> | vm_id | bigint(20) unsigned | NO | MUL | NULL |
>>|
>> | name | varchar(255) | NO | | NULL |
>>|
>> | value | varchar(1024) | NO | | NULL |
>>|
>> | display | tinyint(1) | NO | | 1 |
>>|
>>
>>+---------+---------------------+------+-----+---------+----------------+
>> 5 rows in set (0.01 sec)
>>
>> mysql> desc resource_tags;
>>
>>
>>+---------------+---------------------+------+-----+---------+-----------
>>-----+
>> | Field | Type | Null | Key | Default | Extra
>> |
>>
>>
>>+---------------+---------------------+------+-----+---------+-----------
>>-----+
>> | id | bigint(20) unsigned | NO | PRI | NULL |
>> auto_increment |
>> | uuid | varchar(40) | YES | UNI | NULL |
>> |
>> | key | varchar(255) | YES | | NULL |
>> |
>> | value | varchar(255) | YES | | NULL |
>> |
>> | resource_id | bigint(20) unsigned | NO | MUL | NULL |
>> |
>> | resource_uuid | varchar(40) | YES | | NULL |
>> |
>> | resource_type | varchar(255) | YES | | NULL |
>> |
>> | customer | varchar(255) | YES | | NULL |
>> |
>> | domain_id | bigint(20) unsigned | NO | MUL | NULL |
>> |
>> | account_id | bigint(20) unsigned | NO | MUL | NULL |
>> |
>>
>>
>>+---------------+---------------------+------+-----+---------+-----------
>>-----+
>>
>>
>> 4- The last change should be related to any code expecting the old
>> format, that will fail with the new one. I guess UI will be an example
>>of
>> that, but I didn't check yet. If the JS code receives the new Map
>> serialized, then chances are this will break it, right? Can you tell
>>your
>> thoughts on this? Can you tell me places I should check first to confirm
>> this guess?
>>
>>
>> - Its not just about the uI> You should never break the API backwards
>> compatibility. Remember that lots of third party vendors use our APIs,
>>not
>> the UI. As long as we support the old format, introducing the new one
>> shouldn't be a problem.
>>
>>
>>
>> Considering it all, do you think this change is worth it? For me this
>> seems to be something that was wrong from the beginning and it should
>>have
>> been changed before the mess got spread. But know, although I want to
>>fix
>> it, I'm afraid this involves touching too many things in order to fix
>> something that looks horrible but seems to be actually working and I
>>don't
>> want to break.
>>
>> Thanks. Cheers
>> Antonio
>>
>>
>>
>> 2014-02-12 23:32 GMT+01:00 Rohit Yadav <rohityada...@gmail.com>:
>>
>>> On Wed, Feb 12, 2014 at 9:52 PM, Antonio Fornié Casarrubios
>>> <antonio.for...@gmail.com> wrote:
>>> > Hi Rohit,
>>> >
>>> > I didn't mean changing the format of the HTTP request, but only
>>> changing the
>>> > intermediate format in which we keep it in the property of the
>>>Command
>>> > class. I mentioned the format in the request just to explain what I
>>> meant.
>>> >
>>> > My proposal is to leave the request format as it is, but then when
>>>the
>>> > method "apiDispatcher#setFieldValue" parses the map and assign it to
>>>the
>>> > property, do it in a normal way: which is a Map<String, String>
>>>instead
>>> of a
>>> > Map<String, Map<String, Object>> as it is now. And then, our getter
>>> methods
>>> > (like CreateTagsCommand#GetTag) will be just a normal getter that
>>> doesn't
>>> > need to transform the structure on the fly.
>>>
>>> Cool, let's request the present API layer maintainer(s) and other
>>> folks in the community to comment.
>>>
>>> Regards.
>>>
>>> >
>>> > Thanks, cheers
>>> > antonio
>>> >
>>> >
>>> > 2014-02-11 17:38 GMT+01:00 Rohit Yadav <rohityada...@gmail.com>:
>>> >
>>> >> Hi Antonio,
>>> >>
>>> >> On Tue, Feb 11, 2014 at 9:57 PM, Antonio Fornié Casarrubios
>>> >> <antonio.for...@gmail.com> wrote:
>>> >> > Hi all,
>>> >> >
>>> >> > When invoking a CS API command that has parameters of type Map,
>>>the
>>> >> > request
>>> >> > will be something like this:
>>> >> >
>>> >> >
>>>
>>>URL/api?command=createTags&tags[0].key=region&tags[0].value=canada&tags[
>>>1].key=name&tags[1].value=bob
>>> >> >
>>> >> > in order to send a Map with the pairs:
>>> >> >
>>> >> > tags{
>>> >> > region : "canada",
>>> >> > name : "bob"
>>> >> > }
>>> >> >
>>> >> > Then in the server side the parameters go through several stages
>>> (IMHO
>>> >> > too
>>> >> > many), and have different formats. At some point
>>> >> >
>>> >> > apiDispatcher#setFieldValue
>>> >> >
>>> >> > will assign the value to the command property (CreateTagsCmd#tag
>>>in
>>> the
>>> >> > example) in a VERY strange way:
>>> >> >
>>> >> > CreateTagsCmd#tag = {
>>> >> > 0 : {
>>> >> > "key" : "region",
>>> >> > "value" : "canada"
>>> >> > },
>>> >> > 1 : {
>>> >> > "key" : "name",
>>> >> > "value" : "bob"
>>> >> > }
>>> >> > }
>>> >> >
>>> >> > This is true for several Cmd classes. And the funny thing is they
>>> >> > usually
>>> >> > provide a public getter method to get the Map in an already
>>> "normalized"
>>> >> > structure. The problem is we have this method again a again in
>>>each
>>> of
>>> >> > these commands, only with different name depending on what
>>>property
>>> the
>>> >> > get, and the body is almost copy and pasted. so my next
>>>refactoring
>>> >> > would
>>> >> > be to have a generic method only once in BaseCmd so that all
>>> subclasses
>>> >> > can
>>> >> > reuse it for their Map getters. Pretty obvious, but...
>>> >>
>>> >> This is a well know issue and is such a pain, both for the users of
>>> >> the API to create this API and the programmer who have to put hack
>>>at
>>> >> the backend to extract the map.
>>> >>
>>> >> > Is it really necessary to have this strange format? Wouldn't it be
>>> much
>>> >> > better to just store it in a more normal way from the beginning,
>>>and
>>> >> > have
>>> >> > the getters just standard getters? Does it have any use to have
>>>those
>>> >> > Maps
>>> >> > of Maps?
>>> >>
>>> >> Changing the API will break many client so no one attempted it for
>>> >> keeping backward-compatibility I think.
>>> >>
>>> >> The HTTP RFC states that if same keys are sent in param they must be
>>> >> received as an array. For example, /api?q=1&q=2&q=3 should received
>>>q
>>> >> = [1,2,3] which is what we're not doing.
>>> >>
>>> >> We should do that and this way we can capture maps using keys and
>>> >> values in order, so for example,
>>> >> /api?q.key1=value1&q.key2=value2&q.key1=value3&q.key2=value4 should
>>>be
>>> >> received as as array of maps: [{key1: value1, key2: value2},
>>> >> {key3:value3, key4: value4}] etc.
>>> >>
>>> >> I think it does not have to be maps of maps, but since our API is
>>> >> query based these ugly hacks were invented. We should definitely get
>>> >> rid of them, and perhaps work on the RESTful API layer, cloud-engine
>>> >> and other good stuff we were talking about more than a year ago and
>>> >> deprecate the present query API over next few years. Thoughts,
>>>flames?
>>> >>
>>> >> Regards.
>>> >>
>>> >> >
>>> >> > Thanks. Cheers
>>> >> > Antonio Fornie
>>> >> > Schuberg Philis - MCE
>>> >
>>> >
>>>
>>
>>
