Skip to content

Add matched-instance and version fields to UserRestrictionHistory - #25352

Open
jasonBirchall wants to merge 1 commit into
masterfrom
16408-1-schema
Open

Add matched-instance and version fields to UserRestrictionHistory#25352
jasonBirchall wants to merge 1 commit into
masterfrom
16408-1-schema

Conversation

@jasonBirchall

@jasonBirchall jasonBirchall commented Aug 27, 2026

Copy link
Copy Markdown

Making restrictions more transparent to reviewers (1/4)

Part 1 of 4 for mozilla/addons#16408

When a RestrictionChecker check fails today, we record only the restriction class. Reviewers see "auto-approval disabled because of a restriction" with no way to tell which entry triggered it, and Redash can't tell which restriction entries are doing useful work. This stack records the specific matching rows:

  1. this PR: the schema
  2. per-class match enumeration
  3. checker recording
  4. version link + activity log

The full change is visible as a draft in #25344; I've split it so each PR carries one reviewable question.

This one's is: is this the right schema?

What does this PR change:

  1. UserRestrictionHistory gains restriction_content_type + restriction_object_id, a generic foreign key to the matched restriction row, indexed together.
  2. It also gains a version FK.
  3. All three columns are nullable, on_delete=SET_NULL; one additive migration.
  4. Nothing writes or reads them yet. That will come in later PRs.

Why does it do it this way:

  1. The matched restriction row can live in any of five tables (email pattern, IP network, disposable domain, ASN, JA4 fingerprint), so a generic FK is the only single-column-pair way to point at it. The alternatives were five nullable FKs; four guaranteed NULL on every row, plus a schema change each time a restriction class is added, or a string snapshot, which renders fine but can't be joined, aggregated in Redash, or turned into an admin link (which was requested by the user). The generic FK costs DB-level referential integrity and a django_content_type join; in exchange, the schema never changes when a class is added, and admin links come almost free. The pair is indexed together because the question Redash will ask is "every failure caused by restriction X"; that's the lookup key.
  2. Auto-approval failures are about a specific version, and reviewers need to get from the version under review to the restrictions that fired for it. The checker itself runs inside Version.from_upload() before the version exists (and also serves submission and rating checks that have no version at all), so a later PR backfills this column after creation rather than teaching RestrictionChecker about versions.
  3. Nullable because NULL is state and not missing data. Because we have existing rows that predate these columns, three of the eight restriction checks have no database row to point at (developer agreement, both reputation services), and only auto-approval failures concern a version.
  4. Adding the schema first means the shape gets agreed before any behaviour depends on it. If this PR needs changing, nothing else has to move.

Testing

  • Migration verified via sqlmigrate: three nullable ADD COLUMNs and one CREATE INDEX. Nothing that reads or rewrites existing rows; all operations are auto-reversible.
  • makemigrations users --check --dry-run is clean afterwards.
  • No behaviour to test yet; the columns are exercised by the tests in parts 2–4. Full users and versions test modules pass with the whole stack applied.
  • A lot of this PR comes down to taste.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicate why doing so is unnecessary/impossible.

When a RestrictionChecked check fails we record which restriction class
denied the action, but not which specific restriction row matched. The
matching row is known at the point of failure, then discarded, so
reviewers and Redash can see "an email restriction fired" but never the
reason.

This adds the schema to hold that, without wiring anything up yet.

- it adds two generic foreign keys to the matched restriction row.
Generic FKs because restrictions span five different tables. Indexed
together so "every failure caused by restrictions X" is a cheap query.

- it adds version, tying the auto-approval failure to the version
affected. The checker runs before the version exists, so this will be
backfilled by Version.from_upload() rather than set by the checker.

All new columns are nullable and no existing rows are touched. This is
because there are many records that predate this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant