Update storage mapping mutation #2605

GregorShear · 2026-01-09T21:42:22Z

Description: Adds updateStorageMapping GraphQL mutation for modifying existing storage mappings. Refactors shared validation and health check logic into reusable helper functions. The update response includes a republish field indicating whether the primary storage bucket changed, which signals that affected specs will need republishing.

Workflow steps: Call updateStorageMapping with the same input shape as createStorageMapping:

mutation {
  updateStorageMapping(
    catalogPrefix: "gregCo/"
    dryRun: false
    storage: {
      stores: [
        {provider: "GCS", bucket: "your-bucket", prefix: "collection-data/"}
      ]
      data_planes: ["ops/dp/public/local-cluster"]
    }
  ) {
    updated
    catalogPrefix
    republish
  }
}

Use dryRun: true to validate input and check if a republish would be required without persisting changes.

Notes for reviewers:

The republish field compares the incoming list of stores with the existing
The TODO comment about actually triggering republish remains - this PR only reports whether it's needed

psFried

This looks great so far. I left one minor comment, but apart from that this should be good to go once it's rebased on top of the changes to the other PR

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

GregorShear · 2026-01-13T19:59:04Z

republish logic updated and branch rebased on top of the other PR

psFried

I left a bit more feedback inline, but should be good after that

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

psFried · 2026-01-14T13:07:19Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+            "updated storage mapping"
+        );
+
+        // TODO: if the primary (first) fragmentStore changed, republish the entire prefix.


nit: seems like the plan is for the client to handle the republication, so this can be removed?

does this gql operation need to be aware of the republishing process, or is that just handled elsewhere when changes are detected?

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

…mutations Split the single upsertStorageMapping GraphQL mutation into separate createStorageMapping and updateStorageMapping mutations with distinct behavior: - createStorageMapping: fails if a mapping already exists, checks for existing specs that would be affected - updateStorageMapping: fails if no mapping exists, returns whether a republish is needed due to store changes

GregorShear · 2026-01-20T15:42:57Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+        let sampled_specs = sqlx::query_scalar!(
+            r#"
+            SELECT catalog_name
+            FROM live_specs
+            WHERE starts_with(catalog_name, $1)
+            AND spec IS NOT NULL
+            LIMIT 5
+            "#,
+            &catalog_prefix,
+        )
+        .fetch_all(&mut *txn)
+        .await?;


@psFried
okay to leave these inline or would you rather keep all queries in crates/control-plane-api/src/directives/storage_mappings.rs?

I think inline is generally fine, as long as it's not something we're trying to re-use elsewhere. For this particular query, inline seems fine. But we have some existing functions in crates/control-plane-api/src/directives/storage_mappings.rs that seem like they'd work for inserts and updates to storage mappings. I think it's worth using those, unless there's a reason not to.

