Project

General

Profile

Actions

Idea #3412

closed

[API] Full collection view in list returned by api server

Added by Misha Zatsman over 9 years ago. Updated over 9 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Story points:
1.0

Description

Currently the api server shows all the information about a collection when you request a single collection, but when it lists all the collections it only shows a subset of the information available for each collection.

For example, the manifest text is returned in the single collection view, but not in the list view.

It should be possible to use the select parameter in the collection list request to make the api server return all desired information about the collections.

This will prevent the data manager from hammering the network and api server.


Subtasks 1 (0 open1 closed)

Task #3534: Review 3412-full-collections-index-wipResolvedBrett Smith08/08/2014Actions

Related issues

Blocks Arvados - Idea #3408: [Keep] Implement Production Data ManagerResolvedPeter Amstutz07/29/2014Actions
Actions #1

Updated by Ward Vandewege over 9 years ago

  • Target version set to 2014-08-27 Sprint
Actions #2

Updated by Ward Vandewege over 9 years ago

  • Subject changed from Full collection view in list returned by api server to [API] Full collection view in list returned by api server
Actions #3

Updated by Ward Vandewege over 9 years ago

  • Story points set to 1.0
Actions #4

Updated by Tom Clegg over 9 years ago

  • Description updated (diff)
  • Category set to API
Actions #5

Updated by Tom Clegg over 9 years ago

  • Description updated (diff)
Actions #6

Updated by Brett Smith over 9 years ago

  • Assigned To set to Brett Smith
Actions #7

Updated by Brett Smith over 9 years ago

  • Status changed from New to In Progress
Actions #8

Updated by Brett Smith over 9 years ago

Branch 3412-full-collections-index-wip implements this and is up for review. Tom mentioned on IRC that manifest_text should only be provided when the caller expressly requests it via select. I've added documentation to that effect.

Actions #9

Updated by Tom Clegg over 9 years ago

In services/api/app/controllers/application_controller.rb

  • Could we generalize the "API response field A requires loading database columns B and C" logic here? Something like
  # app/models/arvados_model.rb
  def self.attribute_dependencies
    {}
  end

  # app/models/collection.rb
  def self.attribute_dependencies
    super.merge({ 'files' => ['manifest_text'] })
  end

  # app/controllers/application_controller.rb
    need_db_cols = @select & model_class.columns.map { |c| c.name.to_s }
    (@select - need_db_cols).each do |c|
      need_db_cols += (model_class.attribute_dependencies[c] || [])
    end
    @objects = @objects.select((need_db_cols
                                .uniq
                                .map { |s| "#{table_name}.#{ActiveRecord::Base.connection.quote_column_name s.to_s}" }
                                .join ", ") if @select

I think that might be neater than putting this logic in CollectionsController, especially if we also want to DTRT for other similar cases like node#crunch_worker_state.

In services/api/app/controllers/arvados/v1/collections_controller.rb

  • I love munge_manifest_locators. Thanks for that.

Everything else here LGTM.

Actions #10

Updated by Brett Smith over 9 years ago

Tom Clegg wrote:

In services/api/app/controllers/application_controller.rb

  • Could we generalize the "API response field A requires loading database columns B and C" logic here?

If you're going to write half the code for me, I don't see why not.

Actually, instead of having the method return special cases, I have it generate a full map of API attributes to column names. I figure this gives subclasses even more flexibility about adjusting the results if needed, and when we handle @select, we don't have to split between two kinds of cases: we just get all column names from the method.

  • I love munge_manifest_locators. Thanks for that.

Aww, shucks, I was just being lazy because I didn't want to copy the code a third time for #index. (I'm genuinely curious what you like so much about it.)

Ready for another look at 198b34d.

Actions #11

Updated by Tom Clegg over 9 years ago

Brett Smith wrote:

Actually, instead of having the method return special cases, I have it generate a full map of API attributes to column names. I figure this gives subclasses even more flexibility about adjusting the results if needed, and when we handle @select, we don't have to split between two kinds of cases: we just get all column names from the method.

Even better.

Aww, shucks, I was just being lazy because I didn't want to copy the code a third time for #index. (I'm genuinely curious what you like so much about it.)

I think it was the combination of cleaning up existing code and using &block nicely. And: one small step closer to supporting more/better manifest formats.

LGTM, thanks!

Actions #12

Updated by Brett Smith over 9 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:0ce64862c39e93591635f31a47c5b7d4aa0b9a19.

Actions

Also available in: Atom PDF