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.

…ngs helpers

GregorShear · 2026-01-22T21:44:22Z

crates/control-plane-api/src/directives/storage_mappings.rs

+pub async fn update_storage_mapping<'e, T, E>(
+    detail: Option<&str>,
+    catalog_prefix: &str,
+    spec: T,
+    executor: E,
+) -> sqlx::Result<bool>
+where


Same rationale as the GraphQL mutations - separate create / update functions will generally reduce complexity. Keeping upsert for now to minimize the diff and it's convenient for the recovery/ mappings

👍 yep, sounds good to me

GregorShear · 2026-01-22T21:46:45Z

crates/control-plane-api/src/directives/storage_mappings.rs

+pub fn split_collection_and_recovery_storage(
+    storage: models::StorageDef,
+) -> (models::StorageDef, models::StorageDef) {


Is this a good place for this util function to live? and related tests below?

Yeah, I think so. We'll probably eventually move this whole file out from directives, but I think it's the best place for this logic.

GregorShear · 2026-01-22T21:56:10Z

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

+        // Begin a transaction to fetch existing mapping and update.
        let mut txn = env.pg_pool.begin().await?;

+        // Fetch existing storage mapping to compare stores and verify it exists.


the query just below here looks similar to the existing fetch function in the shared file, except here we're locking the row with FOR UPDATE - i'm inclined to keep this query defined here until we find another place we need the row lock

Yeah, this seems good to me

GregorShear · 2026-01-22T21:57:29Z

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

+        upsert_storage_mapping(
+            detail.as_deref(),
+            &format!("recovery/{catalog_prefix}"),
+            &recovery_storage,
+            &mut txn,
+        )
+        .await?;


i mention this elsewhere but i'm keeping the upsert function here because it does feel convenient for the recovery case. I'd make a case for removing upsert entirely when the recovery/ mappings are gone

Sounds good to me 👍

GregorShear · 2026-01-22T22:01:58Z

crates/control-plane-api/src/directives/storage_mappings.rs

+                if !prefix.as_str().ends_with(COLLECTION_DATA_SUFFIX) {
+                    *prefix = models::Prefix::new(format!("{prefix}{COLLECTION_DATA_SUFFIX}"));
+                }


Just to talk this through - the collection-data/ part may have already been added in a previous operation, so we want to check if it's there first

I'm realizing that maybe it's not actually great for us to return the storage mappings with the /collection-data/ added... we'd presumably display /user-prefix/collection-data/ on the storage mappings UI, which seems like it'd be confusing to users if we then went and also wrote data to /user-prefix/recovery/.

We started out with the idea that we'd want to only expose a single representation of the storage mapping to users, and hide the split behind our API. I still think that's a good idea, but I think I was wrong about my previous suggestion to only conditionally add the collection-data/. Technically, it works, but it doesn't feel right.

I think it might be better if we instead strip the collection-data/ when we return the storage mappings in the API response? Then the UI would only ever see /user-prefix/, and we'd automatically add the collection-data/ when we persist changes. Does that make sense to you?

GregorShear · 2026-01-22T22:02:35Z

crates/control-plane-api/src/directives/storage_mappings.rs

+            .into_iter()
+            .map(|mut store| {
+                let prefix = store.prefix_mut();
+                if let Some(base) = prefix.as_str().strip_suffix(COLLECTION_DATA_SUFFIX) {


similar to above - the collection-data/ part may exist from a previous operation, so we remove it for the recovery/ mapping

GregorShear · 2026-01-22T22:06:29Z

@psFried took another pass here - i believe i've incorporated all of your feedback. I'm making a case to (eventually) deprecate the existing upsert function and move toward separate create/update functions in directives/storage_mappings.rs for the same reason we gave for the gql mutations. Happy to chat through this if you disagree

psFried

The changes all look good, except I realized that we might want to change how we handle the collection-data/ being added. LMK if you want to talk that over on a VC

psFried · 2026-01-23T18:42:25Z

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

+        upsert_storage_mapping(
+            detail.as_deref(),
+            &format!("recovery/{catalog_prefix}"),
+            &recovery_storage,
+            &mut txn,
+        )
+        .await?;


Sounds good to me 👍

psFried · 2026-01-23T19:48:38Z

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

+        // Begin a transaction to fetch existing mapping and update.
        let mut txn = env.pg_pool.begin().await?;

+        // Fetch existing storage mapping to compare stores and verify it exists.


Yeah, this seems good to me

psFried · 2026-01-23T19:49:27Z

crates/control-plane-api/src/directives/storage_mappings.rs

+pub async fn update_storage_mapping<'e, T, E>(
+    detail: Option<&str>,
+    catalog_prefix: &str,
+    spec: T,
+    executor: E,
+) -> sqlx::Result<bool>
+where


👍 yep, sounds good to me

psFried · 2026-01-23T19:50:20Z

crates/control-plane-api/src/directives/storage_mappings.rs

+pub fn split_collection_and_recovery_storage(
+    storage: models::StorageDef,
+) -> (models::StorageDef, models::StorageDef) {


Yeah, I think so. We'll probably eventually move this whole file out from directives, but I think it's the best place for this logic.

psFried · 2026-01-23T20:13:12Z

crates/control-plane-api/src/directives/storage_mappings.rs

+                if !prefix.as_str().ends_with(COLLECTION_DATA_SUFFIX) {
+                    *prefix = models::Prefix::new(format!("{prefix}{COLLECTION_DATA_SUFFIX}"));
+                }


I'm realizing that maybe it's not actually great for us to return the storage mappings with the /collection-data/ added... we'd presumably display /user-prefix/collection-data/ on the storage mappings UI, which seems like it'd be confusing to users if we then went and also wrote data to /user-prefix/recovery/.

We started out with the idea that we'd want to only expose a single representation of the storage mapping to users, and hide the split behind our API. I still think that's a good idea, but I think I was wrong about my previous suggestion to only conditionally add the collection-data/. Technically, it works, but it doesn't feel right.

I think it might be better if we instead strip the collection-data/ when we return the storage mappings in the API response? Then the UI would only ever see /user-prefix/, and we'd automatically add the collection-data/ when we persist changes. Does that make sense to you?

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

GregorShear added 2 commits January 22, 2026 16:13

Extract split_collection_and_recovery_storage and reuse storage_mappi…

cf6ec0b

…ngs helpers

Make storage mapping detail parameter optional

1c67122

GregorShear force-pushed the greg/mutation/updateMapping branch from b5f7b81 to 1c67122 Compare January 22, 2026 21:30

GregorShear commented Jan 22, 2026

View reviewed changes

GregorShear requested a review from psFried January 22, 2026 22:04

GregorShear force-pushed the greg/mutation/updateMapping branch from 36eee1d to eed89cb Compare January 23, 2026 16:27

Refactor evaluate_authorization to separate concerns

dd29c4d

GregorShear force-pushed the greg/mutation/updateMapping branch from eed89cb to dd29c4d Compare January 23, 2026 16:48

chore: trigger ci

0eefab5

psFried reviewed Jan 23, 2026

View reviewed changes

GregorShear added 2 commits January 23, 2026 16:10

update gql schema

b74cea9

Make storage mapping dry_run parameter optional

6d74f60

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregorShear Jan 22, 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!

GregorShear commented Jan 22, 2026

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

GregorShear commented Jan 9, 2026 •

edited

Loading

psFried Jan 20, 2026 •

edited

Loading

GregorShear Jan 22, 2026 •

edited

Loading