Namespace factory is problematic
Problem
Whenever we use create(:namespace)
, we ended up with two namespaces pointing to the same user, but the user would only point to one namespace, picking in a somehow random manner (decided by database)
Background
This is a follow up from https://gitlab.com/gitlab-org/gitlab-ee/issues/4858
The factory itself is fine, but we have this code in the user:
class User < ActiveRecord::Base
before_validation :ensure_namespace_correct
def ensure_namespace_correct
if namespace
namespace.path = namespace.name = username if username_changed?
else
build_namespace(path: username, name: username)
end
end
end
Which itself would create a namespace if there's no namespace given before saving (actually, validating). But if we look at the namespace model:
class Namespace < ActiveRecord::Base
validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
end
This also requires a user to be given. Due to those, we have problematic namespace factory:
FactoryBot.define do
factory :namespace do
sequence(:name) { |n| "namespace#{n}" }
path { name.downcase.gsub(/\s/, '_') }
owner
end
end
Whenever we use create(:namespace)
in the test, it would create an owner, which is a user, which would then implicitly create another namespace, and we ended up with two namespaces pointing to the same user. This would somehow look like this:
user = User.new
user.namespace = Namespace.new
namespace = Namespace.new
namespace.owner = user
When this happened, we could get:
namespace.owner == user # true
user.namespace.owner == user # true
namespace == user.namespace # Sometimes TRUE and sometimes FALSE!
In other words, whenever we use user.namespace
we don't know which namespace it would give to us, because the database would decide that for us. It's kind of random, but not completely random so it could be very hard to reproduce this.
Solution
- Remove namespace factory, and always use
create(:user).namespace
if we want a namespace. This is basically what https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/4453 did, and probably also what I did in the past - Rewrite
User#ensure_namespace_correct
and break the circular dependency between the user and namespace
The former one would be a short-term solution, and we'll need to port those traits to the user factory. The latter one would be the ideal long-term solution, but we'll need to make sure we don't break any existing code.
In addition, we should add an unique index to avoid this from happening again:
add_index :namespaces, :owner_id, unique: true
This is mentioned in https://gitlab.com/gitlab-org/gitlab-ce/issues/42936#note_58424428