Fixes #16646 - Add ability to plugins to modify index scope #3887

ShimShtein · 2016-09-25T18:22:40Z

Added the ability for plugins to modify the scope used to get hosts for #index action. This is needed if a plugin wants to add more includes or eager_load statements, so Active Record won't try to lazy load additional relations and create N+1 queries.

timogoebel · 2016-09-26T06:02:45Z

app/controllers/concerns/scopes_per_action.rb

@@ -0,0 +1,37 @@
+# This module will hold a regstry of scope directives that should be


nitpick: regstry´should beregistry´

ohadlevy · 2016-09-26T06:42:23Z

app/controllers/concerns/scopes_per_action.rb

+  def action_scope_for(action, base_scope)
+    scope = base_scope
+    self.class.scopes_for(action).each do |scope_func|
+      scope = scope_func.call(scope) || scope


instead of looping them all, you can return the first match? or do you rather want to match the last one? the code is a bit hard to read.

I am chaining the scopes, here is an example:
In puppet plugin:

HostsController.add_scope_for(:index) { |base_scope| base_scope.eager_load(:environment) }

In provisioning plugin:

HostsController.add_scope_for(:index) { |base_scope| base_scope.eager_load(:operatingsystem) }

I want to create a scope that will contain both operations:

base_scope = Hosts.authorized actual_scope = base_scope.eager_load(environment).eager_load(operatingsystem)

I achieve this by calling each block, and passing it the result of the previous block.

ohadlevy · 2016-09-26T06:43:08Z

app/services/foreman/plugin.rb

@@ -351,6 +351,12 @@ def parameter_filters(klass)
      @parameter_filters.fetch(klass.is_a?(Class) ? klass.name : klass, [])
    end

+    def hosts_controller_action_scope(action, &block)
+      # Add scope for both API and UI controller.
+      HostsController.add_scope_for(action, block)


does it make sense to always assume you want to extend them both?

It sounds reasonable to me that the info that is returned is similar between the API and UI. In the original bug in Katello I saw that the problem exists in both UI and API.
I won't fight over it though. If you think it's unlikely, I can separate it to different methods.

timogoebel

Looks good to me all in all. I like the DSL and this feature is long past due. The Plugin method needs test coverage (and fixing), though.

timogoebel · 2016-09-27T13:36:44Z

app/services/foreman/plugin.rb

@@ -351,6 +351,12 @@ def parameter_filters(klass)
      @parameter_filters.fetch(klass.is_a?(Class) ? klass.name : klass, [])
    end

+    def hosts_controller_action_scope(action, &block)


Could you please add a test for this? Extra points for inline usage example.

timogoebel · 2016-09-27T13:49:21Z

app/services/foreman/plugin.rb

@@ -351,6 +351,12 @@ def parameter_filters(klass)
      @parameter_filters.fetch(klass.is_a?(Class) ? klass.name : klass, [])
    end

+    def hosts_controller_action_scope(action, &block)
+      # Add scope for both API and UI controller.
+      HostsController.add_scope_for(action, block)


I think, you need to pass the block like so (note the extra ampersand):

HostsController.add_scope_for(action, &block)

Same below.

ShimShtein · 2016-10-06T15:16:37Z

Rebased, fixed plugin registration and added tests.

@timogoebel did I get the extra points?
Is it clear how to use it?

ShimShtein · 2016-10-09T06:12:02Z

Rebased again

timogoebel

The code looks good, thanks for the tests. It is also clear how to use this.
My only concern is, that this won't work in develop mode when classes can be reloaded. My suggestion is to store the scope codeblocks in the plugin model and when the concern is included have it get the scopes from the plugin model. You can check c676093 for an example.

ShimShtein · 2016-10-09T18:02:04Z

My only concern is, that this won't work in develop mode when classes can be reloaded.

In the past we had config.to_prepare block that was run after reload. I can see that in Rails 5, this block has moved to ActiveSupport::Reloader.to_prepare just put those definitions in the to_prepare block and the callback will be registered again after reload.

timogoebel · 2016-10-10T06:53:50Z

