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+Identity
>+
>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
>
