fix: join through player_teams instead of non-existent players.team_id

forgejo_admin commented

2026-03-27 20:55:23 +00:00

Author

Owner

PR #26 Review

DOMAIN REVIEW

Tech stack: SvelteKit (adapter-node), TypeScript, raw SQL via pg Pool, direct Postgres queries.

SQL correctness:
The fix correctly replaces LEFT JOIN teams t ON t.id = p.team_id with the two-hop join through player_teams:

LEFT JOIN player_teams pt ON pt.player_id = p.id
LEFT JOIN teams t ON t.id = pt.team_id

This matches the basketball-api migration 019 which dropped players.team_id in favor of the player_teams junction table (composite PK on player_id, team_id).

Both changed files are correct:

+page.server.ts: The SELECT p.* no longer includes team_id (column dropped), but the return object only uses team_name from the teams join -- correct.
+server.ts: Explicitly selects pt.team_id which maps to player.team_id for the downstream team lookup (line 58-60) and outbox payload (line 100) -- correct.

Many-to-many row duplication concern (non-blocking):
The player_teams table is many-to-many (composite PK). If a player belongs to multiple teams, these LEFT JOINs will produce multiple rows and rows[0] will silently pick an arbitrary team. This is a pre-existing architectural concern (the old p.team_id was 1:1), but the new join surface makes it possible. In practice, westside players are assigned to one team, so the risk is low. A LIMIT 1 or DISTINCT ON would be defensive but is out of scope for this bug fix.

TypeScript types:
Player interface (src/lib/types.ts line 5) still declares team_id: number | null. This remains correct since pt.team_id is aliased as team_id in the sign endpoint query result.

BLOCKERS

None.

This is a 2-file, 6-line fix to a cross-repo regression (basketball-api migration 019 broke westside-contracts queries). The SQL changes are minimal, correct, and tightly scoped. No new functionality is introduced -- this restores existing functionality that broke due to a schema migration in another repo.

On test coverage: The repo has 12 existing tests in tests/validation.test.ts covering input validation. This PR changes raw SQL join clauses, which cannot be meaningfully unit-tested without a database. The Test Plan in the PR body specifies manual integration verification (load contract page, POST sign, verify outbox payload). For a hotfix restoring broken production functionality, this is acceptable -- there is no new feature surface to cover.

NITS

Defensive query for multi-team players (+page.server.ts:15-16, +server.ts:13-14): Consider adding LIMIT 1 to both queries or using DISTINCT ON (p.id) to guard against future multi-team assignments returning duplicate rows. Not required for this fix, but worth a follow-up ticket.
Player interface still has team_id (src/lib/types.ts:5): The team_id field on the Player interface technically no longer lives on the players table -- it comes from the join. This is semantically misleading but functionally correct since the query aliases pt.team_id to team_id. Consider a comment or renaming in a future cleanup.

SOP COMPLIANCE

Branch named after issue: 25-bug-queries-reference-players-team-id-bu references issue #25
PR body has Summary, Changes, Test Plan, Related sections
Related references project (project-westside-basketball) -- no plan slug required (bug fix, not plan work)
No secrets committed
No unnecessary file changes -- exactly 2 files changed, both contain the broken query
Commit messages are descriptive (PR title: fix: join through player_teams instead of non-existent players.team_id)
Closes #25 links PR to parent issue

PROCESS OBSERVATIONS

Cross-repo regression risk: basketball-api migration 019 (Mar 25) dropped players.team_id without updating westside-contracts, which was scaffolded Mar 24 when the column still existed. This is a classic cross-repo schema coupling problem. Consider adding a "downstream consumers" checklist to migration PRs in basketball-api, or a CI smoke test that validates westside-contracts queries against the current schema.
Change failure risk: Low. The fix is mechanical (join path change) with no logic changes. The old code was unconditionally broken (500 on every contract page), so the fix can only improve the situation.
MTTR: This is the fix itself -- fast turnaround from breakage to PR.

VERDICT: APPROVED