@ShimShtein : That would be possible, however it's not the Straight-A student's solution. The code in "to_prepare" runs on every development request. I think, it's better to implement a solution that just runs when a class is actually reloaded.

ShimShtein · 2016-10-13T12:31:24Z

Now the list is global - no reloads should affect it.

lzap · 2016-10-25T09:48:10Z

FYI there are conflicting files.

ShimShtein · 2016-10-27T13:00:19Z

Rebased

timogoebel

@ShimShtein : Thanks for the latest updates. Class reloading still beaks the scopes from Plugins.

Open hosts index page -> everyting is fine.

To reproduce:

touch app/controllers/concerns/scopes_per_action.rb

Open host index page again -> it detects a n+1 query.

timogoebel · 2016-10-27T13:33:06Z

test/unit/plugin_test.rb

@@ -383,6 +383,19 @@ def test_add_smart_proxy_for
    assert_equal({:foo => {:feature => 'Foo'}}, Foreman::Plugin.find(:test_smart_proxy).smart_proxies(Awesome))
  end

+  def test_hosts_controller_action_scope
+    mock_scope = ->(scope) { scope }
+    Foreman::Plugin.register :test_parameter_filter do


should be test_controller_action_scope

ShimShtein · 2016-10-27T14:09:19Z

touch app/controllers/concerns/scopes_per_action.rb

Indeed, reloading of this class will break the scopes. But that class'es sole responsibility is to maintain the registry, so if it changes - the old registry is probably not valid.
If I store this registry in the plugin class and it will get reloaded, it will be lost too.

timogoebel · 2016-10-27T14:13:12Z

@ShimShtein : True. But if you store the data in

the host controller class
the plugin class

When these classes get reloaded, the code is parsed as well and the information is restored.

When you access the data, you can get it from both classes (and merge it) and you shouldn't have a problem. What do you think?

timogoebel · 2016-10-27T14:43:16Z

@domcleal: May @ShimShtein and I ask for your opinion regarding where to store the data so that it survives a class reload in development? Is the current implementation fine, where the data from plugins gets lost when the ScopesPerAction concern is reloaded?

domcleal · 2016-10-27T14:48:28Z

@domcleal: May @ShimShtein and I ask for your opinion regarding where to store the data so that it survives a class reload in development?

It really should be stored in Foreman::Plugin (which is immune from reloads) when the API is called. Either access it from there when called, or something akin to c676093 where it's retrieved when the concern is included.

I'd also suggest not adding an API specifically for hosts, else it'd spawn a new API for every single model/controller collection, it ought to be more generic.

Is the current implementation fine, where the data from plugins gets lost when the ScopesPerAction concern is reloaded?

Not really, IMHO. This is normally treated as a bug, and it ought to be fixed.

ShimShtein · 2016-10-30T10:06:47Z

@domcleal First, thanks for the review. My only question is what to do with scopes that are not part of a particular plugin. For example API::V2::HostsController adds a default scope for index here. Of course I can add this scope manually in the method, but it sounds like I should really use the same framework here too.

ShimShtein · 2016-10-30T14:22:04Z

@domcleal, Sorry, ignore my previous comment, I did not read the code thoroughly enough.

I have change the implementation to store scopes in both the plugin and in the controller (locally). The local storage is there for cases like the API::V2::HostsController.
I didn't cache the result of querying the plugins intentionally. It is done to avoid issues with construction of the controller class before all the plugins are loaded.

ShimShtein · 2016-10-31T15:57:35Z

test failure is not related.

timogoebel · 2016-11-03T13:56:01Z

Merged, thanks @ShimShtein. Please add a usage example to http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin.

ShimShtein · 2016-11-03T14:29:19Z

@timogoebel Done: http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin#Modifying-controllers-query

theforeman-bot added Needs testing Not yet reviewed labels Sep 25, 2016

timogoebel reviewed Sep 26, 2016

View reviewed changes

ShimShtein force-pushed the 16646 branch from 5ec08ce to 0477d77 Compare September 26, 2016 06:25

