[Ovirt-devel] [PATCH server] Revised attempt at refactoring the vm_controller using the service layer.

[Scott Seago]

David Lutterkort wrote:
> On Mon, 2009-04-20 at 20:26 +0000, Scott Seago wrote:
>   
>> This code is based on David Lutterkort's proposal outlined in
>> https://www.redhat.com/archives/ovirt-devel/2009-April/msg00086.html
>>
>> I've included service-layer methods for show, create, update, destroy,
>> vm_action, and cancel_queued_tasks. These are the concrete actions
>> that make sense as part of the API. Most of the remaining
>> vm_controller methods are pure-WUI actions -- for things such as
>> generating the web form for a new VM (the subsequent 'create' action
>> _is_ part of the API).
>>     
>
> I really like that, and I also like that most controller methods are now
> very short, and give you a much better impression what happens to set up
> the view.
>
> The one thing that sticks out is the 'delete' method - that should just
> repeatedly call svc_destroy (wrapping the deletion in a txn is somewhat
> bogus here, the user should be ok with having only part of the list of
> VM's deleted)
>
>   
Yes, I think I inserted a comment around that. I would have done it 
except for one problem -- repeated calls to svc_delete would repeatedly 
re-set @vm and re-check permissions. My thinking was that we should add 
a delete_vms action to the VM pool controller, since we only ever delete 
an array of VMs from a single VM pool. The VM pool is the object that we 
check permission on anyway, so the idea would be that we provide a 
method on the VM pool controller to delete a list of VMs that belong to 
that VM pool. We already have the "multi VM action" functionality 
implemented in the VM pool controller for the same reasons -- these 
multi-VM operations work on a subset of VMs that belong to the VM pool.

So I think we should leave this as-is for this patch but when 
resources_controller is converted to use the service layer we should 
pull the delete_vms action into it. It really doesn't work so well as a 
single action on the VM service since we're acting on multiple VMs.
> I've given this some light testing, and as far as I can tell everything
> works well; so ACK from me. The comments below are minor nits and some
> ideas on further refactoring that should happen separately.
>
>   
>> diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb
>> index fa88773..e5f4d4b 100644
>> --- a/src/app/controllers/application.rb
>> +++ b/src/app/controllers/application.rb
>>     
>
>   
>> +  def authorize_view(msg=nil)
>> +    authorize_action(Privilege::VIEW,msg)
>> +  end
>>     
>
> This is unrelated, but it seems odd that we both have authorize_view
> here and Pool.can_view - I would argue that the Pool.can_XXX methods
> should go away in favor of authorize_XXX and can_XXX methods in the
> AppService module (see below)
>
> I like though how authorize_view simplifies the places where we check
> for view permissions.
>
>   
There are actually 3 different sorts of permission-check 
methods/variables here, each fulfilling a different role:
1) in the ApplicationService module:
    authorized? and authorized! methods:
      These both call set_perms if perm_obj is passed in, and either 
return false or raise a PermissionError if the juser isn't authorized.
2) the @can_XXX instance variables:
      These are used by the view code when a specific permission needs 
to be checked in order to display more advanced functionality (create 
links for users with modify permission, etc)
3) in the WUI Application controller class: authorize_XXX methods
     These are wui-specific actions that call authorized? internally -- 
these methods handle the error page/json/rest response when the 
authorization check fails. They can be called from controller actions 
directly or registered as before_filters. They're never called from 
service-layer methods as they are Rails-specific.

