Make Place.user_id NOT NULL — complete migration period - #3358
Open
Freika wants to merge 4 commits into
Open
Conversation
Removes the `optional: true` guard added during the PR #2494 migration window. The backfill job (BackfillPlacesUserIdJob) enqueued by the May 2026 migration has had time to run; all places should now carry a user_id. Uses the Strong Migrations two-step pattern (check constraint with validate:false → validate → change_column_null) to avoid a full-table lock. Also drops the now-redundant `.where(places: { user_id: user.id })` from Visits::Create#find_existing_place — the NOT NULL column plus belongs_to ownership provides the same guarantee.⚠️ Merge only after confirming zero null user_ids in production: rails runner 'puts Place.where(user_id: nil).count' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsoH1qvvJ6FKzuqNCDXYSG
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bumps schema version to 2026_08_15_100001 and marks places.user_id null: false, reflecting what the two new migrations produce. Without this, CI's db:schema:load leaves both migrations as "pending" and RSpec aborts with ActiveRecord::PendingMigrationError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsoH1qvvJ6FKzuqNCDXYSG
- Add user_id to upsert_all update_attributes in ReverseGeocoding::Places::FetchData to avoid PostgreSQL rejecting the INSERT side of ON CONFLICT DO UPDATE - Simplify backfill job specs to no-op assertions; ownerless places can no longer be created at the DB level - Replace orphan_cleanup_job nil-pass tests (3 tests → 1 no-op test) - Fix full_history_redetect_job_spec: change user:nil to user:create(:user) - Delete backfill_user_id_on_places_spec (migration period is over; update_columns(user_id: nil) now fails the NOT NULL constraint) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsoH1qvvJ6FKzuqNCDXYSG
Visits::Create#find_existing_place was missing the place-level ownership filter, which let user B's new visit attach to user A's place when user B had a prior visit to user A's place at the same coordinates. Restoring .where(user: user) on the Place side prevents this cross-user place leak. fetch_data_spec: build(:place) with FactoryBot 5+ propagates build strategy to associations, leaving user_id nil. Changed to build(:place, user: create(:user)) so insert_all receives a valid user_id. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsoH1qvvJ6FKzuqNCDXYSG
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes out the migration window opened in #2494 (April 2026).
Place.user_idwas left nullable during the 1.7.0 release so the backfill job could run without blocking deploys. The backfill migration (May 2026,BackfillPlacesUserIdJob) has had ~3 months to process; this PR enforces the constraint.Changes:
app/models/place.rb— removesoptional: truefrombelongs_to :user(the comment said "Optional until Stage 2 NOT NULL"; this is Stage 2)db/migrate/20260815100000_add_places_user_id_not_null_check.rb— adds a check constraint withvalidate: false(no table lock; follows the same pattern asAddDatabaseUsersConstraints)db/migrate/20260815100001_validate_places_user_id_not_null.rb— validates the constraint, callschange_column_null :places, :user_id, false, then removes the temporary check constraintapp/services/visits/create.rb— drops the now-redundant.where(places: { user_id: user.id })clause fromfind_existing_place; the column-level NOT NULL plusbelongs_toownership provides the same guaranteeBefore merging, confirm that no rows have a null
user_idin the environment you're deploying to:If the count is non-zero the backfill job is still in progress (or stalled). Do not merge until it's zero —
ValidatePlacesUserIdNotNullwill fail at deploy time otherwise, which rolls back the migration and leaves the schema inconsistent.The check constraint migration (
20260815100000) is safe to run at any time — it only adds a not-yet-validated constraint and takes no lock. The validate migration (20260815100001) is the one that enforces the constraint and will fail fast if any null rows remain.Migration sequence on deploy
Both migrations run inside the normal deploy. No separate rake task needed.
Test plan
Place.where(user_id: nil).countreturns0in staging/production before mergingrails db:migrateagainst a staging database and verify both migrations succeedPlace.new(name: 'X', lonlat: ...).saveraisesActiveRecord::RecordInvalid(user required)Visits::Create.new(user, params).callstill creates/finds places correctlyGenerated by Claude Code