Stop writing both namespace_id and project_id on project-scoped notes

Summary

Prerequisite for #569520 and the eventual tightening of the multi-column NOT NULL constraint on notes from num_nonnulls(namespace_id, project_id, organization_id) >= 1 to = 1.

Currently, every insert and update of a project-scoped note (MR comment, commit note, etc.) populates both project_id and namespace_id. A cleanup BBM (!238033 (merged)) cannot reach a stable single-key state until the application stops dual-writing — otherwise rows cleaned by the BBM get re-populated on the next save.

This was surfaced during review of !238033 (merged) by @mcelicalderonG, and aligns with Phase 3 of #569520 which lists "Update model validations to prevent future dual population".

Root cause

app/models/note.rb#L824-833:

before_validation :ensure_namespace_id, :ensure_organization_id, ...

def ensure_namespace_id
  return if namespace_id.present? && !noteable_changed? && !project_changed?

  self.namespace_id = if for_issue?
                        noteable&.namespace_id
                      elsif for_project_noteable?
                        project&.project_namespace_id   # ← sets namespace_id even though project_id is already set
                      end
end

For for_project_noteable? (MR / commit / project-snippet / diff / etc. — i.e., !(for_personal_snippet? || for_abuse_report? || group_level_issue?)):

  • The caller sets project_id from the noteable.
  • This callback then sets namespace_id = project.project_namespace_id without clearing project_id.
  • The callback runs on both create and update (no on: scope), so any subsequent save of an already-cleaned row repopulates namespace_id.

This matches the ~78% dual-populated rate reported in !238033 (merged) — steady-state behavior, not legacy residue.

Proposed change

In Note#ensure_namespace_id, skip the namespace_id assignment for for_project_noteable?. project_id is authoritative for project-scoped notes (per !238033 (merged) description), and the >= 1 constraint already permits project_id-only rows.

  • Keep for_issue? branch (group-level issues legitimately use namespace_id only).
  • Personal snippets continue to be covered by ensure_organization_id.

Specs to update

spec/models/note_spec.rb#L476-497 currently asserts note.namespace_id == project.project_namespace_id for a project noteable. That expectation must flip to namespace_id.nil? (with project_id set), for both the create case and the "noteable changed" case.

Add a regression spec asserting that for a project-noteable note, project_id is set and namespace_id stays nil after both create and update (the update case is the one that re-populates today).

Acceptance criteria

  • Note#ensure_namespace_id does not set namespace_id for for_project_noteable?.
  • Existing specs in spec/models/note_spec.rb updated to assert single-key state.
  • New regression spec covers the update path (save → save again → still single-key).
  • No regressions in noteable change scenarios (e.g. moving a note between MRs).
  • Verified that DB triggers cascading from notes (e.g. sync_sharding_key_with_notes_table on note_metadata, diff_note_positions, suggestions, note_diff_files) still resolve correctly when notes.namespace_id is NULL — they already fall back to deriving namespace_id from projects.project_namespace_id via notes.project_id, but worth a spec.

Sequencing

This issue must be deployed and rolled out before:

  1. !238033 (merged) — the cleanup BBM (otherwise the BBM is fighting the app).
  2. The follow-up constraint-tightening MR (>= 1 → = 1).

Blocks #569520.

Edited by 🤖 GitLab Bot 🤖