Skip to content

Set MergeRequestDiff#external_diff_store in more cases

Nick Thomas requested to merge 216216-fix-mr-diffs-external-diffs-store-bug into master

What does this MR do?

Merge request diffs can store their data either in-database, or in an external file. That file may be on disk (recommended against) or in object storage (enabled on GitLab.com today).

We've had a couple of reports where the merge_request_diff.external_diff_store column has had the wrong value for some diffs. The practical outcome of this is that the MR "changes" tab cannot be displayed - the file is correctly stored in object storage, but since we don't record 2 in this column, we don't know to look it up from there.

This MR spots and remedies a couple of deficiencies in when we set this value.

Excluding it during project import is unnecessary, because importing a project actually persists all the MR diffs as in-database ones, that are then migrated to become external storage diffs. This is something we need to improve, so they go straight to object storage, but in the meantime, we can clean this up.

Recalculating every time stored_externally is changed, in addition to external_diff (this is just the filename) changing, seems to be the cleanest way to plug the gap.

On GitLab.com, and presumably on a range of other instances too, we now have records that need fixing - this can be done as a follow-up MR once this one is merged. We shouldn't close the associated issue until this is done.

Since we're trying to roll out external diffs on GitLab.com, per gitlab-com/gl-infra/production#1851 (closed) , we may want to Pick into auto-deploy to ensure this fix reaches GitLab.com sooner.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Part of #216216 (closed)

Closes #197317 (closed)

Edited by Nick Thomas

Merge request reports