From 4fbb5b4751488d9751710f03ebf99406d4c55410 Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <lucas@di-pentima.com.ar>
Date: Thu, 19 Mar 2020 18:15:52 -0300
Subject: [PATCH] 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@di-pentima.com.ar>
---
 .../app/controllers/application_controller.rb | 16 ++++++++++
 .../arvados/v1/users_controller.rb            |  4 +++
 .../arvados/v1/users_controller_test.rb       | 32 +++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 369043e780..fbf177b01f 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 1cf3b9d78a..fecc0620c7 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -268,4 +268,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 753e707b62..b38f0d52fb 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'
-- 
2.30.2