cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alena Prokharchyk <Alena.Prokharc...@citrix.com>
Subject Re: [PROPOSAL][QUESTION] Map parameters in API Commands
Date Fri, 28 Feb 2014 17:44:22 GMT


From: Antonio Fornié Casarrubios <antonio.fornie@gmail.com<mailto:antonio.fornie@gmail.com>>
Date: Friday, February 28, 2014 at 2:09 AM
To: Rohit Yadav <rohityadav89@gmail.com<mailto:rohityadav89@gmail.com>>, cloudstack
<dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>, Alena Prokharchyk
<alena.prokharchyk@citrix.com<mailto: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<mailto:rohityadav89@gmail.com>>:
On Wed, Feb 12, 2014 at 9:52 PM, Antonio Fornié Casarrubios
<antonio.fornie@gmail.com<mailto: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<mailto:rohityadav89@gmail.com>>:
>
>> Hi Antonio,
>>
>> On Tue, Feb 11, 2014 at 9:57 PM, Antonio Fornié Casarrubios
>> <antonio.fornie@gmail.com<mailto: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
>
>


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