Arvados

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.

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.

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.

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!