API 500 response: removing member from group

Feature
1

I’m getting a 500 response from the /groups/{group_id}/members.json endpoint:

Message

NoMethodError (undefined method `split' for 862:Integer) /var/www/discourse/app/controllers/groups_controller.rb:496:in `users_from_params'

Backtrace

/var/www/discourse/app/controllers/groups_controller.rb:496:in `users_from_params'
/var/www/discourse/app/controllers/groups_controller.rb:333:in `remove_member' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/abstract_controller/base.rb:194:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal/rendering.rb:30:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/abstract_controller/callbacks.rb:42:in `block in process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activesupport-5.2.2/lib/active_support/callbacks.rb:132:in `run_callbacks' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/abstract_controller/callbacks.rb:41:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal/rescue.rb:22:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal/instrumentation.rb:34:in `block in process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activesupport-5.2.2/lib/active_support/notifications.rb:168:in `block in instrument' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activesupport-5.2.2/lib/active_support/notifications/instrumenter.rb:23:in `instrument' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activesupport-5.2.2/lib/active_support/notifications.rb:168:in `instrument' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal/instrumentation.rb:32:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal/params_wrapper.rb:256:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activerecord-5.2.2/lib/active_record/railties/controller_runtime.rb:24:in `process_action' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/abstract_controller/base.rb:134:in `process' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionview-5.2.2/lib/action_view/rendering.rb:32:in `process' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-mini-profiler-1.0.1/lib/mini_profiler/profiling_methods.rb:78:in `block in profile_method' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal.rb:191:in `dispatch' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_controller/metal.rb:252:in `dispatch' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/routing/route_set.rb:52:in `dispatch' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/routing/route_set.rb:34:in `serve' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/journey/router.rb:52:in `block in serve' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/journey/router.rb:35:in `each' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/journey/router.rb:35:in `serve' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/routing/route_set.rb:840:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-protection-2.0.3/lib/rack/protection/frame_options.rb:31:in `call' 
/var/www/discourse/lib/middleware/omniauth_bypass_middleware.rb:32:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/tempfile_reaper.rb:15:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/conditional_get.rb:38:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/head.rb:12:in `call' 
/var/www/discourse/lib/content_security_policy/middleware.rb:12:in `call' 
/var/www/discourse/lib/middleware/anonymous_cache.rb:216:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/session/abstract/id.rb:232:in `context' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/session/abstract/id.rb:226:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/cookies.rb:670:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/callbacks.rb:28:in `block in call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activesupport-5.2.2/lib/active_support/callbacks.rb:98:in `run_callbacks' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/callbacks.rb:26:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/debug_exceptions.rb:61:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/show_exceptions.rb:33:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/logster-1.3.4/lib/logster/middleware/reporter.rb:31:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/railties-5.2.2/lib/rails/rack/logger.rb:38:in `call_app' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/railties-5.2.2/lib/rails/rack/logger.rb:28:in `call' 
/var/www/discourse/config/initializers/100-quiet_logger.rb:16:in `call' 
/var/www/discourse/config/initializers/100-silence_logger.rb:29:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/remote_ip.rb:81:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/request_id.rb:27:in `call' 
/var/www/discourse/lib/middleware/enforce_hostname.rb:17:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/method_override.rb:22:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/middleware/executor.rb:14:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/sendfile.rb:111:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-mini-profiler-1.0.1/lib/mini_profiler/profiler.rb:171:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/message_bus-2.2.0.pre.1/lib/message_bus/rack/middleware.rb:57:in `call' 
/var/www/discourse/lib/middleware/request_tracker.rb:180:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/railties-5.2.2/lib/rails/engine.rb:524:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/railties-5.2.2/lib/rails/railtie.rb:190:in `public_send' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/railties-5.2.2/lib/rails/railtie.rb:190:in `method_missing' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/urlmap.rb:68:in `block in call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/urlmap.rb:53:in `each' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/rack-2.0.6/lib/rack/urlmap.rb:53:in `call' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/unicorn-5.4.0/lib/unicorn/http_server.rb:606:in `process_client' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/unicorn-5.4.0/lib/unicorn/http_server.rb:701:in `worker_loop' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/unicorn-5.4.0/lib/unicorn/http_server.rb:549:in `spawn_missing_workers' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/unicorn-5.4.0/lib/unicorn/http_server.rb:142:in `start' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/unicorn-5.4.0/bin/unicorn:126:in `<top (required)>' /var/www/discourse/vendor/bundle/ruby/2.5.0/bin/unicorn:23:in `load' 
/var/www/discourse/vendor/bundle/ruby/2.5.0/bin/unicorn:23:in `<main>'

Env

hostname talk-app
process_id 210
application_version 37088c4221bbaa8de1d24309376da1740ce455a2
HTTP_HOST ***********
REQUEST_URI /groups/54/members.json?api_key=**********&api_username=system
REQUEST_METHOD DELETE
HTTP_USER_AGENT python-requests/2.21.0
HTTP_ACCEPT */*
HTTP_X_FORWARDED_FOR ***********
HTTP_X_REAL_IP  *******
username                 system
params                   api_key       ***********
                         api_username  system

https://github.com/discourse/discourse/blob/master/app/controllers/groups_controller.rb#L496

It appears that your code is expected a comma-delimited string of user IDs, yet your documentation clearly indicates to use a single ID as an integer: Discourse API Docs

Now that I’ve looked at the code I can work around it (and having the option of passing multiple users at once will make my client code much simpler, the docs should probably reflect that), but I think it should still allow for a single integer to be passed, as well as a comma-delimited string.

3 Likes
2

Hmmm … @tgxworld this sure is some confusing code:

https://github.com/discourse/discourse/blob/77d947701c870cb4bd4ec7cb8e9786962c5bf58e/app/controllers/groups_controller.rb#L330-L330

Not sure how it even works to be honest, how is the ajax call able to pass a string there?

2 Likes
3

We’ll need to check if @blake for this one since that particular line was introduced in https://github.com/discourse/discourse/commit/13b3cead06d5d658de69f26582688f470e0365cc

5 Likes
4

It does allow for a single integer to be passed (this is how the discourse app does it). Maybe you forgot to add it, but in your example you aren’t passing in a user_id as a param?

The request in curl will look like this:

curl -i -sS -X DELETE "http://127.0.0.1:3000/groups/42/members.json" \
-H "Content-Type: multipart/form-data;" \
-F "api_key=api_key" \
-F "api_username=system" \ 
-F "user_id=3"

Where “42” is the group_id and “3” is the user_id.

Which returns

{"success":"OK","usernames":["27d52e4065601e0e28e3"]}

I’ll be sure to add to the api docs the comma-delimited endpoint, but the single ID as an integer is still a valid way of removing a single user from a group.

If its a string the ajax call wouldn’t use params[:user_ids] it would use params[:usernames] or params[:user_emails].

3 Likes
5

I was using a single integer, but in the request body as JSON, not form data. That is why the parser is picking it up as an integer, not as an integer-in-a-string which is what happens with the form data.

3 Likes
6

Yeah we should clean the API so if you pass user_id it is unconditionally treated as an integer with an extra .to_i, if it is user_ids it is treated as string and split on ,.

@blake can you take cleaning this up?

4 Likes
8

Cleaned up here:

https://github.com/discourse/discourse/commit/de47b35b2d096d0fc9ac22e8cb85a38bba39bd70

5 Likes