## PR #26 Review ### DOMAIN REVIEW **Tech stack:** SvelteKit (adapter-node), TypeScript, raw SQL via `pg` Pool, direct Postgres queries. **SQL correctness:** The fix correctly replaces `LEFT JOIN teams t ON t.id = p.team_id` with the two-hop join through `player_teams`: ``` LEFT JOIN player_teams pt ON pt.player_id = p.id LEFT JOIN teams t ON t.id = pt.team_id ``` This matches the basketball-api migration 019 which dropped `players.team_id` in favor of the `player_teams` junction table (composite PK on `player_id, team_id`). Both changed files are correct: - `+page.server.ts`: The `SELECT p.*` no longer includes `team_id` (column dropped), but the return object only uses `team_name` from the teams join -- correct. - `+server.ts`: Explicitly selects `pt.team_id` which maps to `player.team_id` for the downstream team lookup (line 58-60) and outbox payload (line 100) -- correct. **Many-to-many row duplication concern (non-blocking):** The `player_teams` table is many-to-many (composite PK). If a player belongs to multiple teams, these LEFT JOINs will produce multiple rows and `rows[0]` will silently pick an arbitrary team. This is a pre-existing architectural concern (the old `p.team_id` was 1:1), but the new join surface makes it possible. In practice, westside players are assigned to one team, so the risk is low. A `LIMIT 1` or `DISTINCT ON` would be defensive but is out of scope for this bug fix. **TypeScript types:** `Player` interface (`src/lib/types.ts` line 5) still declares `team_id: number | null`. This remains correct since `pt.team_id` is aliased as `team_id` in the sign endpoint query result. ### BLOCKERS None. This is a 2-file, 6-line fix to a cross-repo regression (basketball-api migration 019 broke westside-contracts queries). The SQL changes are minimal, correct, and tightly scoped. No new functionality is introduced -- this restores existing functionality that broke due to a schema migration in another repo. **On test coverage:** The repo has 12 existing tests in `tests/validation.test.ts` covering input validation. This PR changes raw SQL join clauses, which cannot be meaningfully unit-tested without a database. The Test Plan in the PR body specifies manual integration verification (load contract page, POST sign, verify outbox payload). For a hotfix restoring broken production functionality, this is acceptable -- there is no new feature surface to cover. ### NITS 1. **Defensive query for multi-team players** (`+page.server.ts:15-16`, `+server.ts:13-14`): Consider adding `LIMIT 1` to both queries or using `DISTINCT ON (p.id)` to guard against future multi-team assignments returning duplicate rows. Not required for this fix, but worth a follow-up ticket. 2. **`Player` interface still has `team_id`** (`src/lib/types.ts:5`): The `team_id` field on the `Player` interface technically no longer lives on the `players` table -- it comes from the join. This is semantically misleading but functionally correct since the query aliases `pt.team_id` to `team_id`. Consider a comment or renaming in a future cleanup. ### SOP COMPLIANCE - [x] Branch named after issue: `25-bug-queries-reference-players-team-id-bu` references issue #25 - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references project (`project-westside-basketball`) -- no plan slug required (bug fix, not plan work) - [x] No secrets committed - [x] No unnecessary file changes -- exactly 2 files changed, both contain the broken query - [x] Commit messages are descriptive (PR title: `fix: join through player_teams instead of non-existent players.team_id`) - [x] `Closes #25` links PR to parent issue ### PROCESS OBSERVATIONS - **Cross-repo regression risk:** basketball-api migration 019 (Mar 25) dropped `players.team_id` without updating westside-contracts, which was scaffolded Mar 24 when the column still existed. This is a classic cross-repo schema coupling problem. Consider adding a "downstream consumers" checklist to migration PRs in basketball-api, or a CI smoke test that validates westside-contracts queries against the current schema. - **Change failure risk:** Low. The fix is mechanical (join path change) with no logic changes. The old code was unconditionally broken (500 on every contract page), so the fix can only improve the situation. - **MTTR:** This is the fix itself -- fast turnaround from breakage to PR. ### VERDICT: APPROVED

2026-03-27 20:53:15 +00:00

Summary

All contract pages returning HTTP 500: column p.team_id does not exist
basketball-api migration 019 dropped players.team_id in favor of player_teams join table (Mar 25)
westside-contracts was scaffolded Mar 24 when the column still existed — cross-repo regression

Changes

src/routes/contract/[token]/+page.server.ts: join through player_teams pt then teams t ON t.id = pt.team_id
src/routes/contract/[token]/sign/+server.ts: same join pattern, aliased columns with p. and pt. prefixes

Test Plan

/contract/{token} loads without 500
/contract/{token}/sign POST succeeds
Outbox event payload still includes team_id
No regressions in contract display or signing flow

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

forgejo_admin/westside-contracts #25 — the Forgejo issue this PR fixes
project-westside-basketball — the project this work belongs to

Closes #25

## Summary - All contract pages returning HTTP 500: `column p.team_id does not exist` - basketball-api migration 019 dropped `players.team_id` in favor of `player_teams` join table (Mar 25) - westside-contracts was scaffolded Mar 24 when the column still existed — cross-repo regression ## Changes - `src/routes/contract/[token]/+page.server.ts`: join through `player_teams pt` then `teams t ON t.id = pt.team_id` - `src/routes/contract/[token]/sign/+server.ts`: same join pattern, aliased columns with `p.` and `pt.` prefixes ## Test Plan - [ ] `/contract/{token}` loads without 500 - [ ] `/contract/{token}/sign` POST succeeds - [ ] Outbox event payload still includes `team_id` - [ ] No regressions in contract display or signing flow ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - `forgejo_admin/westside-contracts #25` — the Forgejo issue this PR fixes - `project-westside-basketball` — the project this work belongs to Closes #25

forgejo_admin added 1 commit

fix: join through player_teams instead of non-existent players.team_id 14bfc0a15c

Closes #25

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin merged commit 89439ee1db into main

2026-03-27 21:01:24 +00:00

forgejo_admin deleted branch 25-bug-queries-reference-players-team-id-bu

forgejo_admin referenced this pull request from a commit

2026-03-27 21:01:25 +00:00

fix: join through player_teams instead of non-existent players.team_id (#26)

Rows
Columns

fix: join through player_teams instead of non-existent players.team_id #26