refactor: make normalizeColumns table dependent

[TylerHillery]

What kind of change does this PR introduce?

Refactor to make normalizeColumns() table dependent and consolidate all new column checking to use this function so we can have all rules defined in one spot.

What is the current behavior?

Currently all rules apply to all callers of normalizeColumns even though the new columns added are tied to certain tables. This has caused other normalize column like behavior elsewhere for specific functions.

What is the new behavior?

• Made a COLUMN_MIGRATION_RULES object that defines all rules per table. Callers of normalizeColumns() now must pass in one of the keys of this object which corresponds to the table name.
• Old loop returned as soon as it hit the first rule whose migration hadn't run, so only
  one rule ever actually got applied. New version collects every pending rule for the table
  and excludes all their columns. This matters now since s3_multipart_uploads has two rules
  (custom-metadata -> user_metadata, s3-multipart-uploads-metadata -> metadata).
• Folded buckets' type filtering and s3_multipart_uploads' metadata filtering into the same
  rules table too, since they were doing the exact same thing by hand elsewhere. Deleted
  hasMultipartMetadataColumn() and normalizeMultipartUploadColumns() since they're not needed
  anymore.
• Made normalizeColumns() overloaded instead of generic so we could drop the as T casts.

[coveralls]

Coverage Report for CI Build 30860045086

Coverage increased (+0.06%) to 80.555%

Details

• Coverage increased (+0.06%) from the base build.
• Patch coverage: 1 uncovered change across 1 file (21 of 22 lines covered, 95.45%).
• 4 coverage regressions across 2 files.

Uncovered Changes

File	Changed	Covered	%
src/storage/database/pg.ts	22	21	95.45%

Coverage Regressions

4 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
src/internal/database/migrations/migrate.ts	3	69.67%
src/storage/database/pg.ts	1	89.25%

---

Coverage Stats

Relevant Lines:	12631
Covered Lines:	10601
Line Coverage:	83.93%
Relevant Branches:	7487
Covered Branches:	5605
Branch Coverage:	74.86%
Branches in Coverage %:	Yes
Coverage Strength:	437.48 hits per line

---

💛 - Coveralls

[TylerHillery]

One thing I noticed is if lastestMigration is undefined we just assume migrations have ran versus hasMigrations() specifically calls tenantHasMigrations which calls getTenantConfig() to get the tenant's latest migration. Should we be doing that here? I left as is for now.

  async hasMigration(migration: keyof typeof DBMigration): Promise<boolean> {
    if (this.latestMigration !== undefined) {
      return DBMigration[this.latestMigration] >= DBMigration[migration]
    }

    return tenantHasMigrations(this.tenantId, migration)
  }

[Copilot]

Pull request overview

Refactors StoragePgDB.normalizeColumns() to be table-scoped via a centralized COLUMN_MIGRATION_RULES mapping, so column gating for not-yet-run migrations is defined in one place and consistently applied across callers.

Changes:

• Introduces COLUMN_MIGRATION_RULES + ColumnRuleTable, and updates normalizeColumns() to take a table name and apply all pending rules for that table.
• Updates bucket/object/multipart-upload query + insert/update paths to use table-scoped normalizeColumns() and removes multipart-specific normalization helpers.
• Adds unit tests validating table-scoped normalization for both comma-separated column strings and record-shaped inputs (including multi-rule tables).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/storage/database/pg.ts	Adds table-scoped normalization rules and rewires bucket/object/multipart-upload column gating to use them.
src/storage/database/pg.test.ts	Adds focused tests covering table-scoped normalization behavior across strings/records and multiple pending rules.

Suppressed comments (1)

src/storage/database/pg.ts:446

• findBucketById now calls normalizeColumns('buckets', columns), but normalizeColumns is a no-op when latestMigration is undefined. That means callers can request type (or *) on tenants that haven’t run iceberg-catalog-flag-on-buckets and get a SQL error selecting a non-existent column. Previously this was guarded via hasMigration() which queried tenant migrations when latestMigration wasn’t provided.

          text: `
            SELECT ${selectColumns(this.normalizeColumns('buckets', columns))}
            FROM storage.buckets

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

LGTM. This is a clean, well-tested refactor with no bugs found.

What was reviewed: the new table-scoped COLUMN_MIGRATION_RULES / normalizeColumns() logic and its call sites in createBucket, findBucketById, object CRUD, and multipart upload methods; confirmed the migration-ordering claim in the description (custom-metadata=25, s3-multipart-uploads-metadata=57) matches DBMigration; verified the removed hasMultipartMetadataColumn/normalizeMultipartUploadColumns helpers have no remaining references. Checked the buckets.type short-circuit when latestMigration is undefined (flagged by the bug-hunt system and ruled out) — confirmed independently that all HTTP call sites populate latestMigration via the db plugin, and no caller requests the type column through the one queue/event path that omits it.

Extended reasoning...

Overview

This PR refactors normalizeColumns() in src/storage/database/pg.ts to be table-scoped via a new COLUMN_MIGRATION_RULES map, consolidating three previously-separate ad hoc column-filtering implementations (buckets.type, s3_multipart_uploads.metadata, objects.user_metadata) into one function. It also fixes a real bug in the old loop: it returned on the first not-yet-migrated rule instead of collecting all pending rules, so a table with two rules (s3_multipart_uploads) would only ever apply one. Changes are confined to pg.ts and its test file, with a solid new test suite covering string vs record inputs, multi-rule tables, and the undefined-migration fallback.

Security risks

None. This only affects which columns are selected/inserted based on migration state — no user input reaches SQL text directly (columns pass through quoteIdentifier/selectColumns as before), and no auth/permission logic is touched.

Level of scrutiny

Moderate — this is a refactor of internal migration-compatibility plumbing in a core storage class, not a config tweak, but the change is small, mechanical, and comes with strong new unit test coverage that directly exercises the behavior being changed (including the multi-rule regression the PR explicitly calls out).

Other factors

I independently checked the one nuance in this diff worth double-checking: since createBucket/findBucketById now route buckets.type filtering through normalizeColumns()'s synchronous if (!latestMigration) return columns short-circuit (previously they used the async hasMigration(), which falls back to a live tenant-config check when latestMigration is undefined), a bucket instantiated without latestMigration could see stale behavior for the type column. This was already flagged and ruled out by the bug-hunting system. I traced it further myself: every HTTP-driven call site gets latestMigration populated unconditionally in the db.ts preHandler hook (both multitenant and single-tenant paths), and the only construction site that omits it (base-event.ts, used by the queue/event workers) never calls findBucketById/createBucket requesting the type column. So this doesn't appear to be reachable in practice. There's also an outstanding non-blocking question from the PR author (Tyler) about this same undefined-migration fallback behavior for hasMigration() more generally, which he explicitly left as-is and isn't a new issue introduced by this PR.

[TylerHillery]

closing in favor of #1238