GregorShear · 2026-01-20T15:43:39Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+        sqlx::query!(
+            r#"
+            UPDATE storage_mappings
+            SET spec = $2, detail = $3, updated_at = now()


should we be manually updating the updated_at value? i would expect this to be handled with an UPDATE trigger...

Yeah, we just handle those manually instead of with triggers. It's not something I feel especially strongly about, and we do still use triggers here and there for other things. Though I think my general position is to try to avoid triggers when we can, to make things more explicit.

That said, I think the existing upsert function is probably what we should use here instead of inline sql, and that already takes care of setting updated_at.

GregorShear · 2026-01-20T15:44:34Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+        sqlx::query!(
+            r#"
+            UPDATE storage_mappings
+            SET spec = $2, detail = $3, updated_at = now()
+            WHERE catalog_prefix = 'recovery/' || $1
+            "#,
+            &catalog_prefix as &str,
+            crate::TextJson(&recovery_storage) as crate::TextJson<&models::StorageDef>,
+            detail,
+        )
+        .execute(&mut *txn)
+        .await?;


I think this recovery/ logic is right but would love some extra scrutiny here

See my other comments about factoring out a function that returns both the storage defs from the original input, and about reusing the existing functions for updating the database. I'm thinking we should use upsert_storage_mapping, and then this inline query would not be necessary.

psFried

This looks like a good start. I left some comments inline, mostly about trying to re-use the existing logic from directives::storage_mappings. LMK if you'd like to talk through any of that

psFried · 2026-01-20T20:40:56Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+        let sampled_specs = sqlx::query_scalar!(
+            r#"
+            SELECT catalog_name
+            FROM live_specs
+            WHERE starts_with(catalog_name, $1)
+            AND spec IS NOT NULL
+            LIMIT 5
+            "#,
+            &catalog_prefix,
+        )
+        .fetch_all(&mut *txn)
+        .await?;


I think inline is generally fine, as long as it's not something we're trying to re-use elsewhere. For this particular query, inline seems fine. But we have some existing functions in crates/control-plane-api/src/directives/storage_mappings.rs that seem like they'd work for inserts and updates to storage mappings. I think it's worth using those, unless there's a reason not to.

psFried · 2026-01-21T16:52:43Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+                .cloned()
+                .map(|mut store| {
+                    let prefix = store.prefix_mut();
+                    *prefix = models::Prefix::new(format!("{prefix}collection-data/"));


I foresee potential issues with this logic in update scenarios. The UI will fetch the existing storage mappings, which already have the collection-data/ prefix, and then pass those storage mappings to update, which would add the prefix again. So I'm thinking at a minimum we'd want to change the corresponding logic that's used during the update to check whether the prefix is there already. But also, this feels like it ought to be in a function that's shared between insert and update operations. Ideally, we'd have a pure function that accepts a single models::StorageDef, and returns a tuple of both the collection and recovery storage defs. Then we could continue to use that same function, even after we no longer persist the recovery mappings in postgres.

psFried · 2026-01-21T16:59:32Z

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs

+        sqlx::query!(
+            r#"
+            UPDATE storage_mappings
+            SET spec = $2, detail = $3, updated_at = now()
+            WHERE catalog_prefix = 'recovery/' || $1
+            "#,
+            &catalog_prefix as &str,
+            crate::TextJson(&recovery_storage) as crate::TextJson<&models::StorageDef>,
+            detail,
+        )
+        .execute(&mut *txn)
+        .await?;


See my other comments about factoring out a function that returns both the storage defs from the original input, and about reusing the existing functions for updating the database. I'm thinking we should use upsert_storage_mapping, and then this inline query would not be necessary.

GregorShear requested a review from psFried January 9, 2026 21:43

psFried reviewed Jan 12, 2026

View reviewed changes

crates/control-plane-api/src/server/public/graphql/storage_mappings.rs Outdated Show resolved Hide resolved

GregorShear force-pushed the greg/mutation/updateMapping branch from ca9af4c to ac89dcd Compare January 13, 2026 19:28

GregorShear marked this pull request as ready for review January 13, 2026 19:55

GregorShear requested a review from psFried January 13, 2026 19:59

psFried requested changes Jan 14, 2026

View reviewed changes

Base automatically changed from greg/mapping/mutation to master January 15, 2026 16:24

GregorShear force-pushed the greg/mutation/updateMapping branch from 8323a70 to 50cd8ec Compare January 15, 2026 16:45

GregorShear force-pushed the greg/mutation/updateMapping branch from 46431bd to cbb12a9 Compare January 20, 2026 15:40

GregorShear commented Jan 20, 2026

View reviewed changes

GregorShear requested a review from psFried January 20, 2026 15:44

psFried requested changes Jan 21, 2026

View reviewed changes

Update storage mapping mutation #2605

Are you sure you want to change the base?

Update storage mapping mutation #2605

Conversation

GregorShear commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psFried left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GregorShear commented Jan 13, 2026

Uh oh!

psFried left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psFried Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psFried left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GregorShear commented Jan 9, 2026 •

edited

Loading

psFried Jan 20, 2026 •

edited

Loading