Ho Rohit and Min, My apologies, but sometimes I just write the things I see in my head, as I know they would work, but don't give enough details so other people can see the same picture. Thanks for the heads up, I will prepare it.
In terms of what it solves I would say: nothing. And why that? Because in the way it is now everything works... as far as I know. However, how hard it would be to add another ReponseView enum? How many classes would we have to change? How many if cheks? We have now ResponseView.Full and ResponseView.Restricted. I can imagine that if we create another entry the work would be super huge (based on the code I have seen so far). I will make it clear with examples and show that with a proper refactor we can add commands and ResponseView.XX changing only 1 class... and the change is really small. Next step is to move towards what you suggested and send some snippets. Cheers, Wilder -----Original Message----- From: Min Chen [mailto:min.c...@citrix.com] Sent: woensdag 25 juni 2014 7:00 To: dev@cloudstack.apache.org; Prachi Damle Cc: int-toolkit Subject: Re: Why to have API Commands for Admin actions? Hi Wilder, #3 in your list is definitely something we need to do to reuse command code and avoid the mistake of changing one place and forgetting the other one. But I don't quite get how to avoid passing response view parameter in ApiResponseGenerator in the execute() method using your method annotation idea. It will be great if you can use CreateVpcCmd as an example to give some code snippet to illustrate your idea, as well as your proposed flyweight pattern. Thanks -min On 6/25/14 4:27 AM, "Wilder Rodrigues" <wrodrig...@schubergphilis.com> wrote: >Hi there, > >Thanks to Min, Rohit and Prachi for the details about the current >implementation. > >On the if-else thing, that's not a problem to have them. At some point >in time you have to have if-else in the code. The point is just to not >abuse of it. But no worries, because there is no abuse of if-else in >the current IAM implementation. IMO it is just there because we use the >enum in the parameter. We could perhaps also have the enum associated >with a type in a lightweight fly-weight-ish implementation, which would >get rid of it. I will get back to that one in detail... > >I now understand that there might be more command types in the future, >which could actually bring more response views as well. > >Concerning changes on the API, I don't think there would be any. >Thinking a bit more about the approach I had in mind yesterday - which >I did not made clear in the email - we could try something like this: > >1. change the annotation from type to method, so we can annotate the >execute method 2. in the annotation we inform the response view (as it >is already done) 3. move the common code to the base command (e.g. >CreateVpcCmd has the common code and CreateVpcAsAdminCmd has some >specialized code, if needed) 4. The response view won't be passed as >parameter, it will be in the method annotation 5. The methods down the >pipe that need the response view, can extract it from the annotation 6. >have patterns in place that will keep track of ResponseView and >algorithm related to it (i.e. implement flyweight and template patterns >to accomplish this). This approach will kill the "ifs" and also >increase extensibility and maintainability of the code. > >Imho that would make clear and easier to implement commands and would >avoid copy/paste code from a base command into an "as admin" command, >for example. > >What do you guys think? > >We are currently busy with the implementation of redundant VPC feature. >But I think I will keep an I on that one because I would like to >contribute more. :) > >Cheers, >Wilder > >-----Original Message----- >From: Prachi Damle [mailto:prachi.da...@citrix.com] >Sent: woensdag 25 juni 2014 12:43 >To: dev@cloudstack.apache.org >Cc: int-toolkit >Subject: RE: Why to have API Commands for Admin actions? > >To elaborate further, I would like to add that even if this is adding >some if-else code around checking the enum value when generating the >response, it is replacing several other if-else's that were present in >the code earlier that used to check if the user is an Admin/Domain >Admin/regular user against the Db. > >With IAM, we cannot have such if-else conditions around hardcoded roles. >The design should work with custom roles and custom 'response views' >allocated to the user. >This change is a first step in achieving this. > >Prachi > >-----Original Message----- >From: Min Chen [mailto:min.c...@citrix.com] >Sent: Tuesday, June 24, 2014 10:04 AM >To: dev@cloudstack.apache.org >Cc: int-toolkit >Subject: Re: Why to have API Commands for Admin actions? > >Hi Wilder, > > This is a recent change introduced by IAM feature, see FS here >https://cwiki.apache.org/confluence/display/CLOUDSTACK/CloudStack+Ident >ity >+ >and+Access+Management+%28IAM%29+Plugin, particularly see details in >Response View section. The intention of this is to eventually provide >custom response view for different custom IAM roles. As the first step, >we only provided two static response view: Full and Restricted, and >just map Full view to current admin commands. In the future, we should >allow admin to define custom response view through column filter, and >associate each custom response view with IAM policy. Hope that this can >give you some context on this part of code. > > Thanks. > -min > >On 6/24/14 4:55 AM, "Wilder Rodrigues" <wrodrig...@schubergphilis.com> >wrote: > >>Hi all, >> >>We are currently working on the redundant VPC implementation. In order >>to take the right steps from the beginning, we started analyzing the >>existing code base, from the API commands into the >>VPCVirtualNetworkAppliancaManagerImpl. >> >>Although it's not related to the feature itself, we found out that the >>current way of using the APICommand annotation and the CreateVPCCmd >>class (and its derived) is not really clear. For example, there are 2 >>command classes to create a VPC. The difference between them is: one >>has ResponseView.Full parameter in the @APICommand (ie. >>CreateVPCCmdByAdmin); and the other has ResponseView.Rstricted >>parameter in the @APICommand (i.e. CreateVPCCmd). Moreover, the call >>to >>responseGenerator.createVpcresponde() method uses a ResponseView enum >>according to what has been specified in the annotation parameter. >> >>We understand that having a different enum in the >>responseGenerator.createVpcresponde() method will affect many things, >>because it goes deep into the code until reaches the APIDBUtils and >>the database. It is also checked in the ApiServer class, when the >>command is evaluated based on a string passed to the getCmdClass() method. >> >>Since we can identify the user in the account manager, also checking >>the kind of access the user has, what is the point in having the >>Annotation + the Enum in the code? Keep in mind that the latter is >>passed several times as parameters to other methods, which adds many >>"ifs" and unnecessary complexity. >> >>We could also make possible to use the Annotation in the method >>itself, which could avoid having to pass the Enum to the method. That >>means that the Enum would be removed from the annotation and we would >>use the annotation only to identify the API name, response object and >>entity type. In order to check the user's credentials, we would use >>the >>checkAccess() from the account manager, as it is already being used in >>the ApiDispatcher class. >> >>I know those are huge changes, if we actually agree in going for >>anything like this in the future. But the Admin commands are not doing >>much except for change the enum which is passed to the create response >>method. Most of the content of the execute() method is a copy/paste >>from the extended Command class. >> >>Just trying to start some chat towards the subject. >> >>Thanks for your time. >> >>Cheers, >>Wilder >
