[ARVADOS] created: 1.3.0-2374-g5c941aa10
Git user
git at public.arvados.org
Tue Mar 24 21:05:13 UTC 2020
at 5c941aa10b7b4bf9d0804f98eb93a8104bfb7658 (commit)
commit 5c941aa10b7b4bf9d0804f98eb93a8104bfb7658
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Tue Mar 24 17:39:01 2020 -0300
16263: Bool filter bug fix.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/sdk/go/arvados/resource_list.go b/sdk/go/arvados/resource_list.go
index d1a25c438..a5cc7d3b9 100644
--- a/sdk/go/arvados/resource_list.go
+++ b/sdk/go/arvados/resource_list.go
@@ -55,7 +55,7 @@ func (f *Filter) UnmarshalJSON(data []byte) error {
}
operand := elements[2]
switch operand.(type) {
- case string, float64, []interface{}, nil:
+ case string, float64, []interface{}, nil, bool:
default:
return fmt.Errorf("invalid filter operand %q", elements[2])
}
commit 0761f57a7703655bdbfd77c261f3016592fb7f8b
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Tue Mar 24 18:03:34 2020 -0300
16263: Adds test exposing a bug on filter unmarshalling.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/sdk/go/arvados/resource_list_test.go b/sdk/go/arvados/resource_list_test.go
index 4e09c5375..b36e82c91 100644
--- a/sdk/go/arvados/resource_list_test.go
+++ b/sdk/go/arvados/resource_list_test.go
@@ -34,3 +34,40 @@ func TestMarshalFiltersWithNil(t *testing.T) {
t.Errorf("Encoded as %q, expected %q", buf, expect)
}
}
+
+func TestUnmarshalFiltersWithNil(t *testing.T) {
+ buf := []byte(`["modified_at","=",null]`)
+ f := &Filter{}
+ err := f.UnmarshalJSON(buf)
+ if err != nil {
+ t.Fatal(err)
+ }
+ expect := Filter{Attr: "modified_at", Operator: "=", Operand: nil}
+ if f.Attr != expect.Attr || f.Operator != expect.Operator || f.Operand != expect.Operand {
+ t.Errorf("Decoded as %q, expected %q", f, expect)
+ }
+}
+
+func TestMarshalFiltersWithBoolean(t *testing.T) {
+ buf, err := json.Marshal([]Filter{
+ {Attr: "is_active", Operator: "=", Operand: true}})
+ if err != nil {
+ t.Fatal(err)
+ }
+ if expect := []byte(`[["is_active","=",true]]`); 0 != bytes.Compare(buf, expect) {
+ t.Errorf("Encoded as %q, expected %q", buf, expect)
+ }
+}
+
+func TestUnmarshalFiltersWithBoolean(t *testing.T) {
+ buf := []byte(`["is_active","=",true]`)
+ f := &Filter{}
+ err := f.UnmarshalJSON(buf)
+ if err != nil {
+ t.Fatal(err)
+ }
+ expect := Filter{Attr: "is_active", Operator: "=", Operand: true}
+ if f.Attr != expect.Attr || f.Operator != expect.Operator || f.Operand != expect.Operand {
+ t.Errorf("Decoded as %q, expected %q", f, expect)
+ }
+}
commit 6004ef0180d72531d187d6c9df412f22bc8b3cf4
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Mar 23 16:20:41 2020 -0300
16263: Makes gofmt happy.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 5a7877d0b..c6f6b2c97 100644
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -344,15 +344,15 @@ func (conn *Conn) SpecimenDelete(ctx context.Context, options arvados.DeleteOpti
}
var userAttrsCachedFromLoginCluster = map[string]bool{
- "created_at": true,
- "email": true,
- "first_name": true,
- "is_active": true,
- "is_admin": true,
- "last_name": true,
- "modified_at": true,
- "prefs": true,
- "username": true,
+ "created_at": true,
+ "email": true,
+ "first_name": true,
+ "is_active": true,
+ "is_admin": true,
+ "last_name": true,
+ "modified_at": true,
+ "prefs": true,
+ "username": true,
"etag": false,
"full_name": false,
commit 9fb4398f258c39ac88aef0983de7692c048a2c9b
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Mar 23 15:32:59 2020 -0300
16263: Don't cache modified_by_*_uuid fields when using LoginCluster.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 279b7a51d..5a7877d0b 100644
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -351,18 +351,18 @@ var userAttrsCachedFromLoginCluster = map[string]bool{
"is_admin": true,
"last_name": true,
"modified_at": true,
- "modified_by_client_uuid": true,
- "modified_by_user_uuid": true,
"prefs": true,
"username": true,
- "etag": false,
- "full_name": false,
- "identity_url": false,
- "is_invited": false,
- "owner_uuid": false,
- "uuid": false,
- "writable_by": false,
+ "etag": false,
+ "full_name": false,
+ "identity_url": false,
+ "is_invited": false,
+ "modified_by_client_uuid": false,
+ "modified_by_user_uuid": false,
+ "owner_uuid": false,
+ "uuid": false,
+ "writable_by": false,
}
func (conn *Conn) UserList(ctx context.Context, options arvados.ListOptions) (arvados.UserList, error) {
commit 8ba91a5558a9fbb743c726a7e38f7e7dcd7bf8ee
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Mon Mar 23 12:27:18 2020 -0300
16263: Adds nullify behavior to users's batch_update endpoint.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index fbf177b01..7b82cdb61 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -486,12 +486,20 @@ class ApplicationController < ActionController::Base
# Go code may send empty values (ie: empty string instead of NULL) that
# should be translated to NULL on the database.
def set_nullable_attrs_to_null
- (resource_attrs.keys & nullable_attributes).each do |attr|
- val = resource_attrs[attr]
+ nullify_attrs(resource_attrs.to_hash).each do |k, v|
+ resource_attrs[k] = v
+ end
+ end
+
+ def nullify_attrs(a = {})
+ new_attrs = a.to_hash.symbolize_keys
+ (new_attrs.keys & nullable_attributes).each do |attr|
+ val = new_attrs[attr]
if (val.class == Integer && val == 0) || (val.class == String && val == "")
- resource_attrs[attr] = nil
+ new_attrs[attr] = nil
end
end
+ return new_attrs
end
def reload_object_before_update
diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 59c9f3948..289e82567 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -22,7 +22,7 @@ class Arvados::V1::UsersController < ApplicationController
rescue ActiveRecord::RecordNotUnique
retry
end
- u.update_attributes!(attrs)
+ u.update_attributes!(nullify_attrs(attrs))
@objects << u
end
@offset = 0
diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index b38f0d52f..817a1c9ef 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -1057,6 +1057,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
newuuid => {
'first_name' => 'noot',
'email' => 'root at remot.example.com',
+ 'username' => '',
},
}})
assert_response(:success)
commit d45255b6af1ce80d640ecdb21c0af7aa0b95370f
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Thu Mar 19 18:15:52 2020 -0300
16263: Assigns nil to select attributes when receiving empty values.
Controller may translate NULL values to "" on certain object string fields.
The same with integers, NULLs are converted to 0.
When controller retrieves objects from railsAPI and uses the data to create
or update objects, some of those fields should get converted back to NULL.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 369043e78..fbf177b01 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -45,6 +45,7 @@ class ApplicationController < ActionController::Base
before_action :load_required_parameters
before_action(:find_object_by_uuid,
except: [:index, :create] + ERROR_ACTIONS)
+ before_action(:set_nullable_attrs_to_null, only: [:update, :create])
before_action :load_limit_offset_order_params, only: [:index, :contents]
before_action :load_where_param, only: [:index, :contents]
before_action :load_filters_param, only: [:index, :contents]
@@ -478,6 +479,21 @@ class ApplicationController < ActionController::Base
@object = @objects.first
end
+ def nullable_attributes
+ []
+ end
+
+ # Go code may send empty values (ie: empty string instead of NULL) that
+ # should be translated to NULL on the database.
+ def set_nullable_attrs_to_null
+ (resource_attrs.keys & nullable_attributes).each do |attr|
+ val = resource_attrs[attr]
+ if (val.class == Integer && val == 0) || (val.class == String && val == "")
+ resource_attrs[attr] = nil
+ end
+ end
+ end
+
def reload_object_before_update
# This is necessary to prevent an ActiveRecord::ReadOnlyRecord
# error when updating an object which was retrieved using a join.
diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 224f2c0bd..59c9f3948 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -271,4 +271,8 @@ class Arvados::V1::UsersController < ApplicationController
end
super
end
+
+ def nullable_attributes
+ super + [:email, :first_name, :last_name, :username]
+ end
end
diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 753e707b6..b38f0d52f 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -88,6 +88,38 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
assert_nil created['identity_url'], 'expected no identity_url'
end
+ test "create new user with empty username" do
+ authorize_with :admin
+ post :create, params: {
+ user: {
+ first_name: "test_first_name",
+ last_name: "test_last_name",
+ username: ""
+ }
+ }
+ assert_response :success
+ created = JSON.parse(@response.body)
+ assert_equal 'test_first_name', created['first_name']
+ assert_not_nil created['uuid'], 'expected uuid for the newly created user'
+ assert_nil created['email'], 'expected no email'
+ assert_nil created['username'], 'expected no username'
+ end
+
+ test "update user with empty username" do
+ authorize_with :admin
+ user = users('spectator')
+ assert_not_nil user['username']
+ put :update, params: {
+ id: users('spectator')['uuid'],
+ user: {
+ username: ""
+ }
+ }
+ assert_response :success
+ updated = JSON.parse(@response.body)
+ assert_nil updated['username'], 'expected no username'
+ end
+
test "create user with user, vm and repo as input" do
authorize_with :admin
repo_name = 'usertestrepo'
commit c1e8853d7cc587b606fdb74ac244540476e6620f
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date: Thu Mar 19 13:18:05 2020 -0300
16263: Adds test exposing the bug.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>
diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index 2adb5811e..19dd4e1ad 100644
--- a/lib/controller/integration_test.go
+++ b/lib/controller/integration_test.go
@@ -73,7 +73,7 @@ func (s *IntegrationSuite) SetUpSuite(c *check.C) {
TLS:
Insecure: true
Login:
- # LoginCluster: z1111
+ LoginCluster: z1111
SystemLogs:
Format: text
RemoteClusters:
@@ -149,7 +149,7 @@ func (s *IntegrationSuite) clientsWithToken(clusterID string, token string) (con
return ctx, ac, kc
}
-func (s *IntegrationSuite) userClients(c *check.C, conn *rpc.Conn, rootctx context.Context, clusterID string, activate bool) (context.Context, *arvados.Client, *keepclient.KeepClient) {
+func (s *IntegrationSuite) userClients(rootctx context.Context, c *check.C, conn *rpc.Conn, clusterID string, activate bool) (context.Context, *arvados.Client, *keepclient.KeepClient) {
login, err := conn.UserSessionCreate(rootctx, rpc.UserSessionCreateOptions{
ReturnTo: ",https://example.com",
AuthInfo: rpc.UserSessionAuthInfo{
@@ -190,7 +190,7 @@ func (s *IntegrationSuite) TestGetCollectionByPDH(c *check.C) {
conn1 := s.conn("z1111")
rootctx1, _, _ := s.rootClients("z1111")
conn3 := s.conn("z3333")
- userctx1, ac1, kc1 := s.userClients(c, conn1, rootctx1, "z1111", true)
+ userctx1, ac1, kc1 := s.userClients(rootctx1, c, conn1, "z1111", true)
// Create the collection to find its PDH (but don't save it
// anywhere yet)
@@ -223,3 +223,32 @@ func (s *IntegrationSuite) TestGetCollectionByPDH(c *check.C) {
c.Check(err, check.IsNil)
c.Check(coll.PortableDataHash, check.Equals, pdh)
}
+
+// Test for bug #16263
+func (s *IntegrationSuite) TestListUsers(c *check.C) {
+ rootctx1, _, _ := s.rootClients("z1111")
+ conn1 := s.conn("z1111")
+ conn3 := s.conn("z3333")
+
+ // Make sure LoginCluster is properly configured
+ for cls := range s.testClusters {
+ c.Check(
+ s.testClusters[cls].config.Clusters[cls].Login.LoginCluster,
+ check.Equals, "z1111",
+ check.Commentf("incorrect LoginCluster config on cluster %q", cls))
+ }
+ // Make sure z1111 has users with NULL usernames
+ lst, err := conn1.UserList(rootctx1, arvados.ListOptions{Limit: -1})
+ nullUsername := false
+ c.Assert(err, check.IsNil)
+ c.Assert(len(lst.Items), check.Not(check.Equals), 0)
+ for _, user := range lst.Items {
+ if user.Username == "" {
+ nullUsername = true
+ }
+ }
+ c.Assert(nullUsername, check.Equals, true)
+ // Ask for the user list on z3333 using z1111's system root token
+ _, err = conn3.UserList(rootctx1, arvados.ListOptions{Limit: -1})
+ c.Assert(err, check.IsNil, check.Commentf("getting user list: %q", err))
+}
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list