Project

General

Profile

Actions

Feature #20470

closed

Select fields with group contents query

Added by Peter Amstutz 11 months ago. Updated 7 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 Amstutz05/04/2023Actions
Precedes Arvados - Bug #20527: Document new select parameter of groups.contents APIResolvedBrett Smith05/22/2023Actions
Actions #1

Updated by Peter Amstutz 11 months ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz 11 months ago

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

Updated by Peter Amstutz 11 months ago

  • Category set to API
Actions #4

Updated by Peter Amstutz 11 months ago

  • Tracker changed from Bug to Feature
Actions #5

Updated by Peter Amstutz 11 months 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 11 months 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 11 months 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 11 months ago

20470-contents-select @ e0f045ddedd05716bd813f801654308d3bbd2dd0

I decided to make the behavior match the test.

developer-run-tests: #3634

Actions #11

Updated by Tom Clegg 11 months 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 11 months ago

  • Assigned To set to Peter Amstutz
Actions #13

Updated by Peter Amstutz 11 months 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 11 months 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 11 months 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 11 months ago

LGTM, thanks

Actions #17

Updated by Peter Amstutz 11 months ago

  • Status changed from In Progress to Resolved
Actions #18

Updated by Brett Smith 10 months ago

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

Updated by Peter Amstutz 7 months ago

  • Release set to 66
Actions

Also available in: Atom PDF