From c3872e7a1c817cd39b702f694f70d34f28f7f472 Mon Sep 17 00:00:00 2001
From: Peter Amstutz <peter.amstutz@curii.com>
Date: Wed, 29 Nov 2023 14:00:06 -0500
Subject: [PATCH] 21205: Now adds the final part of the uuid to make the name
unique
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>
---
services/api/app/models/arvados_model.rb | 50 ++++++++++++-------
.../arvados/v1/collections_controller_test.rb | 4 +-
.../arvados/v1/groups_controller_test.rb | 4 +-
.../api/test/unit/container_request_test.rb | 4 +-
4 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index b0a66a6cb6..9ee2cca410 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -24,6 +24,7 @@ class ArvadosModel < ApplicationRecord
before_destroy :ensure_owner_uuid_is_permitted
before_destroy :ensure_permission_to_destroy
before_create :update_modified_by_fields
+ before_create :add_uuid_to_name, :if => Proc.new { @_add_uuid_to_name }
before_update :maybe_update_modified_by_fields
after_create :log_create
after_update :log_update
@@ -471,9 +472,7 @@ class ArvadosModel < ApplicationRecord
end
def save_with_unique_name!
- uuid_was = uuid
- name_was = name
- max_retries = 3
+ max_retries = 2
transaction do
conn = ActiveRecord::Base.connection
conn.exec_query 'SAVEPOINT save_with_unique_name'
@@ -503,27 +502,20 @@ class ArvadosModel < ApplicationRecord
conn.exec_query 'ROLLBACK TO SAVEPOINT save_with_unique_name'
- if uuid_was.nil? && !uuid.nil?
+ if uuid_was.nil?
+ # new record, the uuid caused a name collision (very
+ # unlikely but possible), so generate new uuid
self[:uuid] = nil
if self.is_a? Collection
- # Reset so that is assigned to the new UUID
+ # Also needs to be reset
self[:current_version_uuid] = nil
end
+ # need to adjust the name after the uuid has been generated
+ add_uuid_to_make_unique_name
+ else
+ # existing record, just update the name directly.
+ add_uuid_to_name
end
-
- # make sure we have a uuid
- self.assign_uuid
-
- # new_name used to have a timestamp added to it, but it turns
- # out that timestamps with 1ms precision and 2 retries wasn't
- # enough to avoid collisions (as well as being inherently
- # limited to 1000 collection creations per second). So to
- # scale better, append the final part of the uuid.
- #
- # The name field has a limit of 256 characters, so also
- # truncate if necessary to avoid throwing a "field too big"
- # exception.
- self[:name] = "#{name_was[0..236]} (#{self.uuid[-15..-1]})"
retry
end
end
@@ -584,6 +576,26 @@ class ArvadosModel < ApplicationRecord
*ft[:param_out])
end
+ @_add_uuid_to_name = false
+ def add_uuid_to_make_unique_name
+ @_add_uuid_to_name = true
+ end
+
+ def add_uuid_to_name
+ # Incorporate the random part of the UUID into the name. This
+ # lets us prevent name collision but the part we add to the name
+ # is still somewhat meaningful (instead of generating a second
+ # random meaningless string).
+ #
+ # Because ArvadosModel is an abstract class and assign_uuid is
+ # part of HasUuid (which is included by the other concrete
+ # classes) the assign_uuid hook gets added (and run) after this
+ # one. So we need to call assign_uuid here to make sure we have a
+ # uuid.
+ assign_uuid
+ self.name = "#{self.name[0..236]} (#{self.uuid[-15..-1]})"
+ end
+
protected
def self.deep_sort_hash(x)
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 574cd366fc..43797035bc 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -409,7 +409,7 @@ EOS
ensure_unique_name: true
}
assert_response :success
- assert_match /^owned_by_active \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
+ assert_match /^owned_by_active \(#{json_response['uuid'][-15..-1]}\)$/, json_response['name']
end
end
@@ -1285,7 +1285,7 @@ EOS
assert_equal false, json_response['is_trashed']
assert_nil json_response['trash_at']
assert_nil json_response['delete_at']
- assert_match /^same name for trashed and persisted collections \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
+ assert_match /^same name for trashed and persisted collections \(#{json_response['uuid'][-15..-1]}\)$/, json_response['name']
end
test 'cannot show collection in trashed subproject' do
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 d8daa4bdd7..ee7f716c80 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -474,7 +474,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_not_equal(new_project['uuid'],
groups(:aproject).uuid,
"create returned same uuid as existing project")
- assert_match(/^A Project \(\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d\.\d{3}Z\)$/,
+ assert_match(/^A Project \(#{new_project['uuid'][-15..-1]}\)$/,
new_project['name'])
end
@@ -800,7 +800,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
ensure_unique_name: true
}
assert_response :success
- assert_match /^trashed subproject 3 \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
+ assert_match /^trashed subproject 3 \(#{json_response['uuid'][-15..-1]}\)$/, json_response['name']
end
test "move trashed subproject to new owner #{auth}" do
diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb
index 98136aa53b..e6abdc771b 100644
--- a/services/api/test/unit/container_request_test.rb
+++ b/services/api/test/unit/container_request_test.rb
@@ -1136,8 +1136,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
"more than one collection with the same owner and name"
assert output_coll.name.include?(output_name),
"New name should include original name"
- assert_match /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z/, output_coll.name,
- "New name should include ISO8601 date"
+ assert_match /#{output_coll.uuid[-15..-1]}/, output_coll.name,
+ "New name should include last 15 characters of uuid"
end
[[0, :check_output_ttl_0],
--
2.30.2