I think that would have been a more correct format from the very beginning
details[0].rootdisksize=2
but right now it is no good, since we should keep them congruent, so it
should be
details[0].key=rootdisksize
details[0].value=2
right? I mean how to pass a map in a request should be always the same.
When the params are parsed from the request in order to set the values into
the fields, nothing is actually checking the tokens "key" and "value",
just the format bla[0].a=b
But something else that is very strange is that, although in all the Map
parameters we put the data in this strange format, then the
getDetails/getBla methods, some of them convert it to a Map<String, String>
(there are actually two types of code for this, copied and pasted along
dozens of methods) while others just return it as it is (so the client'd
better know it is getting that format).
So I am concerned about this, because if I apply this change, then all the
getters will return a Map<String, String> and any client relying on a Map
of Maps will now fail. My change would affect all, because instead of
repeating the code I will just do it once in the unpackParams method.
About the DB, I remembered also the strange format, but after Alena's mail
I couldn't find any example so I assumed I was wrong. But I think we have
at least three ways of storing this:
First:
| key | City |
| value | London |
Second:
| City | London |
And third (ie network_offeringstags.tags)
| {0={value=London,key=City}} |
(in a single table cell)
And BTW, in the example that I found of the First way, it DID read both
rows back in one Map of Maps correctly.
One more thing, can I assume then all the params are coming in the format
details[0].key=rootdisksize
details[0].value=2
Because I want to base the parse in regex (instead of manual parsing like
now) and so far I am using these Patterns
protected static final String MAP_KEY_PATTERN_EXPRESSION =
"^([^\\[^\\]]+)\\[(\\d+)\\]\\.key$";
protected static final String MAP_VALUE_PATTERN_EXPRESSION =
"^[^\\[^\\]]+\\[\\d+\\]\\.value$";
Which is good if we want to verify the "key" and "value" tokens, but it
would fail with the other format you mentioned.
Thanks. Cheers
Antonio
2014-03-03 22:50 GMT+01:00 Marcus <shadow...@gmail.com>:
> FYI I was working on implementing the rootdisksize detail when I ran
> into this. The parameter is detected and works fine if I do
> 'details[0].rootdisksize=2', but this is a special one since its not
> persisted in the db, it's only used when allocating the root volume
> and then stripped off of the details before they're persisted. That
> led me to run further testing and came up with the above.
>
> On Mon, Mar 3, 2014 at 2:06 PM, Alena Prokharchyk
> <alena.prokharc...@citrix.com> wrote:
> > Adding Bharat to the thread as he was the one who introduced the details
> > param to the deployVm call.
> >
> > Bharat, why did you pick this format for storing the vm details? Were you
> > following the rules from some other calls? If so, what those calls are?
> >
> > -Alena.
> >
> > On 3/3/14, 1:00 PM, "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&ta
> >>>>>>>gs[
> >>>> >>>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
> >>>> >>> >
> >>>> >>> >
> >>>> >>>
> >>>> >>
> >>>> >>
> >>>>
> >>>>
> >
>
