Project

General

Profile

Actions

Feature #20470

closed

Select fields with group contents query

Added by Peter Amstutz about 1 year ago. Updated 10 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Story points:
-
Release relationship:
Auto

Subtasks 1 (0 open1 closed)

Task #20479: Review 20470-contents-selectResolvedPeter Amstutz05/04/2023Actions

Related issues

Related to Arvados - Bug #20469: Responsiveness of viewing project full of running processesResolvedPeter AmstutzActions
Precedes Arvados - Bug #20527: Document new select parameter of groups.contents APIResolvedBrett Smith05/22/2023Actions
Actions #1

Updated by Peter Amstutz about 1 year ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz about 1 year ago

  • Related to Bug #20469: Responsiveness of viewing project full of running processes added
Actions #3

Updated by Peter Amstutz about 1 year ago

  • Category set to API
Actions #4

Updated by Peter Amstutz about 1 year ago

  • Tracker changed from Bug to Feature
Actions #5

Updated by Peter Amstutz about 1 year ago

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

Actions #8

Updated by Peter Amstutz about 1 year ago

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.

Actions #9

Updated by Tom Clegg about 1 year ago

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."

Actions #10

Updated by Peter Amstutz about 1 year ago

20470-contents-select @ e0f045ddedd05716bd813f801654308d3bbd2dd0

I decided to make the behavior match the test.

developer-run-tests: #3634

Actions #11

Updated by Tom Clegg about 1 year ago

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.

Actions #12

Updated by Peter Amstutz about 1 year ago

  • Assigned To set to Peter Amstutz
Actions #13

Updated by Peter Amstutz about 1 year ago

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

Actions #14

Updated by Tom Clegg about 1 year ago

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
Actions #15

Updated by Peter Amstutz about 1 year ago

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

Actions #16

Updated by Tom Clegg about 1 year ago

LGTM, thanks

Actions #17

Updated by Peter Amstutz about 1 year ago

  • Status changed from In Progress to Resolved
Actions #18

Updated by Brett Smith about 1 year ago

  • Precedes Bug #20527: Document new select parameter of groups.contents API added
Actions #19

Updated by Peter Amstutz 10 months ago

  • Release set to 66
Actions

Also available in: Atom PDF