Skip to content

Add V161: case-insensitive phoenix_kit_users.username via citext - #681

Merged
ddon merged 4 commits into
BeamLabEU:mainfrom
timujinne:fix/case-insensitive-username
Aug 5, 2026
Merged

Add V161: case-insensitive phoenix_kit_users.username via citext#681
ddon merged 4 commits into
BeamLabEU:mainfrom
timujinne:fix/case-insensitive-username

Conversation

@timujinne

Copy link
Copy Markdown
Contributor

Why

Two accounts existed in production with the usernames Pavel and pavel. Nobody created a duplicate on purpose — the system allowed it end to end:

  • phoenix_kit_users_username_uidx is a plain btree on a varchar column, so the two values are distinct keys.
  • unsafe_validate_unique(:username, …) compares exactly.
  • get_user_by_username/1 is Repo.get_by(User, username: …) — also exact.

The mechanism that actually produced the duplicate is worth spelling out, because it is systematic rather than a fluke: generate_username_from_email/1force-downcases, while ensure_unique_username/3 checks availability case-sensitively. A username set manually with a capital is therefore invisible to the generator — it proposes the lowercase form, finds it "free", and adds no suffix.

The consequence worse than the duplicate itself: signing in by username is case-sensitive too, so typing your own name with different capitalisation lands you in a different account. To the user that reads as "my permissions disappeared", with no error anywhere.

email is unaffected — it has been citext since V01 and is deliberately left alone.

What

V161 converts phoenix_kit_users.username to citext.

