Arvados

Added tests at 5ea0a240e - branch 15306-api-nonjson-params

It seems that params are correctly interpreted (at least on the include_trash case), or maybe the tests aren't doing the right thing?

If I add some debug printfs to railsAPI and do a test request with workbench->controller->railsAPI, I get this:

App 28128 output: find_objects_for_index: request original_url is "https://localhost:42701/arvados/v1/collections/zzzzz-4zz18-subprojgonecoll?include_trash=false" 
App 28128 output: find_objects_for_index: request query_parameters is {"include_trash"=>"false"}
App 28128 output: find_objects_for_index: params[:include_trash] is "false", interpreting that as true

But when the tests in note-1 use ?include_trash=false, they work, and the debug printf says "params[:include_trash] is false, interpreting that as false."

I suspect the difference is that load_required_parameters() converts strings to bools, but we don't declare the include_trash parameter for the "show collection" action, so it doesn't get converted.

Updates at 0d5c86670 expose the bug. This happens also on the 1.3-dev branch so it's not a regression caused by the rails 5 upgrade.

Trying to see if this bug was introduced as a regression, I got to 87ea4388edb977d246b09b78d9d4bfa5c2ba5170 (from #9587 - 2 years ago), and the tests fail the same. I'm stopping the digging to use the time to fix the issue.

More tests & fix at b3d5254ce
Test run: https://ci.curoverse.com/job/developer-run-tests/1279/

Groups also have the same problem. Added tests and fix on 18ae48a2e
Tesst run: https://ci.curoverse.com/job/developer-run-tests/1280/

Does the JSON test really pass the include_trash parameter as JSON? The "show" half of the test says :format => :json but I think that is just specifying a desired response format.

I suspect that in order to pass JSON-encoded params to show/list, you need to do a post with :_method => "GET"...

As for the fix itself, it seems like this still leaves other similar loopholes open. Nearly all actions call find_object_by_uuid, which calls find_objects_for_index, which uses params[:include_trash] -- but only show and index go through the bool-parsing code. Perhaps we could close the loophole by calling a bool_param(:include_trash) method from application_controller.rb instead of using the raw params[:include_trash] value. That method can return false (or even error out) if the param isn't already a bool. If it isn't a bool by now then it wasn't declared by _*_requires_parameters(), isn't a supported/advertised flag for the current action, and shouldn't be used.

Status update: 57eb9ae04 - Test run: https://ci.curoverse.com/job/developer-run-tests/1284/

Added a bool_param() method that returns the param if it exists and it's a boolean, or false. A wb integration test about trashed container requests started failing, so I added include_trash definitions on ContainerRequests#show & ContainerRequests#index, but the same integration test keeps failing, so I'm investigating.

Thanks to Tom for finding the obvious error I made. Fixed the previous failure by moving the param declarations from the CR model to the controller.
Also added a log warning message when receiving non boolean parameters.

Updates at e9a3a68fd - Test run: https://ci.curoverse.com/job/developer-run-tests/1285/

LGTM, thanks