Along these lines, the details parameter in deployVirtualMachine seems
broken. If I call "details[0].key=foo,details[0].value=bar", it stores
entries in the database like this:
id | vmid | name | value | display
12 | 25 | value | bar | 1
13 | 25 | key | foo | 1
It seems as though this might be correct per Alena's email, but I
don't understand how this can be reconstructed into foo=bar when
there's no binding between the two rows. Perhaps details are supposed
to be passed differently than the resource tags, because if I do
"details[0].foo=bar&details[1].baz=12", I get:
id | vmid | name | value | display
12 | 25 | foo | bar | 1
13 | 25 | baz | 12 | 1
And indeed there is code utilizing these details already committed
that expects this format, as deployVirtualMachines getDetails() only
seems to pass a correct Map<String, String> with Key, Value if I use
this format.
On Mon, Mar 3, 2014 at 11:48 AM, Antonio Fornié Casarrubios
<antonio.for...@gmail.com> wrote:
> Hi Alena,
>
> Of course, the API will not have any changes. This is not a functional
> change, just some refactoring. The problem is there are many things in CS
> that really need some refactoring otherwise the problem will continue
> growing more and more, but doing the change and above all making sure it
> all works afterwards is not simple.
>
> I will make sure that everything works exactly the same way and that the
> data returned is also the same.
>
> Thanks. Cheers
> Antonio
>
>
> 2014-03-03 18:48 GMT+01:00 Alena Prokharchyk <alena.prokharc...@citrix.com>:
>
>> 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
>> >>> >
>> >>> >
>> >>>
>> >>
>> >>
>>
>>
