Ok, sounds like there needs to be some work done to make these more consistent, perhaps. Can you comment on why rootdisksize was made from a parameter into a part of the details map?
On Tue, Mar 4, 2014 at 3:12 AM, Bharat Kumar <bharat.ku...@citrix.com> wrote: > Hi ALL, > There are many other APIs that use Map like createNetworkOffering, > updateZone, createTemplate, in most of the cases we do not > say how to use maps, one way would be to write this in the description or to > use the same way to access maps of all APIs. > > BTW the way to use details in deploy vm API is > details[0].foo=bar&details[1].baz=12 where foo and baz are keys. > > Also if we want to use the regix protected static final String > MAP_KEY_PATTERN_EXPRESSION = "^([^\\[^\\]]+)\\[(\\d+)\\]\\.key$"; > protected static final > String MAP_VALUE_PATTERN_EXPRESSION = "^[^\\[^\\]]+\\[\\d+\\]\\.value$"; > > wil this work in the following case. I believe service is the key here which > repeats. > http://10.147.59.119:8080/client/api?command=createNetworkOffering&response=json&sessionkey=/kGFJDXFmMQU8JZnnC7QFfj3tV8=&name=bharat&displayText=bharat&guestIpType=Isolated&lbType=publicLb&; > servicecapabilitylist[0].service=SourceNat&servicecapabilitylist[0].capabilitytype=SupportedSourceNatTypes& > servicecapabilitylist[0].capabilityvalue=peraccount& > servicecapabilitylist[1].service=lb&servicecapabilitylist[1].capabilitytype=SupportedLbIsolation& > servicecapabilitylist[1].capabilityvalue=dedicated&availability=Optional&egresspolicy=ALLOW&state=Creating&status=Creating&allocationstate=Creating&supportedServices=Vpn,Dhcp,Dns,Firewall,Lb,UserData,SourceNat,StaticNat,PortForwarding&specifyIpRanges=false&specifyVlan=false&isPersistent=false&conservemode=false&serviceProviderList[0].service=Vpn&serviceProviderList[0].provider=VirtualRouter&serviceProviderList[1].service=Dhcp&serviceProviderList[1].provider=VirtualRouter&serviceProviderList[2].service=Dns&serviceProviderList[2].provider=VirtualRouter&serviceProviderList[3].service=Firewall&serviceProviderList[3].provider=VirtualRouter&serviceProviderList[4].service=Lb&serviceProviderList[4].provider=VirtualRouter&serviceProviderList[5].service=UserData&serviceProviderList[5].provider=VirtualRouter&serviceProviderList[6].service=SourceNat&serviceProviderList[6].provider=JuniperSRX&serviceProviderList[7].service=StaticNat&serviceProviderList[7].provider=JuniperSRX&serviceProviderList[8].service=PortForwarding&serviceProviderList[8].provider=JuniperSRX&egressdefaultpolicy=true&traffictype=GUEST&_=1393925230248 > > > > On 04-Mar-2014, at 2:30 am, Marcus <shadow...@gmail.com> wrote: > > 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 > > > > > > > > >
