Skip to content

refactor(mirrors): drop denormalized ProductMirror.storage_type - #456

Draft
alukach wants to merge 6 commits into
mainfrom
remove-mirror-storage-type
Draft

refactor(mirrors): drop denormalized ProductMirror.storage_type#456
alukach wants to merge 6 commits into
mainfrom
remove-mirror-storage-type

Conversation

@alukach

@alukach alukach commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Stacked on #455 — review/merge that first; GitHub retargets this to main once it lands.

Why

Nothing branched on storage_type: the data proxy keys off the connection's details.provider, and the only source.coop consumer was a display label. With provider values now equal to the backend vocabulary (#455), the provider→storage_type maps are pure identity.

Changes

  • Drop storage_type from ProductMirrorSchema; delete both STORAGE_TYPE_BY_PROVIDER maps.
  • Mirror "Type" label now reads the provider from connectionInfo (sourced from the connection's details.provider).
  • No migration: ProductMirrorSchema is a plain z.object, so existing records still carrying storage_type parse fine (unknown key stripped).

Verification

tsc clean · jest affected suites 116 · lint clean.

🤖 Generated with Claude Code

alukach and others added 2 commits July 22, 2026 11:46
@alukach @claude
fix(data-connections): align provider values to the backend vocabulary
780ff13 
Creating a GCP data connection persisted `details.provider: "gcp"`, but the
data proxy reads `details.provider` directly and only accepts "gcs"/"gs"
for Google (never "gcp"); the same latent mismatch existed for Azure ("az"
vs the accepted "azure"). The provider→storage_type map already emitted the
right values, so mirrors were fine — only the stored provider string was
wrong.

Rename the DataProvider values to what the backend accepts: Azure "az"→
"azure", GCP→GCS "gcp"→"gcs". Member GCP→GCS, schema/type Gcp*→Gcs*. Auth
type stays `gcp_workload_identity` (WIF is a GCP-platform feature, not
storage). Provider labels spelled out in the form.

Legacy rows: reads bypass Zod, so `normalizeConnection` in the
data-connections DynamoDB client remaps "az"→"azure" / "gcp"→"gcs" on
fetchById/listAll. Marked ponytail; drop after backfill.

The now-redundant provider→storage_type translation is left in place here
and removed in the follow-up that drops ProductMirror.storage_type.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alukach @claude
refactor(mirrors): drop denormalized ProductMirror.storage_type
bc16ddc 
Nothing branched on `storage_type`: the data proxy keys off the
connection's `details.provider`, and the only source.coop consumer was a
display label. With provider values now equal to the backend vocabulary
(previous commit), the provider→storage_type maps in createProduct /
addProductMirror are pure identity — so remove the field and both maps.

- Drop `storage_type` from ProductMirrorSchema; delete both
  STORAGE_TYPE_BY_PROVIDER maps (mirror no longer stores it).
- Mirror "Type" label now reads the provider from connectionInfo (sourced
  from the connection's details.provider on the edit page).
- No migration: ProductMirrorSchema is a plain z.object, so existing
  records still carrying storage_type parse fine (unknown key stripped).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 22, 2026
Closed
Closed
@vercel

vercel Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
source-cooperative Ready Ready Preview, Comment Jul 22, 2026 10:30pm

Request Review

@claude

claude Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @alukach's task in 1m 33s —— View job

Reviewing PR #456

  • Gather context (diff, changed files)
  • Review diff for correctness/security/best-practice issues
  • Post findings

✅ No blocking issues — safe to merge.

  • src/components/features/data-connections/ProductMirrorsManager.tsx:236-240 — minor inconsistency: the new "Type" field reads connectionInfo[mirror.connection_id]?.provider with no existence guard, while the adjacent "Bucket" field is wrapped in connectionInfo[mirror.connection_id] && (...). If a mirror's connection is ever missing from connectionInfo (not currently reachable since deleteDataConnection refuses deletion while dependent mirrors exist, but worth keeping consistent for defense-in-depth), "Type" would silently render blank instead of being omitted like "Bucket". Not blocking, just a small inconsistency: 
    <Field label="Type">
  <Text size="2">{connectionInfo[mirror.connection_id]?.provider}</Text>
</Field>

Otherwise the refactor is clean:

  • The storage_type → provider identity claim holds — DataProvider (s3/azure/gcs) now matches the values STORAGE_TYPE_BY_PROVIDER used to produce, and no other code references storage_type after this change (verified via repo-wide search).
  • ProductMirrorSchema correctly drops storage_type as a plain z.object, so old persisted records with the extra key still parse (unknown key stripped) — matches the "no migration needed" claim in the PR description.
  • Test deletions match the removed behavior (mirror-derives-storage_type test removed alongside the code it tested).
  • page.tsx's connectionInfo now sources provider from c.details.provider, which is a field common across connection detail variants (already used elsewhere in the codebase), so the type change is sound.

alukach and others added 2 commits July 22, 2026 15:10
@alukach @claude
fix(fixtures): azure connection provider az->azure
de9b172 
The azure-data-connection fixture still carried the pre-rename provider
value; AzureDataConnectionSchema now requires "azure", and utils.mock.ts
eagerly parses this fixture at module load, so every test importing it (and
local dev seeding via init-local.ts) threw. Caught in CI review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alukach
Merge branch 'align-provider-values' into remove-mirror-storage-type
64a8cbf
@alukach alukach mentioned this pull request Jul 22, 2026
Merged
alukach and others added 2 commits July 22, 2026 15:28
@alukach @claude
refactor(data-connections): migration script instead of read-time shim
b42b9bd 
Replace the runtime normalizeConnection read-shim (LEGACY_PROVIDER_ALIASES)
with a one-time migration script that converts persisted provider values
"az"->"azure" / "gcp"->"gcs". Keeps the DB read path free of legacy-value
handling; the conversion is done once at deploy instead of on every read.

- Remove normalizeConnection + its test; fetchById/listAll return rows as-is.
- Add scripts/migrate-provider-values.ts, matching the existing migration
  convention (positional table arg, DynamoDB scan + guarded UpdateCommand,
  progress logging). Dry-run by default; --apply to write. Re-runnable
  (canonical rows are skipped).

Run the migration (dry-run, then --apply) before deploying the value rename.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@alukach
Merge branch 'align-provider-values' into remove-mirror-storage-type
1852fba 
# Conflicts:
#	src/lib/clients/database/data-connections.ts
#	src/types/data-connection.ts
alukach added a commit that referenced this pull request Jul 22, 2026
@alukach @claude
fix(data-connections): align provider values to the backend vocabulary (
06adc4c 
#455)

## Problem
Creating a GCP data connection persisted `details.provider: "gcp"`, but
the data proxy (data.source.coop) reads `details.provider` directly and
only accepts `"gcs"`/`"gs"` for Google — never `"gcp"`. The same latent
mismatch existed for Azure (`"az"` vs the accepted `"azure"`). The
`provider→storage_type` map already emitted the right values, so mirrors
were fine — **only the stored provider string was wrong**.

## Change — rename `DataProvider` values to what the backend accepts
```
enum DataProvider { S3 = "s3", Azure = "azure" /* was "az" */, GCS = "gcs" /* was GCP="gcp" */ }
```
- Member `GCP`→`GCS`, schema/type `Gcp*`→`Gcs*`. Auth type stays
`gcp_workload_identity` (WIF is a GCP-*platform* feature, not storage).
Provider labels spelled out in the form.
- **Maps kept** — the now-redundant `provider→storage_type` translation
is left in place here and removed in the stacked follow-up (#456).

## Migration (legacy `az`/`gcp` rows)
A one-time script — `scripts/migrate-provider-values.ts` — converts
persisted `details.provider` values (`"az"→"azure"`, `"gcp"→"gcs"`),
matching the repo's existing migration convention (positional table arg,
DynamoDB scan + guarded `UpdateCommand`, progress logging). **Dry-run by
default; `--apply` to write.** Re-runnable (canonical rows are skipped).
Run it (dry-run, then `--apply`) at/before deploy:
```
npx tsx scripts/migrate-provider-values.ts sc-prod-data-connections          # preview
npx tsx scripts/migrate-provider-values.ts sc-prod-data-connections --apply   # write
```
(Earlier revisions used a runtime read-shim; replaced with this script
so the DB read path carries no legacy-value handling.)

## Backend compatibility (verified against data.source.coop)
**S3** unchanged. **Azure** accepts both `"az"` and `"azure"` →
compatible during rollout. **GCS**: old `"gcp"` never worked (hit the
error arm); `"gcs"` is accepted. Pairs with data.source.coop#191
(enables the GCS backend).

## Verification
`tsc` clean · `jest` affected suites green · lint clean · fixture
`provider` fixed (`az`→`azure`) so module-load parsing passes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Base automatically changed from align-provider-values to main July 22, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@alukach