Skip to content

Ignore FactoryBot #create for cross-database modification detection

Dylan Griffith requested to merge 339816-dont-detect-issues-in-factories into master

What does this MR do and why?

Our PreventCrossDatabaseModification detection logic is designed to find places in our code where we are writing to 2 different databases in the context of a transaction of one of those databases.

For example:

# BAD
Project.transaction do
  Project.create!
  Ci::Runner.create! # It's a different database so not in the transaction and confusing/risky
end
# GOOD
Project.transaction do
  Project.create!
end

Ci::Runner.create! # Less confusing and therefore less risky but you need to decide how/if you need rollback semantics

This was introduced because we are decomposing our database into separate databases. Specifically we are moving all CI tables to a separate CI database. It's important that we find and fix any places in our code where we are opening a transaction and expecting all transaction semantics (ie. rollbacks) to apply to all database queries within that context. When you have 2 databases there may be places where we are only rolling back some of the queries in that transaction which could lead to data inconsistency bugs. As such we need to detect and restructure such transactions so that each case is properly handled.

This was causing too many false positives which are not representative of code that is actually running in production. It is very common in tests to build a tree of related objects in FactoryBot and save them all in one go with save! (this is the default behaviour in FactoryBot for association building). When this is done it creates an outer transaction and saves all the models within the context of this transaction. If 2 of the models belong in a different gitlab_schema (database) then this triggers our cross-database modification detection logic. While there is a chance that this could be indicative of problems in our model code (eg. before_save and so on) it is more often than not just a problem with how our factories are building a relation tree.

A simple example is included as an RSpec test here where we create a ci_runner and project (which in turn creates a user) with create(:ci_runner, :project, projects: [build(:project)]) all in one go. This is convenient in tests but it is never going to happen in real production code as there is no way to create a project at the same time as creating a runner (let alone user). You would always have a project and a user and then later create the runner for that project.

I also considered changing our factories to avoid doing these types of object initialization in !72742 (closed) but this didn't seem like a good direction to go in. Firstly we have a Rubocop rule telling us not to do this. Secondly it would require changes to a lot of factories and thirdly it would mean we can't just build(... some objects without causing DB writes for creates for the associations which might mean spec performance regressions as there is unnecessary DB writes.

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #339816 (closed)

Edited by Dylan Griffith

Merge request reports

Loading