cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Antonio Fornié Casarrubios <antonio.for...@gmail.com>
Subject Re: [PROPOSAL][QUESTION] Map parameters in API Commands
Date Mon, 10 Mar 2014 21:23:50 GMT
Hi all,

My initial intention was to get rid of the strange Map<Map<String,
Object>>, because most people found it counter intuitive and unnecessary
(at least nobody knows why yet). But I cannot proceed with the change that
I proposed any more because I cannot assume that all the maps come in the
same format. I cannot make any change that makes CS non backward compatible.

Assuming that all the parameters arrived with this format:
mymap[0].key=country
mymap[0].value=Netherlands
we could have done it. But since we have others formats depending on the
Command
mymap[0].country=Netherlands
my change would brake it.

@Jayapal, don't worry about my change. Given that we don't have a unique
convention I guess nothing stops you from doing it that way. I also prefer
that way. But most of the cases are the other way if I'm not wrong (the
first example). And I will not send any change that brakes that, of course.
Still, it would be needed to get rid of the duplications (copy and paste)
of code that transforms that Map of Maps into a regular Map. Just have it
in a single place and invoke it from any Command that needs it. You all
agree?

@Marcus. You are right, how we send the params in the request and how we
keep them in memory are two separate things. But still, parsing them in a
single place that is reused elsewhere is not so beneficial if we don't have
a single convention. Actually there is a big coupling there (and will still
be), because although there is a single place with the knowledge of how the
params come (method unpackParams), the bizarre structure is spread and has
to be fixed into a normal Map in their getters. All these methods struggle
with the fact that the "key" and "value" are still there in memory. So all
that code is impregnated with this parsing specific details.

Apart from all of that, and keeping the several formats as they are, I
still want to parse the maps with Regular Expressions, instead of manually
parsing the characters "[",  "." and so on... The current code is more
error prone and less readable than using Regex. You all agree?

Thank you all. Cheers
Antonio



2014-03-10 20:01 GMT+01:00 Mike Tutkowski <mike.tutkowski@solidfire.com>:

> Just as an FYI: I recently added support to pass in a Min and Max IOPS for
> the root disk of a VM to create. I first investigated how this was being
> done with the custom root disk and then copied that approach. That being
> the case, I leveraged the map instead of making new parameters. I do agree
> we should define more clearly when one approach should be taken over the
> other.
>
> Thanks!
>
>
> On Mon, Mar 10, 2014 at 12:13 PM, Marcus <shadowsor@gmail.com> wrote:
>
>> I suppose its also worth pointing out that the api params and how we
>> pass them around internally are two separate things. It's easy to just
>> add a new custom api parameter and then put it into the details map
>> (or an object) to be passed around, and it provides a much better user
>> experience from the API consumer's perspective as the parameters are
>> documented as normal and the user doesn't have to deal with parameters
>> inside parameters. Most of the other parameter maps seem to have
>> related info, such as serviceproviderlist, networksids, or the
>> resource tags map.  The new details map for deployVirtualMachine just
>> seems like a place to put random parameters so that we don't have to
>> keep adding them explicitly, especially given that they're not all
>> persisted in the user_vm_details table. It might be nice to consider
>> more targeted maps such as 'serviceofferingdetails' which would
>> contain related parameters that could override service offerings such
>> as cpu, memory, and a separate map for 'diskofferingdetails' that
>> could override iops and other disk attributes, to be reused on
>> createVolume API. rootdisksize doesn't fit, as there's currently no
>> root size on service offering, but we could in the future add a 'min
>> root size' attribute into the service offering that would override the
>> template size if it were larger than the selected template.
>>
>> On Mon, Mar 10, 2014 at 11:38 AM, Marcus <shadowsor@gmail.com> wrote:
>> > Yes, we'd have to leave the current one intact, but allow the other as
>> well.
>> >
>> > On Mon, Mar 10, 2014 at 11:36 AM, Bharat Kumar <bharat.kumar@citrix.com>
>> wrote:
>> >> Hi,
>> >> i don't  know if this is still valid, but there was one more parameter
>> disksize which which specifies the disk size for data disks.
>> >> I would prefer adding both the root disk size and disk size to the
>> details map. ideally we should also change the name disksize to
>> >> dataDisksize to remove confusion but this might break backward
>> compatibility.
>> >>
>> >> Adding them to a map will be intuitive as we already use a map to
>> specify any custom parameters related to VM.
>> >>
>> >> Thanks
>> >> Bharat.
>> >>
>> >> On 10-Mar-2014, at 10:57 pm, Marcus <shadowsor@gmail.com> wrote:
>> >>
>> >>> It is valid, as I've implemented it. So we need to decide if we're
>> >>> using 'details' or rootdisksize as an api param. That's why I'm
>> >>> asking.
>> >>>
>> >>> On Mon, Mar 10, 2014 at 1:43 AM, Bharat Kumar <
>> bharat.kumar@citrix.com> wrote:
>> >>>> Hi All,
>> >>>> the roodiskresize is no longer valid. as there is no code which
is
>> using rootdiskresize currently.
>> >>>>
>> >>>> As a part of the custom service offering we had to enable specifying
>> custom values to parameters cpu, memory and cpuCores.
>> >>>> instead of adding a parameter for each of these values we changed
it
>> use a details map.  This will also not require any further
>> >>>> changes in the API if we need to add some more custom values in
>> future.
>> >>>>
>> >>>> On 08-Mar-2014, at 1:42 am, Marcus <shadowsor@gmail.com> wrote:
>> >>>>
>> >>>>> Any suggestion? Do we go forward assuming that the correct parameter
>> >>>>> for resize on deploy is:
>> >>>>>
>> >>>>> deployVirtualMachine&details[0].rootdisksize=3
>> >>>>>
>> >>>>> or do we change it to
>> >>>>>
>> >>>>> deployVirtualMachine&rootdisksize=3
>> >>>>>
>> >>>>> On Tue, Mar 4, 2014 at 4:14 PM, Marcus <shadowsor@gmail.com>
wrote:
>> >>>>>> 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.kumar@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 <shadowsor@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.fornie@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.Prokharchyk@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.fornie@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.Prokharchyk@citrix.com>:
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> From: Antonio Fornié Casarrubios <antonio.fornie@gmail.com>
>> >>>>>>> Date: Friday, February 28, 2014 at 2:09 AM
>> >>>>>>> To: Rohit Yadav <rohityadav89@gmail.com>, cloudstack
<
>> >>>>>>> dev@cloudstack.apache.org>, Alena Prokharchyk <
>> >>>>>>> alena.prokharchyk@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 <rohityadav89@gmail.com>:
>> >>>>>>>
>> >>>>>>> On Wed, Feb 12, 2014 at 9:52 PM, Antonio Fornié Casarrubios
>> >>>>>>> <antonio.fornie@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 <rohityadav89@gmail.com>:
>> >>>>>>>
>> >>>>>>> Hi Antonio,
>> >>>>>>>
>> >>>>>>> On Tue, Feb 11, 2014 at 9:57 PM, Antonio Fornié Casarrubios
>> >>>>>>> <antonio.fornie@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
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>
>> >>
>>
>
>
>
> --
> *Mike Tutkowski*
> *Senior CloudStack Developer, SolidFire Inc.*
> e: mike.tutkowski@solidfire.com
> o: 303.746.7302
> Advancing the way the world uses the cloud<http://solidfire.com/solution/overview/?video=play>
> *(tm)*
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message