Bug #3920: Prevent 500 ScopedSearch errors on the API #1126

dLobatog · 2014-01-03T16:00:24Z

This PR applies a filter on every controller, around the index action.

It's functionality is basically to avoid 500 errors caused by broken
ScopedSearch queries, for instance "wrongfield = dlkas".

Tests just check that the appropriate template is rendered for the API, and in the UI case it checks the flash message and an appropriate redirection.

Without this PR, wrongly formed queries will result on 500 errors from the API and the UI side. Some of the controllers already implemented a fix for this, so this functionality was extracted and applied across all controllers.

Here's a list of the controllers where this is fixed (all of them except 2):

          500: settings - fixed
                  models   - fixed
                  images   - fixed
                  architectures - fixed
                  media - fixed
                  roles - fixed
                  ptables - fixed
                  operatingsystems - fixed
                  environments - fixed
                  domains - fixed
                  common_parameters - fixed
                  subnets - fixed

          200:  compute_resources - fixed
                  reports - fixed 
                  users - fixed
                  lookup_values - fixed
                  hostgroups - fixed
                  config_templates - fixed
                  puppetclasses - fixed
                  lookup_keys - fixed
                  audits - fixed

                  fact_values
                  hosts
                  are hard to fix because of post processing after search. Error handling is done in the controller itself, not on application_controller.


          n/a:   statistics
                  dashboard
                  facts
                  about
                  compute_resources_vms

http://projects.theforeman.org/issues/3920

dmitri-d · 2014-01-15T12:44:26Z

I'm seeing:

Started GET "/users?utf8=%E2%9C%93&search=arg+%3D+ghw" for 127.0.0.1 at 2014-01-15 12:39:27 +0000
Processing by UsersController#index as HTML
Parameters: {"utf8"=>"â��", "search"=>"arg = ghw"}
User Load (0.2ms) SELECT "users".* FROM "users" WHERE "users"."id" = ? LIMIT 1 [["id", 1]]
Setting current user thread-local variable to admin
(0.2ms) SELECT COUNT() FROM "taxonomies" WHERE "taxonomies"."type" IN ('Organization')
Setting current organization thread-local variable to none
(0.1ms) SELECT COUNT() FROM "taxonomies" WHERE "taxonomies"."type" IN ('Location')
Setting current location thread-local variable to none
(0.1ms) SELECT id FROM "taxonomies" WHERE "taxonomies"."type" IN ('Location') LIMIT 1
(0.1ms) SELECT id FROM "taxonomies" WHERE "taxonomies"."type" IN ('Organization') LIMIT 1
User Load (0.2ms) SELECT "users".* FROM "users" ORDER BY firstname LIMIT 1
Completed 500 Internal Server Error in 10.2ms

NoMethodError (undefined method []' for nil:NilClass): app/controllers/application_controller.rb:355:inrescue in search_error_handler'
app/controllers/application_controller.rb:352:in search_error_handler' app/controllers/users_controller.rb:11:inblock in class:UsersController'
app/models/concerns/foreman/thread_session.rb:33:in clear_thread' lib/middleware/catch_json_parse_errors.rb:9:incall'

when I enter "arg = ghw" in search textbox on the Users screen. Shouldn't be the default value of the "options" method parameter in ApplicationController#search_error_handler an empty hash instead of a nil?

I'll check other places too.

dLobatog · 2014-01-15T15:36:45Z

Right, I'm not sure why I left nil instead of {}.

dmitri-d · 2014-01-15T17:32:56Z

common_parameters, audits, config_templates, ptables, media, architectures, subnets, and roles controllers have the same issue as above.

dmitri-d · 2014-01-15T17:37:36Z

nm, looks like your last commit fixed the issue... :)

dLobatog · 2014-01-16T00:32:59Z

It seemed to be fine except for 1.9.3 and sqlite3, which had an error I don't understand

[test] so we can see if its good to merge or not.

domcleal · 2014-01-16T08:44:05Z

@elobato try a rebase, it was a patch application error

dmitri-d · 2014-01-16T10:21:39Z

app/controllers/application_controller.rb

+
+    instance_variable_set('@keys_counter', Puppetclass.joins(:class_params).select('distinct environment_classes.lookup_key_id').group(:name).count) if options[:template_vars].include?('keys_counter')
+  end
+


This is good, but I wonder if the code setting instance variables would be better placed in corresponding controllers (mostly a readability issue).

nm, I see those are used in a quite a few controllers.

