[arvados] created: 2.7.0-6602-g9bda36e2c9
git repository hosting
git at public.arvados.org
Wed May 22 19:43:17 UTC 2024
at 9bda36e2c94aefc6cb05763e453ad4afdaa6199d (commit)
commit 9bda36e2c94aefc6cb05763e453ad4afdaa6199d
Author: Tom Clegg <tom at curii.com>
Date: Wed May 22 15:42:56 2024 -0400
12917: Support include=container_uuid at groups#contents endpoint.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/controller/router/request.go b/lib/controller/router/request.go
index 68fffa0681..254a8b7fab 100644
--- a/lib/controller/router/request.go
+++ b/lib/controller/router/request.go
@@ -141,15 +141,17 @@ func (rtr *router) loadRequestParams(req *http.Request, attrsKey string, opts in
delete(params, attrsKey)
}
- if order, ok := params["order"].(string); ok {
+ for _, paramname := range []string{"include", "order"} {
// We must accept strings ("foo, bar desc") and arrays
// (["foo", "bar desc"]) because RailsAPI does.
// Convert to an array here before trying to unmarshal
// into options structs.
- if order == "" {
- delete(params, "order")
- } else {
- params["order"] = strings.Split(order, ",")
+ if val, ok := params[paramname].(string); ok {
+ if val == "" {
+ delete(params, paramname)
+ } else {
+ params[paramname] = strings.Split(val, ",")
+ }
}
}
diff --git a/lib/controller/router/request_test.go b/lib/controller/router/request_test.go
index b689eb681f..0e19c51682 100644
--- a/lib/controller/router/request_test.go
+++ b/lib/controller/router/request_test.go
@@ -35,7 +35,8 @@ type testReq struct {
tokenInQuery bool
noContentType bool
- body *bytes.Buffer
+ body *bytes.Buffer // provided by caller
+ bodyContent []byte // set by (*testReq)Request() if body not provided by caller
}
const noToken = "(no token)"
@@ -46,8 +47,10 @@ func (tr *testReq) Request() *http.Request {
param[k] = v
}
+ var body *bytes.Buffer
if tr.body != nil {
// caller provided a buffer
+ body = tr.body
} else if tr.json {
if tr.jsonAttrsTop {
for k, v := range tr.attrs {
@@ -72,11 +75,12 @@ func (tr *testReq) Request() *http.Request {
param[tr.attrsKey] = tr.attrs
}
}
- tr.body = bytes.NewBuffer(nil)
- err := json.NewEncoder(tr.body).Encode(param)
+ body = bytes.NewBuffer(nil)
+ err := json.NewEncoder(body).Encode(param)
if err != nil {
panic(err)
}
+ tr.bodyContent = body.Bytes()
} else {
values := make(url.Values)
for k, v := range param {
@@ -97,8 +101,9 @@ func (tr *testReq) Request() *http.Request {
}
values.Set(tr.attrsKey, string(jattrs))
}
- tr.body = bytes.NewBuffer(nil)
- io.WriteString(tr.body, values.Encode())
+ body = bytes.NewBuffer(nil)
+ io.WriteString(body, values.Encode())
+ tr.bodyContent = body.Bytes()
}
method := tr.method
if method == "" {
@@ -108,7 +113,7 @@ func (tr *testReq) Request() *http.Request {
if path == "" {
path = "example/test/path"
}
- req := httptest.NewRequest(method, "https://an.example/"+path, tr.body)
+ req := httptest.NewRequest(method, "https://an.example/"+path, body)
token := tr.token
if token == "" {
token = arvadostest.ActiveTokenV2
@@ -127,10 +132,6 @@ func (tr *testReq) Request() *http.Request {
return req
}
-func (tr *testReq) bodyContent() string {
- return string(tr.body.Bytes())
-}
-
func (s *RouterSuite) TestAttrsInBody(c *check.C) {
attrs := map[string]interface{}{"foo": "bar"}
@@ -172,7 +173,7 @@ func (s *RouterSuite) TestBoolParam(c *check.C) {
} {
c.Logf("#%d, tr: %#v", i, tr)
req := tr.Request()
- c.Logf("tr.body: %s", tr.bodyContent())
+ c.Logf("tr.body: %s", tr.bodyContent)
var opts struct{ EnsureUniqueName bool }
params, err := s.rtr.loadRequestParams(req, tr.attrsKey, &opts)
c.Logf("params: %#v", params)
@@ -191,7 +192,7 @@ func (s *RouterSuite) TestBoolParam(c *check.C) {
} {
c.Logf("#%d, tr: %#v", i, tr)
req := tr.Request()
- c.Logf("tr.body: %s", tr.bodyContent())
+ c.Logf("tr.body: %s", tr.bodyContent)
var opts struct {
EnsureUniqueName bool `json:"ensure_unique_name"`
}
@@ -205,22 +206,25 @@ func (s *RouterSuite) TestBoolParam(c *check.C) {
}
}
-func (s *RouterSuite) TestOrderParam(c *check.C) {
- for i, tr := range []testReq{
- {method: "POST", param: map[string]interface{}{"order": ""}, json: true},
- {method: "POST", param: map[string]interface{}{"order": ""}, json: false},
- {method: "POST", param: map[string]interface{}{"order": []string{}}, json: true},
- {method: "POST", param: map[string]interface{}{"order": []string{}}, json: false},
- {method: "POST", param: map[string]interface{}{}, json: true},
- {method: "POST", param: map[string]interface{}{}, json: false},
- } {
- c.Logf("#%d, tr: %#v", i, tr)
- req := tr.Request()
- params, err := s.rtr.loadRequestParams(req, tr.attrsKey, nil)
- c.Assert(err, check.IsNil)
- c.Assert(params, check.NotNil)
- if order, ok := params["order"]; ok && order != nil {
- c.Check(order, check.DeepEquals, []interface{}{})
+func (s *RouterSuite) TestStringOrArrayParam(c *check.C) {
+ for _, paramname := range []string{"order", "include"} {
+ for i, tr := range []testReq{
+ {method: "POST", param: map[string]interface{}{paramname: ""}, json: true},
+ {method: "POST", param: map[string]interface{}{paramname: ""}, json: false},
+ {method: "POST", param: map[string]interface{}{paramname: []string{}}, json: true},
+ {method: "POST", param: map[string]interface{}{paramname: []string{}}, json: false},
+ {method: "POST", param: map[string]interface{}{}, json: true},
+ {method: "POST", param: map[string]interface{}{}, json: false},
+ } {
+ c.Logf("%s #%d, tr: %#v", paramname, i, tr)
+ req := tr.Request()
+ c.Logf("tr.body: %s", tr.bodyContent)
+ params, err := s.rtr.loadRequestParams(req, tr.attrsKey, nil)
+ c.Assert(err, check.IsNil)
+ c.Assert(params, check.NotNil)
+ if order, ok := params[paramname]; ok && order != nil {
+ c.Check(order, check.DeepEquals, []interface{}{})
+ }
}
}
@@ -233,6 +237,7 @@ func (s *RouterSuite) TestOrderParam(c *check.C) {
} {
c.Logf("#%d, tr: %#v", i, tr)
req := tr.Request()
+ c.Logf("tr.body: %s", tr.bodyContent)
var opts arvados.ListOptions
params, err := s.rtr.loadRequestParams(req, tr.attrsKey, &opts)
c.Assert(err, check.IsNil)
@@ -243,4 +248,40 @@ func (s *RouterSuite) TestOrderParam(c *check.C) {
c.Check(params["order"], check.DeepEquals, []interface{}{"foo", "bar desc"})
}
}
+
+ for i, tr := range []testReq{
+ {method: "POST", param: map[string]interface{}{"include": "container_uuid,owner_uuid"}, json: true},
+ {method: "POST", param: map[string]interface{}{"include": "container_uuid,owner_uuid"}, json: false},
+ {method: "POST", param: map[string]interface{}{"include": "[\"container_uuid\", \"owner_uuid\"]"}, json: false},
+ {method: "POST", param: map[string]interface{}{"include": []string{"container_uuid", "owner_uuid"}}, json: true},
+ {method: "POST", param: map[string]interface{}{"include": []string{"container_uuid", "owner_uuid"}}, json: false},
+ } {
+ c.Logf("#%d, tr: %#v", i, tr)
+ {
+ req := tr.Request()
+ c.Logf("tr.body: %s", tr.bodyContent)
+ var opts arvados.ListOptions
+ params, err := s.rtr.loadRequestParams(req, tr.attrsKey, &opts)
+ c.Assert(err, check.IsNil)
+ c.Check(opts.Include, check.DeepEquals, []string{"container_uuid", "owner_uuid"})
+ if _, ok := params["include"].([]string); ok {
+ c.Check(params["include"], check.DeepEquals, []string{"container_uuid", "owner_uuid"})
+ } else {
+ c.Check(params["include"], check.DeepEquals, []interface{}{"container_uuid", "owner_uuid"})
+ }
+ }
+ {
+ req := tr.Request()
+ c.Logf("tr.body: %s", tr.bodyContent)
+ var opts arvados.GroupContentsOptions
+ params, err := s.rtr.loadRequestParams(req, tr.attrsKey, &opts)
+ c.Assert(err, check.IsNil)
+ c.Check(opts.Include, check.DeepEquals, []string{"container_uuid", "owner_uuid"})
+ if _, ok := params["include"].([]string); ok {
+ c.Check(params["include"], check.DeepEquals, []string{"container_uuid", "owner_uuid"})
+ } else {
+ c.Check(params["include"], check.DeepEquals, []interface{}{"container_uuid", "owner_uuid"})
+ }
+ }
+ }
}
diff --git a/sdk/go/arvados/api.go b/sdk/go/arvados/api.go
index dd1a6c4c32..d2e2b2088c 100644
--- a/sdk/go/arvados/api.go
+++ b/sdk/go/arvados/api.go
@@ -151,7 +151,7 @@ type ListOptions struct {
IncludeOldVersions bool `json:"include_old_versions"`
BypassFederation bool `json:"bypass_federation"`
ForwardedFor string `json:"forwarded_for,omitempty"`
- Include string `json:"include"`
+ Include []string `json:"include"`
}
type CreateOptions struct {
@@ -182,7 +182,7 @@ type GroupContentsOptions struct {
Order []string `json:"order"`
Distinct bool `json:"distinct"`
Count string `json:"count"`
- Include string `json:"include"`
+ Include []string `json:"include"`
Recursive bool `json:"recursive"`
IncludeTrash bool `json:"include_trash"`
IncludeOldVersions bool `json:"include_old_versions"`
diff --git a/sdk/python/arvados-v1-discovery.json b/sdk/python/arvados-v1-discovery.json
index ef187f6663..0dcec6bb20 100644
--- a/sdk/python/arvados-v1-discovery.json
+++ b/sdk/python/arvados-v1-discovery.json
@@ -2660,9 +2660,9 @@
"location": "query"
},
"include": {
- "type": "string",
+ "type": "array",
"required": false,
- "description": "Include objects referred to by listed field in \"included\" (only owner_uuid).",
+ "description": "Include objects referred to by listed fields in \"included\" response field. Subsets of [\"owner_uuid\", \"container_uuid\"] are supported.",
"location": "query"
},
"include_old_versions": {
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index be73d39dd1..11212e1b69 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -7,6 +7,7 @@ require "trashable"
class Arvados::V1::GroupsController < ApplicationController
include TrashableController
+ before_action :load_include_param, only: [:shared, :contents]
skip_before_action :find_object_by_uuid, only: :shared
skip_before_action :render_404_if_no_object, only: :shared
@@ -40,7 +41,7 @@ class Arvados::V1::GroupsController < ApplicationController
type: 'boolean', required: false, default: false, description: 'Include contents from child groups recursively.',
},
include: {
- type: 'string', required: false, description: 'Include objects referred to by listed field in "included" (only owner_uuid).',
+ type: 'array', required: false, description: 'Include objects referred to by listed fields in "included" response field. Subsets of ["owner_uuid", "container_uuid"] are supported.',
},
include_old_versions: {
type: 'boolean', required: false, default: false, description: 'Include past collection versions.',
@@ -130,6 +131,7 @@ class Arvados::V1::GroupsController < ApplicationController
end
def contents
+ @orig_select = @select
load_searchable_objects
list = {
:kind => "arvados#objectList",
@@ -143,7 +145,7 @@ class Arvados::V1::GroupsController < ApplicationController
list[:items_available] = @items_available
end
if @extra_included
- list[:included] = @extra_included.as_api_response(nil, {select: @select})
+ list[:included] = @extra_included.as_api_response(nil, {select: @orig_select})
end
send_json(list)
end
@@ -158,7 +160,6 @@ class Arvados::V1::GroupsController < ApplicationController
# This also returns (in the "included" field) the objects that own
# those projects (users or non-project groups).
#
- #
# The intended use of this endpoint is to support clients which
# wish to browse those projects which are visible to the user but
# are not part of the "home" project.
@@ -170,14 +171,22 @@ class Arvados::V1::GroupsController < ApplicationController
apply_where_limit_order_params
- if params["include"] == "owner_uuid"
+ if @include.include?("owner_uuid")
owners = @objects.map(&:owner_uuid).to_set
- @extra_included = []
+ @extra_included ||= []
[Group, User].each do |klass|
@extra_included += klass.readable_by(*@read_users).where(uuid: owners.to_a).to_a
end
end
+ if @include.include?("container_uuid")
+ @extra_included ||= []
+ container_uuids = @objects.map { |o|
+ o.respond_to?(:container_uuid) ? o.container_uuid : nil
+ }.compact.to_set.to_a
+ @extra_included += Container.where(uuid: container_uuids).to_a
+ end
+
index
end
@@ -189,6 +198,19 @@ class Arvados::V1::GroupsController < ApplicationController
protected
+ def load_include_param
+ @include = params[:include]
+ if @include.nil? || @include == ""
+ @include = Set[]
+ elsif @include.is_a?(String) && @include.start_with?('[')
+ @include = SafeJSON.load(@include).to_set
+ elsif @include.is_a?(String)
+ @include = Set[@include]
+ else
+ return send_error("'include' parameter must be a string or array", status: 422)
+ end
+ end
+
def load_searchable_objects
all_objects = []
@items_available = 0
@@ -262,6 +284,9 @@ class Arvados::V1::GroupsController < ApplicationController
klasses.each do |klass|
all_attributes.concat klass.selectable_attributes
end
+ if klasses.include?(ContainerRequest) && @include.include?("container_uuid")
+ all_attributes.concat Container.selectable_attributes
+ end
@select.each do |check|
if !all_attributes.include? check
raise ArgumentError.new "Invalid attribute '#{check}' in select"
@@ -371,7 +396,7 @@ class Arvados::V1::GroupsController < ApplicationController
limit_all = all_objects.count
end
- if params["include"] == "owner_uuid"
+ if @include.include?("owner_uuid")
owners = klass_object_list[:items].map {|i| i[:owner_uuid]}.to_set
[Group, User].each do |ownerklass|
ownerklass.readable_by(*@read_users).where(uuid: owners.to_a).each do |ow|
@@ -379,6 +404,13 @@ class Arvados::V1::GroupsController < ApplicationController
end
end
end
+
+ if @include.include?("container_uuid") && klass == ContainerRequest
+ containers = klass_object_list[:items].collect { |cr| cr[:container_uuid] }.to_set
+ Container.where(uuid: containers.to_a).each do |c|
+ included_by_uuid[c.uuid] = c
+ end
+ end
end
# Only error out when every searchable object type errored out
@@ -389,7 +421,7 @@ class Arvados::V1::GroupsController < ApplicationController
raise ArgumentError.new(error_msg)
end
- if params["include"]
+ if !@include.empty?
@extra_included = included_by_uuid.values
end
@@ -420,5 +452,4 @@ class Arvados::V1::GroupsController < ApplicationController
"EXISTS(SELECT 1 FROM groups as gp where gp.uuid=#{klass.table_name}.owner_uuid and gp.group_class != 'project')",
user_uuid: current_user.uuid)
end
-
end
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 6e167bb91e..52ed140bae 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -947,6 +947,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
end
get :contents, params: {:include => "owner_uuid", :exclude_home_project => true}
+ assert_response 200
assert_equal 1, json_response['items'].length
assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
@@ -963,6 +964,42 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_response 422
end
+ [[false, 'owner_uuid'],
+ [false, []],
+ [false, ''],
+ [true, 'container_uuid'],
+ [true, ['container_uuid']],
+ [true, ['owner_uuid', 'container_uuid'], ['uuid', 'container_uuid', 'state', 'output']],
+ ].each do |check_container_included, include_param, select_param|
+ test "contents, include=#{include_param.inspect}" do
+ authorize_with :active
+ get :contents, params: {
+ :id => users(:active).uuid,
+ :include => include_param,
+ :limit => 1000,
+ :select => select_param,
+ }
+ assert_response 200
+ if include_param.empty?
+ assert_equal false, json_response.include?('included')
+ return
+ end
+ incl = {}
+ json_response['included'].andand.each do |ctr|
+ incl[ctr['uuid']] = ctr
+ end
+ next if !check_container_included
+ checked_crs = 0
+ json_response['items'].each do |item|
+ next if !item['container_uuid']
+ assert_equal item['container_uuid'], incl[item['container_uuid']]['uuid']
+ assert_not_empty incl[item['container_uuid']]['state']
+ checked_crs += 1
+ end
+ assert_operator 0, :<, checked_crs
+ end
+ end
+
test "include_trash does not return trash inside frozen project" do
authorize_with :active
trashtime = Time.now - 1.second
diff --git a/services/api/test/integration/groups_test.rb b/services/api/test/integration/groups_test.rb
index bc5a08c2c8..4c688756d7 100644
--- a/services/api/test/integration/groups_test.rb
+++ b/services/api/test/integration/groups_test.rb
@@ -157,6 +157,23 @@ class GroupsTest < ActionDispatch::IntegrationTest
end
end
end
+
+ test "group contents with include=array" do
+ get "/arvados/v1/groups/contents",
+ params: {
+ filters: [["uuid", "is_a", "arvados#container_request"]].to_json,
+ include: ["container_uuid"].to_json,
+ select: ["uuid", "state"],
+ limit: 1000,
+ },
+ headers: auth(:active)
+ assert_response 200
+ incl = {}
+ json_response['included'].each { |i| incl[i['uuid']] = i }
+ json_response['items'].each do |c|
+ assert_not_nil incl[c['container_uuid']]['state']
+ end
+ end
end
class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list