Service layer API methods call the methods in 1) and raise exceptions 
for auth errors.
Rails controller actions that don't make service-layer calls (flexigrid 
displays, actions that serve html forms, etc) call the methods in 3) 
which handles error pages automatically.
Rails controller actions that _do_ make service layer calls wrap the 
svc_xxx calls in a rescue block and handle the error pages in the rescue
>> diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb
>> index f5f3c9d..93c81ee 100644
>> --- a/src/app/controllers/smart_pools_controller.rb
>> +++ b/src/app/controllers/smart_pools_controller.rb
>> @@ -199,19 +199,19 @@ class SmartPoolsController < PoolController
>>    def pre_new
>>      @pool = SmartPool.new
>>      @parent = DirectoryPool.get_or_create_user_root(get_login_user)
>> -    @perm_obj = @parent
>> +    set_perms(@parent)
>>      @current_pool_id=@parent.id
>>    end
>>    def pre_create
>>      @pool = SmartPool.new(params[:smart_pool])
>>      @parent = DirectoryPool.get_or_create_user_root(get_login_user)
>> -    @perm_obj = @parent
>> +    set_perms(@parent)
>>      @current_pool_id=@parent.id
>>    end
>>    def pre_edit
>>      @pool = SmartPool.find(params[:id])
>>      @parent = @pool.parent
>> -    @perm_obj = @pool
>> +    set_perms(@pool)
>>      @current_pool_id=@pool.id
>>     
>
> This code is all over the place, i.e.
>         @current_pool_id = some_pool.id
>         set_perms(some_pool)
>
> Why not pull setting @current_pool_id into set_perms ?
>
>   
I was hoping to do that too -- the main problem I ran into was it wasn't 
_always_ the same ID. But perhaps I could make an optional param for 
when it differs. Yes, I think I can do that. The most obvious place it 
differs is for new pool actions -- the current (shown in nav) pool is 
different from the new one being created. But since that's more the 
exception I can probably remove 90% of that problem.
>> diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
>> index c62595a..4c7925c 100644
>> --- a/src/app/models/vm.rb
>> +++ b/src/app/models/vm.rb
>> @@ -220,6 +220,7 @@ class Vm < ActiveRecord::Base
>>        self[:boot_device]= BOOT_DEV_HD
>>        self[:provisioning]= nil
>>      else
>> +      # FIXME: Should this case trigger an error instead?
>>        self[:boot_device]= BOOT_DEV_NETWORK
>>        self[:provisioning]= settings
>>      end
>>     
>
> Should probably be a separate patch
>
>   
Actually I'll probably just remove it. Initially it looked like that 
would be an issue for the refactoring of the cobbler bits, but once I 
discovered that I could do everything in the provisioning method with 
already-existing Vm methods rather than the somewhat clumsy actions on 
the hash this particular bit above isn't really relevant to what I'm 
doing here.
>> diff --git a/src/app/services/application_service.rb b/src/app/services/application_service.rb
>> new file mode 100644
>> index 0000000..e2d0d38
>> --- /dev/null
>> +++ b/src/app/services/application_service.rb
>>     
>
>   
>> +module ApplicationService
>> +  class PermissionError < RuntimeError; end
>> +  class ActionError < RuntimeError; end
>> +
>> +  # Including class must provide a GET_LOGIN_USER
>> +
>> +  def set_perms(perm_obj)
>> +    @perm_obj = perm_obj
>> +    @user = get_login_user
>> +    @can_view = @perm_obj.can_view(@user)
>> +    @can_control_vms = @perm_obj.can_control_vms(@user)
>> +    @can_modify = @perm_obj.can_modify(@user)
>> +    @can_view_perms = @perm_obj.can_view_perms(@user)
>> +    @can_set_perms = @perm_obj.can_set_perms(@user)
>> +  end
>>     
>
> As we talked about on IRC, longer term it would be better if the
> @can_XXX attributes became methods on the service.
>
> I also think the can_XXX methods on pool should be removed and we should
> have something like the following in AppService:
>
>         def user
>           @user ||= get_login_user
>         end
>         
>         def has_privilege(priv, perm_obj = nil)
>           perm_obj ||= @perm_obj
>           perm_obj.has_privilege(user, priv)
>         end
>           
>         def can_view(perm_obj = nil)
>           has_privilege(Privilege::VIEW, perm_obj)
>         end
>
>   
How will this work from the Views? Right now the only place we access 
@can_XXX is in the views, but generally views only have access to 
controller instance variables, not methods..
But certianly the can_XXX methods on pool could go away.
>> diff --git a/src/app/views/vm/_form.rhtml b/src/app/views/vm/_form.rhtml
>> index 58a3d61..610f2bc 100644
>> --- a/src/app/views/vm/_form.rhtml
>> +++ b/src/app/views/vm/_form.rhtml
>> @@ -3,8 +3,7 @@
>>  <%  start_resources = @vm.vm_resource_pool.available_resources_for_vm(@vm) %>
>>  
>>  <!--[form:vm]-->
>> -<%= hidden_field 'vm', 'vm_resource_pool_id' unless @vm_resource_pool_name %>
>> -<%= hidden_field_tag 'vm_resource_pool_name', @vm_resource_pool_name if @vm_resource_pool_name %>
>> +<%= hidden_field 'vm', 'vm_resource_pool_id' %>
>>  <%= hidden_field_tag 'hardware_pool_id', @hardware_pool.id if @hardware_pool %>
>>  
>>      <%= text_field_with_label "Name:", "vm", "description", {:style=>"width:250px;"}  %>
>>     
>
> Could this go into a separate commit, too ?
>
> David
>
>   
Well it could --  but it would require a fair amount of disentangling. 
Basically as I was refactoring the controller methods I realized that 
the 'new VM' controller action was needlessly complicated by a remnant 
of the original UI design from over a year ago. At one point we allowed 
'new VM' links from the HW pool and did some magic around automatically 
finding/creating a matching VM pool. The UI hooks for this have long 
been removed, so I pulled out the unnecessary parts of the controller 
action in the new service layer location. To serparate the commits I'd 
basically have to go to the original codebase, refactor the 'new VM' 
action and then refactor into the service layer again -- I'd rather use 
that time for getting the next controller in place.

Scott