Postgres decides comparison semantics from the column type, so one ALTER fixes uniqueness and every lookup, including a bare Repo.get_by. A functional unique index on lower(username) would have fixed only the constraint and left every read exact-match — and nothing would stop the next lookup from forgetting about case again. This is also the pattern already used in this schema (email in V01, CRM columns in V151), not a new one.

  • Pre-check before any DDL: GROUP BY lower(username) HAVING count(*) > 1, raising and naming the offending value (same shape as V106's). WHERE username IS NOT NULL matters — without it GROUP BY folds every NULL into one group and two username-less accounts abort the upgrade.
  • Idempotent ALTER guarded on udt_name, prefix honored, @disable_ddl_transaction true.
  • down/1 converts back to varchar(255).

Verification

Run against a real PostgreSQL 17.4, empty database, so the whole chain executed from V01 to V161:

  • usernamecitext, email untouched, schema version comment 161
  • phoenix_kit_users_username_uidx survived the ALTER with the same name and predicate
  • inserting 'ALICE' against an existing 'alice' is rejected by the index
  • WHERE username = 'BOB' finds the row stored as 'bob' — the exact SQL shape Repo.get_by emits
  • varchar → citext confirmed binary-coercible against pg_cast (castmethod = 'b'), so no table rewrite; the index rebuild was confirmed on a scratch table (relfilenode changes), which is what makes the constraint start rejecting case variants

Full suite with a database: 3 failures, all in MaintenanceTest (scheduling windows on an unseeded database), which V161 does not touch. mix precommit green.

Also here

Verifying the above was harder than it should be, so two adjacent fixes ride along:

  • config/test.exs hardcoded phoenix_kit_test, which forces a role with CREATEDB — precisely what a shared or managed instance withholds. PGDATABASE/PGPOOL now let you point the suite at a database you already have. Defaults unchanged.
  • AGENTS.md described mix precommit as the local equivalent of CI "including tests with PostgreSQL". It never ran mix test, and CI is workflow_dispatch, so in practice nothing ran the Elixir suite. Adding mix test to precommit was tried and reverted — the suite is not green from a clean checkout (with no database ~5 "unit" tests fail because Settings reads hit the DB on a cache miss), and a permanently red gate teaches people to ignore the gate. The docs now say running the suite is manual, and warn that mix test with no database excludes every integration test and still reports success — the failure mode that matters most when the change is a migration.

Compatibility

An installation that already contains case-variant usernames stops at V160 with a raise naming the conflict, resolves it, and re-runs — the same operator experience V106 already establishes. Nothing is merged or renamed automatically. Installations without collisions are unaffected. citext has been a required extension since V01, so there is no new database prerequisite.

The behavioural change is the fix itself: username lookup and uniqueness become case-insensitive. registration_test.exs had a test pinning the old exact-match behaviour as intended; it is inverted here.

username has been VARCHAR(255) since V08, so comparison was always
exact-match at every layer — bare Repo.get_by, unsafe_validate_unique,
and the unique index itself — despite the schema declaring the field
as a string. Two accounts differing only by case (alice / Alice) could
both register, and get_user_by_username("ALICE") would find neither.
Converts the column to citext, the same fix already applied to email
in V01 and the CRM party email columns in V151: Postgres resolves
comparison semantics from the column type, so one ALTER fixes writes,
reads, and the uniqueness constraint at once.
up/1 pre-checks for existing case-insensitive collisions before any
DDL (mirrors V106's down-step pre-check), raising with the offending
value if found. WHERE username IS NOT NULL guards against nullable
rows being folded into one GROUP BY bucket and reported as a false
collision.
varchar -> citext is binary-coercible (pg_cast.castmethod = 'b',
confirmed live), so the table heap is not rewritten. Also verified
live: the column's B-tree index IS rebuilt by the ALTER (relfilenode
changes) — expected, since citext orders values by lower() rather than
raw bytes, and it's what makes the existing unique index start
rejecting case-variant duplicates immediately after conversion.
@disable_ddl_transaction avoids holding the ACCESS EXCLUSIVE lock for
the length of a migration transaction.
email is untouched (already citext, already correct).
Review pointed out three places where the documentation promised more
than the code delivers.
The pre-check is not atomic with the ALTER — @disable_ddl_transaction
puts them in separate statements, so a live system can still slip a
colliding pair into that window and get Postgres' generic duplicate-key
error instead of the readable message. The real guard is the index
rebuild inside the ALTER. Closing the window would mean holding an
ACCESS EXCLUSIVE lock across both, which is the thing the migration is
built to avoid, so the gap stays — it just says so now.
The pre-check test copies the migration's SELECT rather than calling it,
because once the column is citext a real collision cannot be staged. The
comment claimed a broken migration would break the helper too; it
wouldn't. It now says the two are synced by hand and names what the test
does buy — chiefly the IS NOT NULL guard, without which every
username-less account folds into one false collision.
Cost section counted the index rebuild but not the pre-check's own full
scan of every non-null username.
Verifying V161 meant running the suite, and running the suite turned out
to be the hard part: the test repo hardcoded phoenix_kit_test, so the
only way in was a role with CREATEDB — exactly what a shared or managed
Postgres withholds. PGDATABASE and PGPOOL now let you point at a database
you already have. Defaults are unchanged.
While confirming how the checks run, the docs turned out to be wrong
about it. They described mix precommit as the local equivalent of CI
'including tests with PostgreSQL'. Precommit never ran mix test, and CI
is workflow_dispatch, so in practice nothing ran the Elixir suite unless
someone remembered to.
Adding mix test to precommit was the obvious fix and it does not work:
the suite is not green from a clean checkout, because Settings reads hit
the database on a cache miss and take about five 'unit' tests down with
no database present. A permanently red gate teaches people to ignore the
gate. So precommit keeps its current scope, the docs now say plainly
that running the suite is manual, and they warn that a mix test with no
database excludes every integration test and still reports success --
which is the failure mode that matters when the change is a migration.
Also replaced a gh pr create example that hardcoded one contributor's
fork and branch.
@timujinne
timujinneforce-pushed the fix/case-insensitive-username branch from 9fb3ded to e167069CompareAugust 5, 2026 04:23

@timujinnetimujinne left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Reviewed by an independent model (GLM-5.2, max effort) against the code, with every finding
re-verified by hand before being accepted — a review posted to a PR costs maintainer time when it
is wrong. Full write-up committed to
dev_docs/pull_requests/2026/681-case-insensitive-username/CLAUDE_REVIEW.md.

Verdict: APPROVE. Two findings, both now fixed in this branch.

IMPROVEMENT - MEDIUM — a set-but-empty env var aborted config loading

System.get_env/2 falls back to its default only when the variable is unset. A set-but-empty
PGPOOL= returns "", so String.to_integer("") raised an ArgumentError while config was
loading — before any test ran, with a message that never named PGPOOL. Reachable by
PGPOOL= mix test, or by PGPOOL: with no value in a YAML pipeline.

Fixed: PGDATABASE and PGPOOL now treat empty as absent and trim whitespace, and a non-numeric
PGPOOL raises a message naming the variable and the value. Checked across unset / set-empty /
valid / whitespace-padded.

NITPICK — internal chronology in upstream-facing docs

Comments read "tried on 2026-08-04 and reverted". A bare date without an actor reads as one team's
internal history in a public library. Dates dropped, reasoning kept.

Self-correction

This PR's own text claimed mix test with no database "silently" excludes integration tests. It is
not silent — test_helper.exs prints a warning banner. The point that matters survives: the run
still exits 0 and reports success, so a green summary proves nothing about a migration. Wording
corrected here and in AGENTS.md, since fixing inaccurate documentation with inaccurate
documentation would be a poor trade.

Checked and found correct

  • Dispatch is Module.concat([__MODULE__, "V#{pad_idx}"]) by version number plus the
    @current_version bump — no registry to forget.
  • The pre-check cannot false-positive: WHERE username IS NOT NULL is load-bearing, since
    GROUP BY otherwise folds every NULL into one group and two username-less accounts would abort
    the upgrade. Pinned by test.
  • Re-running against an installation already at 161 is a no-op — coordinator skips, the DO block
    is guarded on udt_name, and the pre-check returns empty on a citext column.
  • Prefix-safety: both existence checks anchor on table_schema, no ::regclass in an IMMEDIATE
    check, escaped_prefix escaped at the entry point. Same shape as V151.
  • get_user_by_username/1, unsafe_validate_unique/3 and ensure_unique_username/3 all ride the
    column's semantics, so no call-site changes are needed.
  • No internal infrastructure leaked into the diff.

On the evidence behind the migration

The claims in the description are from an actual run, not inference: PostgreSQL 17.4, empty
database, whole chain V01→V161. Column became citext, the unique index survived the ALTER with
the same name and predicate, 'ALICE' against an existing 'alice' was rejected, and
WHERE username = 'BOB' found 'bob' — the exact SQL shape Repo.get_by emits. Full suite on
that database: 3 failures, all in MaintenanceTest (scheduling windows on an unseeded database),
which V161 does not touch.

Review of the published PR found that System.get_env/2 falls back to its
default only when the variable is unset. A set-but-empty PGPOOL= returns
an empty string, so String.to_integer/1 raised while config was still
loading — before any test ran, with a message that never named PGPOOL.
It is easy to reach: PGPOOL= from a shell, or PGPOOL: with no value in a
YAML pipeline. Both variables now treat empty as absent and trim
whitespace, and a non-numeric PGPOOL raises something that says which
variable and which value.
Comments carried bare dates for when an approach was tried and reverted.
In a published library that reads as one team's internal history, so the
dates are gone and the reasoning stays.
Also corrected this branch's own wording: it claimed mix test with no
database excludes integration tests silently. It is not silent, the test
helper prints a banner. What matters is unchanged and is now what the
docs say — the run still exits 0 and reports success, so a green summary
proves nothing about a migration.
The review itself is committed under dev_docs/pull_requests, per the
convention in AGENTS.md.
@ddon
ddon merged commit 62d557d into BeamLabEU:mainAug 5, 2026
ddon pushed a commit that referenced this pull request Aug 5, 2026
Reviews the UrlState / V161-citext / V162-payment-option wave merged on
main, and fixes what it turned up.
Fix: `get_user_by_email_or_username_and_password/3` hand-rolled its case
folding as `fragment("LOWER(?)", u.username)`, which matches no index in
the chain. V161's whole premise is that comparison semantics come from
the column type, so with `username` now `citext` plain equality is both
correct and index-backed via `phoenix_kit_users_username_uidx`. This was
the only username lookup still sequentially scanning the users table, on
the one endpoint reachable without authenticating.
Fix: the `PhoenixKitUrlState` JS hook registered `handleEvent` on the
LiveSocket but only removed its `popstate` listener in `destroyed()`, so
each remount left another live callback behind.
Add: `test/phoenix_kit/migrations/v162_test.exs`. V162 shipped with no
test. Pins `ON DELETE SET NULL` in particular — that is the migration's
whole design decision, and a later refactor reaching for a plain
`references/2` would silently make it `RESTRICT` with nothing failing.
Also renames the V162 PR-draft doc out of `680-v161-…`, which named the
pre-renumber identity and collided with PR #680's own directory.
Full findings, and the two recorded-but-unfixed gaps, in
dev_docs/pull_requests/2026/680-682-post-merge-review/CLAUDE_REVIEW.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timujinne
timujinne deleted the fix/case-insensitive-username branch August 6, 2026 05:55
ddon pushed a commit that referenced this pull request Aug 9, 2026
The 161 slot is taken upstream by two open PRs (#681 our own citext-username
branch, #682 mdon's payment-option linkage), so this repair migration moves to
the first free slot when they land. Records why it cannot be renumbered
pre-emptively (contiguity: a hole makes change(1..N) crash on the missing
module — release_check and its test both catch it, verified empirically) and
the exact renumber ritual incl. manifest regeneration.
ddon pushed a commit that referenced this pull request Aug 9, 2026
Upstream took 161 (our own citext-username PR #681) and 162 (payment-option
linkage #682, itself renumbered off 161), so the flush-bug repair migration is
now V163: file+module, self-stamp '163', down restamp '162',
@current_version 163, moduledoc LATEST marker, and the guard test renamed with
@exempt_version 163. Spec counters: 163 files / 28,000 lines, floor-candidate
remainders recalculated (deleted-line figures unchanged — v01..v147 have never
been touched). The generated manifest is now STALE (its chain_hash pins the
pre-merge file set) and must be regenerated before it is trusted.
Sign up for freeto 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.

2 participants

@timujinne@ddon