Story #3412

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

Added by Misha Zatsman about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
API
Target version:
Start date:
08/08/2014
Due date:
% Done:

100%

Estimated time:
(Total: 2.00 h)
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

Task #3534: Review 3412-full-collections-index-wipResolvedBrett Smith


Related issues

Blocks Arvados - Story #3408: [Keep] Implement Production Data ManagerResolved07/29/2014

Associated revisions

Revision 0ce64862
Added by Brett Smith about 5 years ago

Merge branch '3412-full-collections-index'

Closes #3412, #3534.

History

#1 Updated by Ward Vandewege about 5 years ago

  • Target version set to 2014-08-27 Sprint

#2 Updated by Ward Vandewege about 5 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

#3 Updated by Ward Vandewege about 5 years ago

  • Story points set to 1.0

#4 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)
  • Category set to API

#5 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#6 Updated by Brett Smith about 5 years ago

  • Assigned To set to Brett Smith

#7 Updated by Brett Smith about 5 years ago

  • Status changed from New to In Progress

#8 Updated by Brett Smith about 5 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.

#9 Updated by Tom Clegg about 5 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.

#10 Updated by Brett Smith about 5 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.

#11 Updated by Tom Clegg about 5 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!

#12 Updated by Brett Smith about 5 years ago

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

Applied in changeset arvados|commit:0ce64862c39e93591635f31a47c5b7d4aa0b9a19.

Also available in: Atom PDF