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