Refactor User#ensure_namespace_correct
Background
This was brought up by https://gitlab.com/gitlab-org/gitlab-ee/-/jobs/32313185 which was fixed in https://gitlab.com/gitlab-org/gitlab-ee/commit/75eb32b70064b00d914bd4823803a70ceac992fc during the process of merging CE to EE.
The test was testing against data on a particular namespace, but this won't work well:
let(:user) { create(:user) }
let(:namespace) { create(:namespace, owner: user) }
This would actually end up with two namespaces belonging to the user, and then user.namespace
could be returning the namespace we're testing against (in the case of PostgreSQL), or the one which was implicitly created by User#ensure_namespace_correct
(in the case of MySQL).
This could be considered completely random because there's no ORDER BY
for has_one :namespace
association, and the database is free to give whichever namespace there.
In the end, I fixed it by just ignore User#ensure_namespace_correct:
before do
# This would make sure that the user won't try to create another namespace
allow_any_instance_of(User).to receive(:ensure_namespace_correct)
end
Best solution
We should not create anything implicitly via after_save
or before_save
. We could maybe just remove that offending line and fix the failing tests.
Why alternative solutions don't work well
For most of the tests, this could be tweaked to: (and I have done this a few times IIRC)
let(:user) { create(:user) }
let(:namespace) { user.namespace }
In this form, we would just use the namespace created implicitly from User#ensure_namespace_correct.
However this is not nice. In this particular failing test, we're using traits from namespace factory. In this form, basically we don't use namespace factory therefore we cannot use those traits. It does not work if we reverse it:
# This doesn't work:
let(:user) { namespace.owner }
let(:namespace) { create(:namespace) }
This does not work because for the namespace factory, it's simply creating a user and assigning to it. It would read like namespace.owner = create(:user)
, and when we hit create(:user)
, the other namespace would already be created. We also cannot create the namespace first, then assign it to the user, because in order to create a namespace, we need to have an owner first. This was enforced by the namespace validators.
We need to break the mutual dependency. Namespace owner validation is fine. Implicitly creating a namespace is not.
/cc @jarka