Arvados

I'm not sure about the change to load_searchable_objects. It looks convenient for Workbench's current needs ("only show top-level container requests"), but what about when Workbench or some other client wants to search all items in the project, including the "child" CRs?

• Filter out the child CRs on the client side (but this will be slow if there are lots of child CRs)
• Accept filters like ["container_requests.requesting_container_uuid","=",nil], at least in GroupsController#index: load_searchable_objects could make a table-specific @filters before calling apply_where_limit_order_params, omitting any filters that specify a table other than klass.table_name.

Thoughts?

Two minor things

apps/workbench/app/views/projects/_show_pipelines_and_processes.html.erb has some forbidden tab characters.

Comment in infinite_scroll.js should probably say "container requests and pipeline instances" instead of "pipelines and processes".

I'm not sure about the change to load_searchable_objects. It looks convenient for Workbench's current needs ("only show top-level container requests"), but what about when Workbench or some other client wants to search all items in the project, including the "child" CRs? I can think of two ways to make this explicit:

Filter out the child CRs on the client side (but this will be slow if there are lots of child CRs)

Yes, I do not want to do something that is bad for performance knowingly

Accept filters like ["container_requests.requesting_container_uuid","=",nil], at least in GroupsController#index: load_searchable_objects could make a table-specific @filters before calling apply_where_limit_order_params, omitting any filters that specify a table other than klass.table_name.

Added this in groups_controller. Currently this supports only '=' operator. That is, ['table_name.column_name', '=', 'some value']

apps/workbench/app/views/projects/_show_pipelines_and_processes.html.erb has some forbidden tab characters.

Corrected this

Comment in infinite_scroll.js should probably say "container requests and pipeline instances" instead of "pipelines and processes".

Updated accordingly

• I am assuming we need to document the fact that we will support filters such as ['table_name.column_name', '=', 'some value'] ?

Radhika Chippada wrote:

Added this in groups_controller. Currently this supports only '=' operator. That is, ['table_name.column_name', '=', 'some value']

It looks like ["table_name.column_name","!=","foo"] will be taken to mean ["table_name.column_name","=","foo"], which doesn't seem right (if we can't do what the client asks, we should send an error rather than just doing something different).

But more generally: rather than writing more code to translate filters to ActiveRecord queries, why not just put the desired set of filters in @filters and let apply_where_limit_order_params translate them with the full set of features? That way, we'd support all of the operators using the same code we use for other queries. I thought something like this would work:

request_filters = @filters
[...].each do |klass|
  ...
  @filters = request_filters.map do |col, op, val|
    if !col.index('.')
      [col, op, val]
    elsif (col = col.split('.', 2))[0] == klass.table_name
      [col[1], op, val]
    else
      nil
    end
  end.compact
  apply_where_limit_order_params klass
  ...
end

(I think provided_column == klass.table_name is more direct than (@provided_column.classify.constantize rescue nil), and avoids using the client-provided string for anything other than a string comparison...)

• I am assuming we need to document the fact that we will support filters such as ['table_name.column_name', '=', 'some value'] ?

Yes, we should mention this at http://doc.arvados.org/api/methods/groups.html

rather than writing more code to translate filters to ActiveRecord queries, why not just put the desired set of filters in @filters and let apply_where_limit_order_params translate them with the full set of features?

Used this. This is so much better than previous version with no constraint on the operator. Thanks.

Doc update: http://doc.arvados.org/api/methods/groups.html

Done

This test case seems to protect us from accidentally fixing a bug. It's good to test that we don't crash or anything, but wouldn't it be even better if this request was rejected with an error like "invalid attribute no_such_table.uuid"? Perhaps we should add a TODO in GroupsController?

[['no_such_table.uuid', '!=', 'zzzzz-tpzed-xurymjxw79nv3jz'], 200]

I think it would also be good to test that the filters are actually being applied, not just that the response isn't empty. Perhaps each test case could specify one UUID that should be included in the results, and one that should not?

[['no_such_table.uuid' ...] wouldn't it be even better if this request was rejected with an error like "invalid attribute no_such_table.uuid"? Perhaps we should add a TODO in GroupsController?

Gandhi said, "Do tomorrow's work today and today's work now," and so I added this rather than add a TODO

I think it would also be good to test that the filters are actually being applied, not just that the response isn't empty. Perhaps each test case could specify one UUID that should be included in the results, and one that should not?

Yes, I enhanced the tests and also added more failing tests with invalid attributes.

I still don't think we should do the "classify.constantize" thing; it's better to go the other way around, with table_name. We should also make sure a filter like ["users.uuid","=","foo"] is rejected: "users" is a real table, but we can't filter groups.contents on users.uuid. Similarly, "time.uuid" passes the classify.constantize test, so it will also be silently ignored.

How about leaving the request_filters.map the way it was, and just validate all filters once, above the loop, using something like this:

klasses = [Group, Job, PipelineInstance, ...]
tables = klasses.map(&:table_name)
request_filters.each do |col, op, val|
  if col.index('.') && !tables.include?(col.split('.', 2)[0])
    raise ....
  end
end
klasses.each do |klass|
  ...

I still don't think we should do the "classify.constantize" thing; it's better to go the other way around, with table_name ...How about leaving the request_filters.map the way it was, and just validate all filters once, above the loop, using something like this: tables = klasses.map(&:table_name)

This is absolutely a better alternative to my classify.constantize. Updated accordingly. Thanks.

LGTM @ b9f14b6, thanks!

One minor thing: shouldn't these "get contents with '#{filter}' filter" tests be functional tests instead of integration tests, since they each make just one API call?

One minor thing: shouldn't these "get contents with '#{filter}' filter" tests be functional tests instead of integration tests, since they each make just one API call?

You are right. Converted the test(s) into functional tests. Thanks.