I am in the process of addressing Ivan's comment [1] on the roles to master merge. In order to do so I need to be able to do something like this for the destroy action: @product = Product.deletable.find(params[:id]) and something like this for the update action: @product = Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every action. Some [2] may call this before_filter abuse and I agree. To me, loading a model is an abuse of the purpose of a filter and, moreover, often results in overly clever code like this [3].
As I see it, my options to address Ivan's comment are as follows:
Discern the correct permission scope based on a combination of the controller name, action, and whether or not mercury is in retrograde. [4]
Simply set the model to Product.deletable.find(params[:id]) inside of the destroy method and to Product.editable.find(params[:id]) inside of the update method.
Some other rails thing I am unaware of because I am somewhat new to Ruby/Rails.
Some may argue for #1 because it is DRY and saves lines of code. In the case of the v2 products_controller.rb [5] the number of lines is the same. I think it's obvious that my changes are more readable and I don't consider a solution where you have to repeat the action in the filter's only/except DRY. It's certainly not DRY when you have to repeat the action name inside the actual filter.
I propose that we remove our before_filter abuse from our v2 API controllers as we convert them during this roles work. I am willing to go back and fix the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly clever magic. What is your opinion?
> From: "Walden Raines" <walden@redhat.com>
> To: "foreman-dev" <foreman-dev@googlegroups.com>
> Sent: Wednesday, May 14, 2014 4:56:03 PM
> Subject: [foreman-dev] Katello: Authorization and before_filters
>
<snip>
> 1. Discern the correct permission scope based on a combination of the
> controller name, action, and whether or not mercury is in retrograde. [4]
> 2. Simply set the model to Product.deletable.find(params[:id]) inside of
> the destroy method and to Product.editable.find(params[:id]) inside of the
> update method.
I feel approach 2 is the best simply because its more manageable. Also helps in keeping less member variables than necessary. We just need to be careful while converting existing before_filters to this approach. Certainly do able …
···
----- Original Message -----
Some other rails thing I am unaware of because I am somewhat new to
Ruby/Rails.
Some may argue for #1 because it is DRY and saves lines of code. In the case
of the v2 products_controller.rb [5] the number of lines is the same. I
think it’s obvious that my changes are more readable and I don’t consider a
solution where you have to repeat the action in the filter’s only/except
DRY. It’s certainly not DRY when you have to repeat the action name inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API controllers
as we convert them during this roles work. I am willing to go back and fix
the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
> Hello,
>
> I am in the process of addressing Ivan's comment [1] on the roles to master
> merge. In order to do so I need to be able to do something like this for
> the destroy action: @product = Product.deletable.find(params[:id]) and
> something like this for the update action: @product =
> Product.editable.find(params[:id]).
>
> Unfortunately, we are using before_filters to load our models for every
> action. Some [2] may call this before_filter abuse and I agree. To me,
> loading a model is an abuse of the purpose of a filter and, moreover, often
> results in overly clever code like this [3].
>
> As I see it, my options to address Ivan's comment are as follows:
>
> 1. Discern the correct permission scope based on a combination of the
> controller name, action, and whether or not mercury is in retrograde. [4]
> 2. Simply set the model to Product.deletable.find(params[:id]) inside of
> the destroy method and to Product.editable.find(params[:id]) inside of the
> update method. 3. Some other rails thing I am unaware of because I am
> somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same reasons but
I have to admin that it works nicely and DRY-ness has already paid off. In
application controller we define several methods to find out correct permission,
resource etc. This gives us a good base for standard controllers and actions.
When we need to add some custom rules we just extend those methods in
particular controllers.
See [1] for application controller code and [2] for customization in hosts
controller.
[1]
[2]
···
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the same.
I think it’s obvious that my changes are more readable and I don’t consider
a solution where you have to repeat the action in the filter’s only/except
DRY. It’s certainly not DRY when you have to repeat the action name inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API controllers
as we convert them during this roles work. I am willing to go back and fix
the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
> Hello,
>
> my 2 cents below in text
>
>> Hello,
>>
>> I am in the process of addressing Ivan's comment [1] on the roles to master
>> merge. In order to do so I need to be able to do something like this for
>> the destroy action: @product = Product.deletable.find(params[:id]) and
>> something like this for the update action: @product =
>> Product.editable.find(params[:id]).
>>
>> Unfortunately, we are using before_filters to load our models for every
>> action. Some [2] may call this before_filter abuse and I agree. To me,
>> loading a model is an abuse of the purpose of a filter and, moreover, often
>> results in overly clever code like this [3].
>>
>> As I see it, my options to address Ivan's comment are as follows:
>>
>> 1. Discern the correct permission scope based on a combination of the
>> controller name, action, and whether or not mercury is in retrograde. [4]
>> 2. Simply set the model to Product.deletable.find(params[:id]) inside of
>> the destroy method and to Product.editable.find(params[:id]) inside of the
>> update method. 3. Some other rails thing I am unaware of because I am
>> somewhat new to Ruby/Rails.
>
> In foreman we use #1. Originally I was against it because of same reasons but
> I have to admin that it works nicely and DRY-ness has already paid off. In
> application controller we define several methods to find out correct permission,
> resource etc. This gives us a good base for standard controllers and actions.
> When we need to add some custom rules we just extend those methods in
> particular controllers.
>
> See [1] for application controller code and [2] for customization in hosts
> controller.
>
> [1]
> https://github.com/theforeman/foreman/blob/develop/app/controllers/application_controller.rb#L137-L165
> [2]
> https://github.com/theforeman/foreman/blob/develop/app/controllers/hosts_controller.rb#L536-L568
Yeah I am also for DRYness. It's not always only about saving lines,
much more important reason is capturing logic at one place allowing easy
changes. It may be better thought to just use methods instead of
filters. That is explicit, DRY, and non-magical.
def index
@content_views = base_resource_scope
# rest of the action
end
def show
@content_view = base_resource_scope.find(params[:id])
# rest of the action
end
One additional note: we (katello) should seriously consider inheriting
from Foreman's ApplicationController. Foreman has this logic in place
already, we should leverage it. It would also lead to more consistency
between the projects and better sharing, less duplication.
···
On 15.05.14 8:39, Marek Hulan wrote:
> On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the same.
I think it’s obvious that my changes are more readable and I don’t consider
a solution where you have to repeat the action in the filter’s only/except
DRY. It’s certainly not DRY when you have to repeat the action name inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API controllers
as we convert them during this roles work. I am willing to go back and fix
the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
> In foreman we use #1. Originally I was against it because of same reasons but
> I have to admin that it works nicely and DRY-ness has already paid off. In
> application controller we define several methods to find out correct permission,
> resource etc. This gives us a good base for standard controllers and actions.
> When we need to add some custom rules we just extend those methods in
> particular controllers.
I still don't understand how repeating yourself (by mapping the action to the scope) in action_permission() is DRY. To me [2] isn't DRY, it's confusing. Of course the model in update_multiple_disassociate is scoped as editable, just look it up in this giant switch statement.
Moreover, it's not DRY when you are repeating your permission schema in the controller [2] as well as in the permissions definition [3]. To me, this is a case of overly clever code that is masquerading as DRY in an attempt to justify its existence.
Now I would definitely be in favor of some cdoe that would apply the correct scope based on the definition in access_permissions.r. That would truly be DRY, the mappings from action to permission would only exist in one place instead of two.
···
----- Original Message -----
From: "Marek Hulan"
To: foreman-dev@googlegroups.com
Sent: Thursday, May 15, 2014 2:39:39 AM
Subject: Re: [foreman-dev] Katello: Authorization and before_filters
Hello,
my 2 cents below in text
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Hello,
I am in the process of addressing Ivan’s comment [1] on the roles to master
merge. In order to do so I need to be able to do something like this for
the destroy action: @product = Product.deletable.find(params[:id]) and
something like this for the update action: @product =
Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every
action. Some [2] may call this before_filter abuse and I agree. To me,
loading a model is an abuse of the purpose of a filter and, moreover, often
results in overly clever code like this [3].
As I see it, my options to address Ivan’s comment are as follows:
Discern the correct permission scope based on a combination of the
controller name, action, and whether or not mercury is in retrograde. [4]
Simply set the model to Product.deletable.find(params[:id]) inside of
the destroy method and to Product.editable.find(params[:id]) inside of the
update method. 3. Some other rails thing I am unaware of because I am
somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same reasons but
I have to admin that it works nicely and DRY-ness has already paid off. In
application controller we define several methods to find out correct permission,
resource etc. This gives us a good base for standard controllers and actions.
When we need to add some custom rules we just extend those methods in
particular controllers.
See [1] for application controller code and [2] for customization in hosts
controller.
[1]
[2]
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the same.
I think it’s obvious that my changes are more readable and I don’t consider
a solution where you have to repeat the action in the filter’s only/except
DRY. It’s certainly not DRY when you have to repeat the action name inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API controllers
as we convert them during this roles work. I am willing to go back and fix
the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
I think we can give this method a go and that it should largely handle at
least 80% of cases. I am typically one for being as explicit as possible to
make reading the code easier and to reduce the code hunt to figure out what
is happening. For example, in the update controller, seeing:
is very clear what is happening for that action. Does that become somewhat
repetitious? Yes. But so does writing:
base_resource_scope.find(params[:id])
And to me, the former is clearer. And while I largely agree with DRY
arguments, I think the goal should be to DRY while maintaining clarity when
reading and understanding the code.
One thing about the way the permissions are determined. I am less a fan of
the fact that two mappings exist - one in the permission definition and one
in the controller. I think it would be nicer if the permission policy
defined within the security blocks was able to be fetched and used to
determine the permission to use within the body of the controller action
(and overridden when needed in special cases), but for now we can start
with the current approach.
That being said, as with most things, I agree that we should strive towards
consistency and then iterate improvements that we can all live with.
Eric
···
On Thu, May 15, 2014 at 5:21 AM, Petr Chalupa wrote:
On 15.05.14 8:39, Marek Hulan wrote:
Hello,
my 2 cents below in text
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Hello,
I am in the process of addressing Ivan’s comment [1] on the roles to
master
merge. In order to do so I need to be able to do something like this for
the destroy action: @product = Product.deletable.find(params[:id]) and
something like this for the update action: @product =
Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every
action. Some [2] may call this before_filter abuse and I agree. To me,
loading a model is an abuse of the purpose of a filter and, moreover,
often
results in overly clever code like this [3].
As I see it, my options to address Ivan’s comment are as follows:
Discern the correct permission scope based on a combination of the
controller name, action, and whether or not mercury is in retrograde. [4]
Simply set the model to Product.deletable.find(params[:id]) inside of
the destroy method and to Product.editable.find(params[:id]) inside of
the
update method. 3. Some other rails thing I am unaware of because I am
somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same reasons
but
I have to admin that it works nicely and DRY-ness has already paid off. In
application controller we define several methods to find out correct
permission,
resource etc. This gives us a good base for standard controllers and
actions.
When we need to add some custom rules we just extend those methods in
particular controllers.
See [1] for application controller code and [2] for customization in hosts
controller.
Yeah I am also for DRYness. It’s not always only about saving lines, much
more important reason is capturing logic at one place allowing easy
changes. It may be better thought to just use methods instead of filters.
That is explicit, DRY, and non-magical.
def index
@content_views = base_resource_scope
# rest of the action
end
def show
@content_view = base_resource_scope.find(params[:id])
# rest of the action
end
One additional note: we (katello) should seriously consider inheriting
from Foreman’s ApplicationController. Foreman has this logic in place
already, we should leverage it. It would also lead to more consistency
between the projects and better sharing, less duplication.
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the
same.
I think it’s obvious that my changes are more readable and I don’t
consider
a solution where you have to repeat the action in the filter’s
only/except
DRY. It’s certainly not DRY when you have to repeat the action name
inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API
controllers
as we convert them during this roles work. I am willing to go back and
fix
the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
> > In foreman we use #1. Originally I was against it because of same reasons
> > but I have to admin that it works nicely and DRY-ness has already paid
> > off. In application controller we define several methods to find out
> > correct permission, resource etc. This gives us a good base for standard
> > controllers and actions. When we need to add some custom rules we just
> > extend those methods in particular controllers.
> >
> > See [1] for application controller code and [2] for customization in hosts
> > controller.
> > [1]
> > https://github.com/theforeman/foreman/blob/develop/app/controllers/applic
> > ation_controller.rb#L137-L165 [2]
> > https://github.com/theforeman/foreman/blob/develop/app/controllers/hosts_
> > controller.rb#L536-L568
> I still don't understand how repeating yourself (by mapping the action to
> the scope) in action_permission() is DRY. To me [2] isn't DRY, it's
> confusing. Of course the model in update_multiple_disassociate is scoped
> as editable, just look it up in this giant switch statement.
Hosts controller is unfortunately a giant class, I just wanted to demonstrate,
that any custom behavior is possible. The DRY-ness here is in fact that you
don't repeat :view_$resource in every index action (same for every action)
don't repeat @$resource = $resource.authorized($permission) in every action
> Moreover, it's not DRY when you are repeating your permission schema in the
> controller [2] as well as in the permissions definition [3]. To me, this
> is a case of overly clever code that is masquerading as DRY in an attempt
> to justify its existence.
Reusing a map from permissions definition would be great. I'm not entirely sure
that It's always 1:1, but in general we could try to improve that. Still with
possibility to specify custom permission. Still I think that #1 (from the
original post) is more DRY than #2.
···
On Thursday 15 of May 2014 11:44:14 Walden Raines wrote:
–
Marek
Now I would definitely be in favor of some cdoe that would apply the correct
scope based on the definition in access_permissions.r. That would truly be
DRY, the mappings from action to permission would only exist in one place
instead of two.
----- Original Message -----
From: “Marek Hulan” mhulan@redhat.com
To: foreman-dev@googlegroups.com
Sent: Thursday, May 15, 2014 2:39:39 AM
Subject: Re: [foreman-dev] Katello: Authorization and before_filters
Hello,
my 2 cents below in text
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Hello,
I am in the process of addressing Ivan’s comment [1] on the roles to
master
merge. In order to do so I need to be able to do something like this for
the destroy action: @product = Product.deletable.find(params[:id]) and
something like this for the update action: @product =
Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every
action. Some [2] may call this before_filter abuse and I agree. To me,
loading a model is an abuse of the purpose of a filter and, moreover,
often
results in overly clever code like this [3].
As I see it, my options to address Ivan’s comment are as follows:
Discern the correct permission scope based on a combination of the
controller name, action, and whether or not mercury is in retrograde. [4]
2. Simply set the model to Product.deletable.find(params[:id]) inside of
the destroy method and to Product.editable.find(params[:id]) inside of the
update method. 3. Some other rails thing I am unaware of because I am
somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same reasons
but I have to admin that it works nicely and DRY-ness has already paid off.
In application controller we define several methods to find out correct
permission, resource etc. This gives us a good base for standard
controllers and actions. When we need to add some custom rules we just
extend those methods in particular controllers.
See [1] for application controller code and [2] for customization in hosts
controller.
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the same.
I think it’s obvious that my changes are more readable and I don’t
consider
a solution where you have to repeat the action in the filter’s only/except
DRY. It’s certainly not DRY when you have to repeat the action name
inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API
controllers as we convert them during this roles work. I am willing to
go back and fix the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
I'm personally for the #1, just for the fact that it lowers the risk
of forgetting scoping the resource in some action.
– Ivan
···
----- Original Message -----
> On Thursday 15 of May 2014 11:44:14 Walden Raines wrote:
> > > In foreman we use #1. Originally I was against it because of same reasons
> > > but I have to admin that it works nicely and DRY-ness has already paid
> > > off. In application controller we define several methods to find out
> > > correct permission, resource etc. This gives us a good base for standard
> > > controllers and actions. When we need to add some custom rules we just
> > > extend those methods in particular controllers.
> > >
> > > See [1] for application controller code and [2] for customization in
> > > hosts
> > > controller.
> > > [1]
> > > https://github.com/theforeman/foreman/blob/develop/app/controllers/applic
> > > ation_controller.rb#L137-L165 [2]
> > > https://github.com/theforeman/foreman/blob/develop/app/controllers/hosts_
> > > controller.rb#L536-L568
>
> > I still don't understand how repeating yourself (by mapping the action to
> > the scope) in action_permission() is DRY. To me [2] isn't DRY, it's
> > confusing. Of course the model in update_multiple_disassociate is scoped
> > as editable, just look it up in this giant switch statement.
> Hosts controller is unfortunately a giant class, I just wanted to
> demonstrate,
> that any custom behavior is possible. The DRY-ness here is in fact that you
> 1) don't repeat :view_$resource in every index action (same for every action)
> 2) don't repeat @$resource = $resource.authorized($permission) in every
> action
>
> > Moreover, it's not DRY when you are repeating your permission schema in the
> > controller [2] as well as in the permissions definition [3]. To me, this
> > is a case of overly clever code that is masquerading as DRY in an attempt
> > to justify its existence.
> Reusing a map from permissions definition would be great. I'm not entirely
> sure
> that It's always 1:1, but in general we could try to improve that. Still with
> possibility to specify custom permission. Still I think that #1 (from the
> original post) is more DRY than #2.
>
> --
> Marek
>
> > Now I would definitely be in favor of some cdoe that would apply the
> > correct
> > scope based on the definition in access_permissions.r. That would truly be
> > DRY, the mappings from action to permission would only exist in one place
> > instead of two.
> >
> > Cheers,
> > Walden
> >
> > [3]
> > https://github.com/theforeman/foreman/blob/develop/app/services/foreman/acc
> > ess_permissions.rb#L294-350
> >
> >
> > ----- Original Message -----
> > From: "Marek Hulan"
> > To: foreman-dev@googlegroups.com
> > Sent: Thursday, May 15, 2014 2:39:39 AM
> > Subject: Re: [foreman-dev] Katello: Authorization and before_filters
> >
> > Hello,
> >
> > my 2 cents below in text
> >
> > On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
> > > Hello,
> > >
> > > I am in the process of addressing Ivan's comment [1] on the roles to
> > > master
> > > merge. In order to do so I need to be able to do something like this for
> > > the destroy action: @product = Product.deletable.find(params[:id]) and
> > > something like this for the update action: @product =
> > > Product.editable.find(params[:id]).
> > >
> > > Unfortunately, we are using before_filters to load our models for every
> > > action. Some [2] may call this before_filter abuse and I agree. To me,
> > > loading a model is an abuse of the purpose of a filter and, moreover,
> > > often
> > > results in overly clever code like this [3].
> > >
> > > As I see it, my options to address Ivan's comment are as follows:
> > > 1. Discern the correct permission scope based on a combination of the
> > >
> > > controller name, action, and whether or not mercury is in retrograde. [4]
> > > 2. Simply set the model to Product.deletable.find(params[:id]) inside of
> > > the destroy method and to Product.editable.find(params[:id]) inside of
> > > the
> > > update method. 3. Some other rails thing I am unaware of because I am
> > > somewhat new to Ruby/Rails.
> >
> > In foreman we use #1. Originally I was against it because of same reasons
> > but I have to admin that it works nicely and DRY-ness has already paid off.
> > In application controller we define several methods to find out correct
> > permission, resource etc. This gives us a good base for standard
> > controllers and actions. When we need to add some custom rules we just
> > extend those methods in particular controllers.
> >
> > See [1] for application controller code and [2] for customization in hosts
> > controller.
> >
> > [1]
> > https://github.com/theforeman/foreman/blob/develop/app/controllers/applicati
> > on_controller.rb#L137-L165 [2]
> > https://github.com/theforeman/foreman/blob/develop/app/controllers/hosts_con
> > troller.rb#L536-L568
> > > Some may argue for #1 because it is DRY and saves lines of code. In the
> > > case of the v2 products_controller.rb [5] the number of lines is the
> > > same.
> > > I think it's obvious that my changes are more readable and I don't
> > > consider
> > > a solution where you have to repeat the action in the filter's
> > > only/except
> > > DRY. It's certainly not DRY when you have to repeat the action name
> > > inside
> > > the actual filter.
> > >
> > > I propose that we remove our before_filter abuse from our v2 API
> > > controllers as we convert them during this roles work. I am willing to
> > > go back and fix the controllers that have already been converted.
> > >
> > > We are currently in a position to decide between code clarity and overly
> > > clever magic. What is your opinion?
> > >
> > > Cheers,
> > > Walden
> > >
> > > [1] https://github.com/Katello/katello/pull/4073/files#r12623774
> > > [2]
> > > http://blog.thefrontiergroup.com.au/2014/02/before_action_an_anti_pattern/
> > > [3]
> > > https://github.com/Katello/katello/blob/roles/app/controllers/katello/api/
> > > v
> > > 2/content_views_controller.rb#L193 [4] http://i.imgur.com/a6kOYXD.jpg
> > > [5]
> > > https://github.com/waldenraines/katello/compare/Katello:roles...5720?expan
> > > d
> > > =1#diff-cedcc866cbdbe325575a0c9eee8a1fe3L14
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
> I think we can give this method a go and that it should largely handle at
> least 80% of cases. I am typically one for being as explicit as possible to
> make reading the code easier and to reduce the code hunt to figure out what
> is happening. For example, in the update controller, seeing:
>
> product =
> Product.editable.in_organization(params[:organization_id]).find(params[:id])
>
> is very clear what is happening for that action. Does that become somewhat
> repetitious? Yes. But so does writing:
>
> base_resource_scope.find(params[:id])
Well the context would be rather
def edit
base_resource_scope.find(params[:id])
not sure about in_organization in your example, put that into some scope
method would make sense only if most of resources were belonging to some
organization (which in katello actually makes sense, in foreman maybe will to)
> And to me, the former is clearer. And while I largely agree with DRY
> arguments, I think the goal should be to DRY while maintaining clarity when
> reading and understanding the code.
>
> One thing about the way the permissions are determined. I am less a fan of
> the fact that two mappings exist - one in the permission definition and one
> in the controller. I think it would be nicer if the permission policy
> defined within the security blocks was able to be fetched and used to
> determine the permission to use within the body of the controller action
> (and overridden when needed in special cases), but for now we can start
> with the current approach.
I completely agree, but that's also the reason why base_resource_scope seems
better to me. You can later change the permission determination to be based on
policy and you don't have to change all more "readable" controllers.
> That being said, as with most things, I agree that we should strive towards
> consistency and then iterate improvements that we can all live with.
I think we're on the same page.
···
On Thursday 15 of May 2014 11:44:20 Eric D Helms wrote:
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Hello,
I am in the process of addressing Ivan’s comment [1] on the roles to
master
merge. In order to do so I need to be able to do something like this
for
the destroy action: @product = Product.deletable.find(params[:id]) and
something like this for the update action: @product =
Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every
action. Some [2] may call this before_filter abuse and I agree. To me,
loading a model is an abuse of the purpose of a filter and, moreover,
often
results in overly clever code like this [3].
As I see it, my options to address Ivan’s comment are as follows:
Discern the correct permission scope based on a combination of the
controller name, action, and whether or not mercury is in retrograde.
[4]
2. Simply set the model to Product.deletable.find(params[:id]) inside of
the destroy method and to Product.editable.find(params[:id]) inside of
the
update method. 3. Some other rails thing I am unaware of because I am
somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same reasons
but
I have to admin that it works nicely and DRY-ness has already paid off.
In
application controller we define several methods to find out correct
permission,
resource etc. This gives us a good base for standard controllers and
actions.
When we need to add some custom rules we just extend those methods in
particular controllers.
See [1] for application controller code and [2] for customization in
hosts
controller.
Yeah I am also for DRYness. It’s not always only about saving lines, much
more important reason is capturing logic at one place allowing easy
changes. It may be better thought to just use methods instead of filters.
That is explicit, DRY, and non-magical.
def index
@content_views = base_resource_scope
# rest of the action
end
def show
@content_view = base_resource_scope.find(params[:id])
# rest of the action
end
One additional note: we (katello) should seriously consider inheriting
from Foreman’s ApplicationController. Foreman has this logic in place
already, we should leverage it. It would also lead to more consistency
between the projects and better sharing, less duplication.
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the
same.
I think it’s obvious that my changes are more readable and I don’t
consider
a solution where you have to repeat the action in the filter’s
only/except
DRY. It’s certainly not DRY when you have to repeat the action name
inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API
controllers
as we convert them during this roles work. I am willing to go back and
fix
the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
> I'm personally for the #1, just for the fact that it lowers the risk
> of forgetting scoping the resource in some action.
+1 It's a good point.
T.
···
On 05/16/2014 09:01 AM, Ivan Necas wrote:
– Ivan
----- Original Message -----
On Thursday 15 of May 2014 11:44:14 Walden Raines wrote:
In foreman we use #1. Originally I was against it because of same reasons
but I have to admin that it works nicely and DRY-ness has already paid
off. In application controller we define several methods to find out
correct permission, resource etc. This gives us a good base for standard
controllers and actions. When we need to add some custom rules we just
extend those methods in particular controllers.
I still don’t understand how repeating yourself (by mapping the action to
the scope) in action_permission() is DRY. To me [2] isn’t DRY, it’s
confusing. Of course the model in update_multiple_disassociate is scoped
as editable, just look it up in this giant switch statement.
Hosts controller is unfortunately a giant class, I just wanted to
demonstrate,
that any custom behavior is possible. The DRY-ness here is in fact that you
don’t repeat :view_$resource in every index action (same for every action)
don’t repeat @$resource = $resource.authorized($permission) in every
action
Moreover, it’s not DRY when you are repeating your permission schema in the
controller [2] as well as in the permissions definition [3]. To me, this
is a case of overly clever code that is masquerading as DRY in an attempt
to justify its existence.
Reusing a map from permissions definition would be great. I’m not entirely
sure
that It’s always 1:1, but in general we could try to improve that. Still with
possibility to specify custom permission. Still I think that #1 (from the
original post) is more DRY than #2.
–
Marek
Now I would definitely be in favor of some cdoe that would apply the
correct
scope based on the definition in access_permissions.r. That would truly be
DRY, the mappings from action to permission would only exist in one place
instead of two.
----- Original Message -----
From: “Marek Hulan” mhulan@redhat.com
To: foreman-dev@googlegroups.com
Sent: Thursday, May 15, 2014 2:39:39 AM
Subject: Re: [foreman-dev] Katello: Authorization and before_filters
Hello,
my 2 cents below in text
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Hello,
I am in the process of addressing Ivan’s comment [1] on the roles to
master
merge. In order to do so I need to be able to do something like this for
the destroy action: @product = Product.deletable.find(params[:id]) and
something like this for the update action: @product =
Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every
action. Some [2] may call this before_filter abuse and I agree. To me,
loading a model is an abuse of the purpose of a filter and, moreover,
often
results in overly clever code like this [3].
As I see it, my options to address Ivan’s comment are as follows:
Discern the correct permission scope based on a combination of the
controller name, action, and whether or not mercury is in retrograde. [4]
2. Simply set the model to Product.deletable.find(params[:id]) inside of
the destroy method and to Product.editable.find(params[:id]) inside of
the
update method. 3. Some other rails thing I am unaware of because I am
somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same reasons
but I have to admin that it works nicely and DRY-ness has already paid off.
In application controller we define several methods to find out correct
permission, resource etc. This gives us a good base for standard
controllers and actions. When we need to add some custom rules we just
extend those methods in particular controllers.
See [1] for application controller code and [2] for customization in hosts
controller.
Some may argue for #1 because it is DRY and saves lines of code. In the
case of the v2 products_controller.rb [5] the number of lines is the
same.
I think it’s obvious that my changes are more readable and I don’t
consider
a solution where you have to repeat the action in the filter’s
only/except
DRY. It’s certainly not DRY when you have to repeat the action name
inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API
controllers as we convert them during this roles work. I am willing to
go back and fix the controllers that have already been converted.
We are currently in a position to decide between code clarity and overly
clever magic. What is your opinion?
I have recently been circling back to this in Katello to handle scoping
properly. I have opened two PRs, one against Foreman (to make things easier
for plugins) and one against Katello (to show an example usage) that I
would like to get some feedback and discussion on before I proceed further.
I would kindly ask that any developers familiar in these areas look at both
and give some thoughts or opinions on the changes promptly so that I may
proceed accordingly.
Thanks,
Eric
···
On Thu, May 22, 2014 at 4:25 AM, Tomas Strachota wrote:
On 05/16/2014 09:01 AM, Ivan Necas wrote:
I’m personally for the #1, just for the fact that it lowers the risk
of forgetting scoping the resource in some action.
+1 It’s a good point.
T.
– Ivan
----- Original Message -----
On Thursday 15 of May 2014 11:44:14 Walden Raines wrote:
In foreman we use #1. Originally I was against it because of same
reasons
but I have to admin that it works nicely and DRY-ness has already paid
off. In application controller we define several methods to find out
correct permission, resource etc. This gives us a good base for
standard
controllers and actions. When we need to add some custom rules we just
extend those methods in particular controllers.
I still don’t understand how repeating yourself (by mapping the action
to
the scope) in action_permission() is DRY. To me [2] isn’t DRY, it’s
confusing. Of course the model in update_multiple_disassociate is
scoped
as editable, just look it up in this giant switch statement.
Hosts controller is unfortunately a giant class, I just wanted to
demonstrate,
that any custom behavior is possible. The DRY-ness here is in fact that
you
don’t repeat :view_$resource in every index action (same for every
action)
don’t repeat @$resource = $resource.authorized($permission) in every
action
Moreover, it’s not DRY when you are repeating your permission schema in
the
controller [2] as well as in the permissions definition [3]. To me,
this
is a case of overly clever code that is masquerading as DRY in an
attempt
to justify its existence.
Reusing a map from permissions definition would be great. I’m not
entirely
sure
that It’s always 1:1, but in general we could try to improve that. Still
with
possibility to specify custom permission. Still I think that #1 (from the
original post) is more DRY than #2.
–
Marek
Now I would definitely be in favor of some cdoe that would apply the
correct
scope based on the definition in access_permissions.r. That would
truly be
DRY, the mappings from action to permission would only exist in one
place
instead of two.
----- Original Message -----
From: “Marek Hulan” mhulan@redhat.com
To: foreman-dev@googlegroups.com
Sent: Thursday, May 15, 2014 2:39:39 AM
Subject: Re: [foreman-dev] Katello: Authorization and before_filters
Hello,
my 2 cents below in text
On Wednesday 14 of May 2014 16:56:03 Walden Raines wrote:
Hello,
I am in the process of addressing Ivan’s comment [1] on the roles to
master
merge. In order to do so I need to be able to do something like this
for
the destroy action: @product = Product.deletable.find(params[:id]) and
something like this for the update action: @product =
Product.editable.find(params[:id]).
Unfortunately, we are using before_filters to load our models for every
action. Some [2] may call this before_filter abuse and I agree. To
me,
loading a model is an abuse of the purpose of a filter and, moreover,
often
results in overly clever code like this [3].
As I see it, my options to address Ivan’s comment are as follows:
Discern the correct permission scope based on a combination of
the
controller name, action, and whether or not mercury is in retrograde.
[4]
2. Simply set the model to Product.deletable.find(params[:id]) inside
of
the destroy method and to Product.editable.find(params[:id]) inside of
the
update method. 3. Some other rails thing I am unaware of because I am
somewhat new to Ruby/Rails.
In foreman we use #1. Originally I was against it because of same
reasons
but I have to admin that it works nicely and DRY-ness has already paid
off.
In application controller we define several methods to find out correct
permission, resource etc. This gives us a good base for standard
controllers and actions. When we need to add some custom rules we just
extend those methods in particular controllers.
See [1] for application controller code and [2] for customization in
hosts
controller.
Some may argue for #1 because it is DRY and saves lines of code. In
the
case of the v2 products_controller.rb [5] the number of lines is the
same.
I think it’s obvious that my changes are more readable and I don’t
consider
a solution where you have to repeat the action in the filter’s
only/except
DRY. It’s certainly not DRY when you have to repeat the action name
inside
the actual filter.
I propose that we remove our before_filter abuse from our v2 API
controllers as we convert them during this roles work. I am willing to
go back and fix the controllers that have already been converted.
We are currently in a position to decide between code clarity and
overly
clever magic. What is your opinion?
> I have recently been circling back to this in Katello to handle scoping
> properly. I have opened two PRs, one against Foreman (to make things easier
> for plugins) and one against Katello (to show an example usage) that I would
> like to get some feedback and discussion on before I proceed further.
> I would kindly ask that any developers familiar in these areas look at both
> and give some thoughts or opinions on the changes promptly so that I may
> proceed accordingly.
Looking through the way we've implemented Dynflow via bulk actions I see this being the typical pattern
bulk_products_controller.rb
def do_xyz
products.each do | prod|
<async task for xyz>
end
end
Wonder if you guys think this is a good idea vs having one BulkDynflowTask which takes the class of the individual dynflow action and list of products and does a concurrent run of each product.
To me the advantage of doing a BulkDynflowTask is that we can track it in one spot instead of multiple individual tasks.
You're right: the plan is to have the bulk action that will track the individual tasks.
I have already some code to support that, just need to polish it a bit. Hopefully opening
a PR soon.
– Ivan
···
----- Original Message -----
>
>
> Looking through the way we've implemented Dynflow via bulk actions I see this
> being the typical pattern
> bulk_products_controller.rb
> def do_xyz
> products.each do | prod|
>
> end
> end
>
> Wonder if you guys think this is a good idea vs having one BulkDynflowTask
> which takes the class of the individual dynflow action and list of products
> and does a concurrent run of each product.
>
> To me the advantage of doing a BulkDynflowTask is that we can track it in one
> spot instead of multiple individual tasks.
>
> Any thoughts?
> Partha
>
> --
> You received this message because you are subscribed to the Google Groups
> "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to foreman-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