ohadlevy reviewed Sep 26, 2016

View reviewed changes

domcleal added Needs re-review and removed Not yet reviewed labels Sep 26, 2016

akofink mentioned this pull request Sep 26, 2016

Fixes #16010 - Include referenced objects in hosts Katello/katello#6236

Merged

timogoebel requested changes Sep 27, 2016

View reviewed changes

timogoebel added Waiting on contributor and removed Needs re-review labels Sep 27, 2016

ShimShtein force-pushed the 16646 branch from 0477d77 to a29220d Compare October 6, 2016 15:15

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 6, 2016

ShimShtein force-pushed the 16646 branch 2 times, most recently from 05c53be to 67f70da Compare October 9, 2016 06:11

timogoebel requested changes Oct 9, 2016

View reviewed changes

timogoebel added Waiting on contributor and removed Needs re-review labels Oct 9, 2016

ShimShtein force-pushed the 16646 branch from 67f70da to 5adfefe Compare October 13, 2016 12:30

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 13, 2016

ShimShtein force-pushed the 16646 branch from 5adfefe to b654f2b Compare October 27, 2016 12:59

timogoebel requested changes Oct 27, 2016

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Oct 27, 2016

Fixes #16646 - Add ability to plugins to modify index scope

a4f53d7

ShimShtein force-pushed the 16646 branch from b654f2b to a4f53d7 Compare October 30, 2016 14:14

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 30, 2016

timogoebel added Ready to merge and removed Needs re-review Needs testing labels Nov 3, 2016

timogoebel approved these changes Nov 3, 2016

View reviewed changes

timogoebel merged commit 87f8f03 into theforeman:develop Nov 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #16646 - Add ability to plugins to modify index scope #3887

Fixes #16646 - Add ability to plugins to modify index scope #3887

ShimShtein commented Sep 25, 2016

timogoebel Sep 26, 2016

ShimShtein Sep 26, 2016

ohadlevy Sep 26, 2016

ShimShtein Sep 26, 2016

ohadlevy Sep 26, 2016

ShimShtein Sep 26, 2016

timogoebel left a comment

timogoebel Sep 27, 2016

timogoebel Sep 27, 2016

ShimShtein commented Oct 6, 2016

ShimShtein commented Oct 9, 2016

timogoebel left a comment

ShimShtein commented Oct 9, 2016

timogoebel commented Oct 10, 2016

ShimShtein commented Oct 13, 2016

lzap commented Oct 25, 2016

ShimShtein commented Oct 27, 2016

timogoebel left a comment •

edited

Loading

timogoebel Oct 27, 2016

ShimShtein commented Oct 27, 2016

timogoebel commented Oct 27, 2016

timogoebel commented Oct 27, 2016

domcleal commented Oct 27, 2016

ShimShtein commented Oct 30, 2016

ShimShtein commented Oct 30, 2016

ShimShtein commented Oct 31, 2016

timogoebel commented Nov 3, 2016

ShimShtein commented Nov 3, 2016

		@@ -0,0 +1,37 @@
		# This module will hold a regstry of scope directives that should be

Fixes #16646 - Add ability to plugins to modify index scope #3887

Fixes #16646 - Add ability to plugins to modify index scope #3887

Conversation

ShimShtein commented Sep 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Oct 6, 2016

ShimShtein commented Oct 9, 2016

timogoebel left a comment

Choose a reason for hiding this comment

ShimShtein commented Oct 9, 2016

timogoebel commented Oct 10, 2016

ShimShtein commented Oct 13, 2016

lzap commented Oct 25, 2016

ShimShtein commented Oct 27, 2016

timogoebel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Oct 27, 2016

timogoebel commented Oct 27, 2016

timogoebel commented Oct 27, 2016

domcleal commented Oct 27, 2016

ShimShtein commented Oct 30, 2016

ShimShtein commented Oct 30, 2016

ShimShtein commented Oct 31, 2016

timogoebel commented Nov 3, 2016

ShimShtein commented Nov 3, 2016

timogoebel left a comment •

edited

Loading