Reject notes with more than one sharding key

Chen Zhangrequested to merge
fix/notes-single-sharding-key-validation into master

What does this MR do and why?

Adds a model validation rejecting notes rows that carry more than one sharding key, so such writes fail loudly instead of being silently persisted.

The gap

notes has three candidate sharding keys (namespace_id, project_id, organization_id) and the target invariant is exactly one non-null, tracked in #601451.

Two callbacks already normalize the key:

  • ensure_namespace_id only ever touches namespace_id and project_id.
  • ensure_organization_id returns early unless for_personal_snippet?.

Neither clears organization_id on a non-snippet note. A caller passing organization_id alongside another key therefore persists a multi-key row:

Note.new(noteable: issue, project_id: X, organization_id: Y).save!
# => {namespace_id: nil, project_id: 1, organization_id: 1}   # 2 keys

I probed every combination of the three keys against {noteable present, unresolvable, nil, PersonalSnippet}. The namespace_id + project_id pair is well covered — every shape repaired to a single key. All multi-key survivors involved organization_id.

The repair trigger added in !250291 (merged) does not cover this either: it only fires when namespace_id and project_id are both set, so a project_id + organization_id row is never repaired.

Approach

ExactlyOnePresentValidator across all three keys, matching the existing idiom in WikiPage::Meta.

The cleanup BBM is still draining ~2.33B legacy multi-key rows, so erroring unconditionally would make them unsaveable. The validation is skipped when a persisted record does not touch any sharding key. Intent is captured in a before_validation callback ordered ahead of ensure_namespace_id, because that callback dirties the sharding keys itself and would otherwise mask whether the caller assigned them.

Known limitation: assigning a sharding key its existing value is not a change, so it stays on the repair path. That write is a no-op, so this is acceptable.

Behaviour

Write Result
project_id + organization_id rejected
namespace_id + organization_id rejected
all three keys rejected
project-keyed note accepted
namespace_id + project_id (callback repairs) accepted
organization-keyed personal snippet accepted
legacy multi-key row, unrelated save accepted
legacy multi-key row, caller assigns a key rejected
legacy multi-key row, caller reduces to one key accepted

How to set up and validate locally

issue = Issue.first
org   = Organizations::Organization.first

note = Note.new(noteable: issue, author: User.first, note: 'x',
                project_id: issue.project_id, organization_id: org.id)
note.valid?
# => false
note.errors.full_messages
# => ["must have exactly one of namespace_id, project_id, organization_id"]

Testing

  • spec/models/note_spec.rb — 8 new examples covering the matrix above; full file green (340 examples).
  • spec/models/diff_note_spec.rb, discussion_note_spec.rb, legacy_diff_note_spec.rb green (119 examples), since the validation is inherited.
  • RuboCop clean.

Related

Merge request reports