domcleal · 2014-01-17T10:44:27Z

The duplication between the index method and the around_filter worries me a bit, I worry that we might accidentally let these two implementations drift (particularly as there are security consequences with .my_hosts etc).

Can we just use a redirect_to :back in the search_error_handler? The only difference I see with that and your current implementation is that the (broken) contents of the search field remain for the user to use - and we could fix that by passing the broken value back in a parameter or session if necessary. The implementation would be a lot simpler then.

dLobatog · 2014-01-20T06:13:31Z

Done, redirect_to preserves flash[:error] so users have some feedback on what they did wrong.
"Field xxx not recognized for searching".

Thanks for the tip, I was really overcomplicating things here.

domcleal · 2014-01-20T08:50:49Z

app/controllers/api/base_controller.rb.orig

+      render_error 'standard_error', :status => 500, :locals => { :exception => error }
+    }
+
+<<<<<<< HEAD


Drop this .orig file?

dLobatog · 2014-01-20T11:03:37Z

Fixed, I guess I was a tad asleep 👅

domcleal · 2014-01-20T17:30:24Z

Thanks! As discussed on IRC, I think we can now switch to rescue_from too, and drop the around_filters.

Views that use counters from the search filter Filter to fix 500 errors with ScopedSearch This filter is applied on every controller, around the index action. It's functionality is basically to avoid 500 errors caused by broken ScopedSearch queries, for instance "wrongfield = dlkas". Filter around index to avoid 500 on scopedsearch Search test for around_filter on index Typo on scoped search error handler Use Hostgroup.my_groups when User is admin Simpler implementation of search error handler Fix tests to test redirect_to back Remove unnecessary scoped_search error UI handler

dLobatog · 2014-01-20T18:53:19Z

https://github.com/theforeman/foreman/blob/develop/app/controllers/application_controller.rb#L6 basically does it, I'm not sure what happened but the last rebase I did made it work properly (I get the error messages fixed). I tried 1.3-stable (which contains this rescue_from) but that one throws 500s when searching for the wrong thing.
Whatever that was, it's now checked by a few tests so hopefully it won't get broken anymore. Also there are a lot begin/rescue blocks that were unnecessary and this PR fixes the API errors.

domcleal · 2014-01-21T12:14:41Z

Nice, thanks @elobato, merged as f9bc5a8 for Foreman 1.4.0.

dmitri-d reviewed Jan 16, 2014
View reviewed changes

domcleal reviewed Jan 20, 2014
View reviewed changes

domcleal closed this Jan 21, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug #3920: Prevent 500 ScopedSearch errors on the API #1126

Bug #3920: Prevent 500 ScopedSearch errors on the API #1126

dLobatog commented Jan 3, 2014

dmitri-d commented Jan 15, 2014

dLobatog commented Jan 15, 2014

dmitri-d commented Jan 15, 2014

dmitri-d commented Jan 15, 2014

dLobatog commented Jan 16, 2014

domcleal commented Jan 16, 2014

dmitri-d Jan 16, 2014

dmitri-d Jan 16, 2014

domcleal commented Jan 17, 2014

dLobatog commented Jan 20, 2014

domcleal Jan 20, 2014

dLobatog commented Jan 20, 2014

domcleal commented Jan 20, 2014

dLobatog commented Jan 20, 2014

domcleal commented Jan 21, 2014


		instance_variable_set('@keys_counter', Puppetclass.joins(:class_params).select('distinct environment_classes.lookup_key_id').group(:name).count) if options[:template_vars].include?('keys_counter')
		end

Bug #3920: Prevent 500 ScopedSearch errors on the API #1126

Bug #3920: Prevent 500 ScopedSearch errors on the API #1126

Conversation

dLobatog commented Jan 3, 2014

dmitri-d commented Jan 15, 2014

dLobatog commented Jan 15, 2014

dmitri-d commented Jan 15, 2014

dmitri-d commented Jan 15, 2014

dLobatog commented Jan 16, 2014

domcleal commented Jan 16, 2014

dmitri-d Jan 16, 2014

Choose a reason for hiding this comment

dmitri-d Jan 16, 2014

Choose a reason for hiding this comment

domcleal commented Jan 17, 2014

dLobatog commented Jan 20, 2014

domcleal Jan 20, 2014

Choose a reason for hiding this comment

dLobatog commented Jan 20, 2014

domcleal commented Jan 20, 2014

dLobatog commented Jan 20, 2014

domcleal commented Jan 21, 2014