Arvados

20470-contents-select @ f4e3341d2857114e8c3ea0a7941ff8429491dd59

• Make "select" parameter work for group contents
• Each column is either prefixed (selects a specific column of a specific table) or unprefixed (selects that column for all tables, e.g. "uuid")

developer-run-tests: #3630

20470-contents-select @ 385d9ac5f47bec5c5a5fc9770c74b1a7d8dd2974

• Fix nil case

developer-run-tests: #3631

20470-contents-select @ ff843a40de882d22dd2a5e408c77a2fa4720cc7d

• fix tests

developer-run-tests: #3632

Failing test:

14:45:19 FAIL: collection_test.go:249: CollectionSuite.TestSignatures
14:45:19 
14:45:19 collection_test.go:318:
14:45:19     c.Check(gresp.Items[0].(map[string]interface{})["manifest_text"], check.Equals, nil)
14:45:19 ... obtained string = ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n" 
14:45:19 ... expected = nil

What is happening here is that the test Select: []string{"uuid", "manifest_text"} on the GroupContents request. However this was a no-op before. So the test is buggy. What changed is that select now does something and manifest_text is returned if you ask for it.

I think maybe the fix here is to split the test into one version that doesn't have "select" (so it won't return manifest_text) and the one that does select on "manifest_text" and checks the signature.

The comment on that check says

        // Make sure groups/contents doesn't return manifest_text with                                                                                                                                           
        // collections (if it did, we'd need to sign it).                                                                                                                                                        

I think that's trying to say "if selecting manifest_text this way actually worked, we would check here that it's signed."

20470-contents-select @ e0f045ddedd05716bd813f801654308d3bbd2dd0

I decided to make the behavior match the test.

developer-run-tests: #3634

It looks like we're now silently ignoring bogus attributes in "select", except in the specific case where all select attributes are bogus, where we now return a somewhat misleading/confusing error message. This doesn't seem ideal.

I think we could maintain the error-checking behavior by doing "customize @select for each table" in GroupsController.load_searchable_objects, similar to limit_all/offset_all. We could explicitly check at the top that each "select" entry is valid for at least one klass, and error out if not. Then set @select to the appropriate subset for each klass inside the loop.

20470-contents-select @ 2c854a99434c76e0165543bedfe9e97a1fe81ae8

• make the error reporting which 'select' field is invalid work again

developer-run-tests: #3639

wb1 re-run: developer-run-tests-apps-workbench-integration: #3928

Could we have a comment in ContainersController#find_objects_for_index and ApplicationController#find_object_by_uuid, about why @preserve_select is needed? I think the reason is to ensure ApplicationController#object_list uses the same set of fields.

      @select = @preserve_select = %w(uuid state priority auth_uuid locked_by_uuid)

I'm a little confused by this code -- it looks like it will incorrectly accept "collections.bogus" without checking it, but I couldn't make it behave badly in a test case.

      sp = column.split(".")
      if sp.length == 2 && sp[0] == model_class.table_name
        sp[1]
      elsif model_class.selectable_attributes.include? column
        column
      elsif raise_unknown
        raise ArgumentError.new("Invalid attribute '#{column}' of #{model_class.name} in select parameter")
      else
        nil
      end

20470-contents-select @ 9d3ace1fdba783134eb5557a9b28f8132df552de

• Use selectable_attributes to determine which fields are published in the schema
• Don't accept "collections.bogus" as a selectable field
• Update comments to note the @preserve_select workaround

developer-run-tests: #3644

wb1 re-run developer-run-tests-apps-workbench-integration: #3934

LGTM, thanks