feat: added ConfigChanged webhook event for config mutation notifications#877
feat: added ConfigChanged webhook event for config mutation notifications#877
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces webhook notifications for configuration changes across multiple handlers in the context-aware config API. When configurations are created, updated, deleted, or bulk-operated, a new webhook trigger function is invoked to notify external listeners. The implementation centralizes header construction logic into service utilities and adds serialization support to core types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Config Handler
participant DB as Database
participant Webhook as Webhook Service
participant External as External Listener
participant Redis as Redis Cache
Client->>Handler: POST/PUT/DELETE config
Handler->>DB: Perform config operation
DB-->>Handler: Operation result
Handler->>Redis: Update cache (optional)
Redis-->>Handler: Cache updated
Handler->>Webhook: trigger_config_changed_webhook()
Webhook->>DB: Lookup ConfigChanged webhook
DB-->>Webhook: Webhook endpoint
Webhook->>External: POST webhook notification
External-->>Webhook: Response
Webhook-->>Handler: webhook_status (true/false)
Handler->>Client: HTTP 200/512 based on webhook_status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/context_aware_config/src/api/default_config/handlers.rs`:
- Around line 565-585: The delete handler currently calls
trigger_config_changed_webhook and only logs when webhook_status is false, then
continues to return 204; change it to mirror create/update behavior by returning
an error response when the webhook call fails: after calling
trigger_config_changed_webhook (the webhook_status boolean), if webhook_status
is false return the same 512-style error response used by create/update handlers
instead of just logging. Update the control flow around
trigger_config_changed_webhook in the delete path (references:
trigger_config_changed_webhook, webhook_status, the delete handler that
currently returns 204) so webhook failures propagate to the client consistently.
In `@crates/context_aware_config/src/helpers.rs`:
- Around line 249-283: In trigger_config_changed_webhook, treat a failed
fetch_webhook_by_event as a failure instead of success: when
fetch_webhook_by_event returns Err, log the error (use the project logger
available in AppState or tracing::error) and return false rather than true so
callers receive the correct failure semantics; keep the successful path that
calls execute_webhook_call(...) for the Ok case and ensure the log message
references WebhookEvent::ConfigChanged and the version_id to aid debugging.
🧹 Nitpick comments (5)
crates/service_utils/Cargo.toml (1)
11-11: Nit: inconsistent brace spacing.Other workspace dependencies in this file use
{ workspace = true }(with inner spaces), but this line omits them.-actix-http = {workspace = true} +actix-http = { workspace = true }crates/context_aware_config/src/helpers.rs (1)
219-246: Makeconfig_hashdeterministic across runs.
json_config.to_string()can serialize HashMap-backed fields in nondeterministic key order, which can yield different hashes for identical configs. If this hash is used for change detection/deduping, consider canonical JSON (sorted keys) or ordered maps before hashing.crates/context_aware_config/src/api/context/handlers.rs (3)
141-159: Consider extracting the repeated webhook-trigger + status-code-512 pattern into a helper.This exact block — call
trigger_config_changed_webhook, then branch on the result to build eitherHttpResponse::Ok()orHttpResponse::build(512)— is duplicated verbatim across all six mutation handlers (create, update, move, delete, bulk, weight_recompute). A small helper returning anHttpResponseBuilderwould eliminate ~100 lines of duplication.Sketch of a possible helper
// In crate helpers or at the top of this module: async fn trigger_webhook_and_build_response( version_id: i64, workspace_context: &WorkspaceContext, state: &Data<AppState>, conn: &mut DBConnection, user: &User, change_reason: &str, ) -> HttpResponseBuilder { let ok = trigger_config_changed_webhook( version_id, workspace_context, state, conn, user, change_reason, ) .await; if ok { HttpResponse::Ok() } else { HttpResponse::build( actix_web::http::StatusCode::from_u16(512) .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR), ) } }
579-580: Duplicated "Deleted context by" format string.Line 567 already constructs
format!("Deleted context by {}", user.username)for the description inside the transaction. Line 580 rebuilds the same string for the webhook. Consider computing it once before the transaction and reusing it for both.Proposed fix
+ let delete_reason = format!("Deleted context by {}", user.username); let version_id = db_conn.transaction::<_, superposition::AppError, _>(|transaction_conn| { // ... let config_version_desc = - Description::try_from(format!("Deleted context by {}", user.username)) + Description::try_from(delete_reason.clone()) .map_err(|e| unexpected_error!(e))?; // ... })?; let DbConnection(mut conn) = db_conn; - let webhook_change_reason = format!("Deleted context by {}", user.username); + let webhook_change_reason = delete_reason;
595-609: Ordering inconsistency: indelete_handler, the response builder is constructed after the Redis update, unlike other handlers.In create/update/move/bulk/weight_recompute, the pattern is: webhook → build response → redis → return. Here it's: webhook → redis → build response → return. This works but is inconsistent and could confuse future readers. Consider reordering to match the other handlers for uniformity.
| let webhook_status = trigger_config_changed_webhook( | ||
| version_id, | ||
| &workspace_context, | ||
| &state, | ||
| &mut conn, | ||
| &user, | ||
| &format!( | ||
| "Default config key '{}' deleted by {}", | ||
| key, | ||
| user.get_email() | ||
| ), | ||
| ) | ||
| .await; | ||
|
|
||
| if !webhook_status { | ||
| log::warn!( | ||
| "ConfigChanged webhook failed for workspace: {}", | ||
| workspace_context.settings.workspace_name | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Webhook failure doesn’t affect delete response.
Create/update return 512 on webhook failure; delete only logs and still returns 204, which hides webhook failures and diverges from the stated behavior.
🛠️ Suggested fix
if !webhook_status {
log::warn!(
"ConfigChanged webhook failed for workspace: {}",
workspace_context.settings.workspace_name
);
}
#[cfg(feature = "high-performance-mode")]
put_config_in_redis(
version_id,
state,
&workspace_context.schema_name,
&mut conn,
)
.await?;
+
+ if !webhook_status {
+ return Ok(HttpResponse::build(
+ actix_web::http::StatusCode::from_u16(512)
+ .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR),
+ )
+ .finish());
+ }
}
resp🤖 Prompt for AI Agents
In `@crates/context_aware_config/src/api/default_config/handlers.rs` around lines
565 - 585, The delete handler currently calls trigger_config_changed_webhook and
only logs when webhook_status is false, then continues to return 204; change it
to mirror create/update behavior by returning an error response when the webhook
call fails: after calling trigger_config_changed_webhook (the webhook_status
boolean), if webhook_status is false return the same 512-style error response
used by create/update handlers instead of just logging. Update the control flow
around trigger_config_changed_webhook in the delete path (references:
trigger_config_changed_webhook, webhook_status, the delete handler that
currently returns 204) so webhook failures propagate to the client consistently.
| pub async fn trigger_config_changed_webhook( | ||
| version_id: i64, | ||
| workspace_context: &WorkspaceContext, | ||
| state: &Data<AppState>, | ||
| db_conn: &mut DBConnection, | ||
| user: &User, | ||
| change_reason: &str, | ||
| ) -> bool { | ||
| let payload = json!({ | ||
| "change_reason": change_reason, | ||
| "config_version": version_id, | ||
| }); | ||
|
|
||
| if let Ok(webhook) = fetch_webhook_by_event( | ||
| state, | ||
| user, | ||
| &WebhookEvent::ConfigChanged, | ||
| workspace_context, | ||
| ) | ||
| .await | ||
| { | ||
| execute_webhook_call( | ||
| &webhook, | ||
| &payload, | ||
| &Some(version_id.to_string()), | ||
| workspace_context, | ||
| WebhookEvent::ConfigChanged, | ||
| state, | ||
| db_conn, | ||
| ) | ||
| .await | ||
| } else { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Webhook fetch errors are treated as success.
If fetch_webhook_by_event fails (network/5xx), this returns true, so callers respond 200 even though the webhook wasn’t sent. Consider logging and returning false so 512 semantics apply.
🛠️ Suggested fix
- if let Ok(webhook) = fetch_webhook_by_event(
- state,
- user,
- &WebhookEvent::ConfigChanged,
- workspace_context,
- )
- .await
- {
- execute_webhook_call(
- &webhook,
- &payload,
- &Some(version_id.to_string()),
- workspace_context,
- WebhookEvent::ConfigChanged,
- state,
- db_conn,
- )
- .await
- } else {
- true
- }
+ match fetch_webhook_by_event(
+ state,
+ user,
+ &WebhookEvent::ConfigChanged,
+ workspace_context,
+ )
+ .await
+ {
+ Ok(webhook) => {
+ execute_webhook_call(
+ &webhook,
+ &payload,
+ &Some(version_id.to_string()),
+ workspace_context,
+ WebhookEvent::ConfigChanged,
+ state,
+ db_conn,
+ )
+ .await
+ }
+ Err(err) => {
+ log::error!("Failed to fetch ConfigChanged webhook: {err}");
+ false
+ }
+ }🤖 Prompt for AI Agents
In `@crates/context_aware_config/src/helpers.rs` around lines 249 - 283, In
trigger_config_changed_webhook, treat a failed fetch_webhook_by_event as a
failure instead of success: when fetch_webhook_by_event returns Err, log the
error (use the project logger available in AppState or tracing::error) and
return false rather than true so callers receive the correct failure semantics;
keep the successful path that calls execute_webhook_call(...) for the Ok case
and ensure the log message references WebhookEvent::ConfigChanged and the
version_id to aid debugging.
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
from
92f3b73 to
9197251
Compare
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
2 times, most recently
from
7f60e14 to
381648f
Compare
| } | ||
|
|
||
| #[derive(Deref, DerefMut, Clone, Debug)] | ||
| #[derive(Deref, DerefMut, Clone, Debug, Serialize, Deserialize)] |
There was a problem hiding this comment.
These types are usually added to inject a value into handlers. They don't really need these traits
There was a problem hiding this comment.
Same for the other types in this file
There was a problem hiding this comment.
+1
please revert changes to this file
There was a problem hiding this comment.
superposition/crates/context_aware_config/src/api/context/handlers.rs
Lines 131 to 139 in 381648f
Can you add the trigger for the webhook in add_config_version?
There was a problem hiding this comment.
No , add_config_version is synchronous and the trigger is async , we cannot call async function inside transaction.
There was a problem hiding this comment.
But you can return future which can be awaited on outside the synchronous block of the transaction
Anyways, on failure of webhook we dont even need to revert the queries so response awaiting outside transaction is safe as well
There was a problem hiding this comment.
There is a slight problem with this approach - I would be using execute_webhook_call which needs &mut DBConnection, but the transaction still holds the mutable borrow when we create the future. The future can't capture db_conn — the borrow checker won't allow it.
You can't return a future that captures &mut DBConnection from inside a transaction because the connection's lifetime is scoped to the transaction closure.
There was a problem hiding this comment.
Explanation:
Modified add_config_version:
pub fn add_config_version<'a, T>(
state: &'a Data<AppState>,
tags: Option<Vec<String>>,
description: Description,
db_conn: &'a mut DBConnection,
workspace_context: &'a WorkspaceContext,
payload: &'a T,
) -> superposition::Result<(i64, Pin<Box<dyn Future<Output = bool> + Send + 'a>>)>
where
T: Serialize + Send + Sync,
{
use config_versions::dsl::config_versions;
let version_id = generate_snowflake_id(state)?;
let config = generate_cac(db_conn, &workspace_context.schema_name)?;
let json_config = json!(config);
let config_hash = blake3::hash(json_config.to_string().as_bytes()).to_string();
let config_version = ConfigVersion {
id: version_id,
config: json_config,
config_hash,
tags,
created_at: Utc::now(),
description,
};
diesel::insert_into(config_versions)
.values(&config_version)
.returning(ConfigVersion::as_returning())
.schema_name(&workspace_context.schema_name)
.execute(db_conn)?;
let webhook_future = Box::pin(async move {
execute_webhook_call(
payload,
&Some(version_id.to_string()),
workspace_context,
WebhookEvent::ConfigChanged,
state,
db_conn,
)
.await
});
Ok((version_id, webhook_future))
}
Sample Usage:
#[authorized]
#[put("")]
async fn create_handler(
workspace_context: WorkspaceContext,
state: Data<AppState>,
custom_headers: CustomHeaders,
req: Json<PutRequest>,
mut db_conn: DbConnection,
user: User,
) -> superposition::Result<HttpResponse> {
let tags = parse_config_tags(custom_headers.config_tags)?;
let description = match req.description.clone() {
Some(val) => val,
None => {
// TODO: get rid of `query_description` function altogether
let resp = query_description(
Value::Object(req.context.clone().into_inner().into()),
&mut db_conn,
&workspace_context.schema_name,
);
match resp {
Err(AppError::DbError(diesel::result::Error::NotFound)) => {
return Err(bad_argument!(
"Description is required when context does not exist"
));
}
Ok(desc) => desc,
Err(e) => return Err(e),
}
}
};
let req_change_reason = req.change_reason.clone();
validate_change_reason(
&workspace_context,
&req_change_reason,
&mut db_conn,
&state.master_encryption_key,
)?;
let (put_response, version_id, webhook_future) = db_conn
.transaction::<_, superposition::AppError, _>(|transaction_conn| {
let put_response = operations::upsert(
req.into_inner(),
description,
transaction_conn,
true,
&user,
&workspace_context,
false,
&state.master_encryption_key,
)
.map_err(|err: superposition::AppError| {
log::error!("context put failed with error: {:?}", err);
err
})?;
let (version_id, future_webhook) = add_config_version(
&state,
tags,
req_change_reason.into(),
transaction_conn,
&workspace_context,
&put_response,
)?;
Ok((put_response, version_id, future_webhook))
})?;
let webhook_status = webhook_future.await;
let mut http_resp = if webhook_status {
HttpResponse::Ok()
} else {
HttpResponse::build(
actix_web::http::StatusCode::from_u16(512)
.unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR),
)
};
http_resp.insert_header((
AppHeader::XConfigVersion.to_string(),
version_id.to_string(),
));
#[cfg(feature = "high-performance-mode")]
put_config_in_redis(version_id, state, &workspace_context.schema_name, &mut conn)
.await?;
Ok(http_resp.json(put_response))
}
You will get an error saying :
lifetime may not live long enough
returning this value requires that `'1` must outlive `'2`
There was a problem hiding this comment.
The problem is that the future borrows transaction_conn, which only lives for the duration of the transaction closure — but we're trying to return the future out of the closure and .await it after the transaction completes.
We can't borrow transaction_conn in a future that outlives the transaction.
| db_conn: &mut DBConnection, | ||
| user: &User, | ||
| change_reason: &str, | ||
| ) -> bool { |
There was a problem hiding this comment.
Return result here, so you can throw 512 when it does error out
| let payload = json!({ | ||
| "change_reason": change_reason, | ||
| "config_version": version_id, | ||
| }); |
There was a problem hiding this comment.
Would it be better to also include the new config json? The entire CAC config?
There was a problem hiding this comment.
why would that be needed ?
There was a problem hiding this comment.
For an experiment webhook we send the experiment data, for this webhook it makes sense to send the current config data
crates/service_utils/src/helpers.rs
Outdated
| pub async fn fetch_webhook_by_event( | ||
| state: &Data<AppState>, | ||
| user: &User, | ||
| event: &WebhookEvent, | ||
| workspace_context: &WorkspaceContext, | ||
| ) -> result::Result<Webhook> { | ||
| let http_client = reqwest::Client::new(); | ||
| let url = format!("{}/webhook/event/{event}", state.cac_host); | ||
| let user_str = serde_json::to_string(user).map_err(|err| { | ||
| log::error!("Something went wrong, failed to stringify user data {err}"); | ||
| unexpected_error!( | ||
| "Something went wrong, failed to stringify user data {}", | ||
| err | ||
| ) | ||
| })?; | ||
|
|
||
| let headers_map = | ||
| construct_header_map(workspace_context, vec![("x-user", user_str)])?; | ||
|
|
||
| let response = http_client | ||
| .get(&url) | ||
| .headers(headers_map) | ||
| .header( | ||
| header::AUTHORIZATION, | ||
| format!("Internal {}", state.superposition_token), | ||
| ) | ||
| .send() | ||
| .await; | ||
|
|
||
| match response { | ||
| Ok(res) => { | ||
| if res.status() == 404 { | ||
| log::info!("No Webhook found for event: {}", event); | ||
| return Ok(Webhook::default()); | ||
| } | ||
| let webhook = res.json::<Webhook>().await.map_err(|err| { | ||
| log::error!("failed to parse Webhook response with error: {}", err); | ||
| unexpected_error!("Failed to parse Webhook.") | ||
| })?; | ||
| Ok(webhook) | ||
| } | ||
| Err(error) => { | ||
| log::error!("Failed to fetch Webhook with error: {:?}", error); | ||
| Err(unexpected_error!(error)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
you have moved the code here to service utils, make a straight away DB call, why bother having all this indirect calls, when its moved to service_utils
it just simplifies everything
crates/service_utils/src/helpers.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn construct_header_map( |
There was a problem hiding this comment.
| pub fn construct_header_map( | |
| pub fn construct_cac_headers_map( |
There was a problem hiding this comment.
not needed since this would in cac_api.rs
crates/service_utils/src/helpers.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn construct_header_map( |
There was a problem hiding this comment.
also, I think after changing fetch_webhook_by_event to a DB call instead of API call, this function will not be needed outside the scope of experimentation_platform crate
we can move it back then
| Ok(version_id) | ||
| } | ||
|
|
||
| pub async fn trigger_config_changed_webhook( |
There was a problem hiding this comment.
can we move both the trigger function to service_utils have them unified ?
will that possible ?
if not what is the blocker for this ?
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
from
381648f to
b76e2de
Compare
3 participants
Added ConfigChanged webhook event for cache invalidation
Introduces a new
ConfigChangedwebhook event type that fires whenever a config version is created — i.e., on any mutation to contexts, default configs, or dimensions.Motivation
Downstream consumers (e.g., Lambda functions) need to be notified when configuration changes occur so they can invalidate caches scoped to a specific workspace and organisation.
Changes
ConfigChangedvariant added toWebhookEventenumtrigger_config_changed_webhookhelper — async function that fires the webhook after DB transaction commitsadd_config_versionremains synchronous — called inside transactions; webhook dispatch happens after commitchange_reason(from the request) andconfig_versionWebhook payload
{ "event_info": { "webhook_event": "ConfigChanged", "workspace_id": "...", "organisation_id": "...", "config_version": "..." }, "payload": { "change_reason": "...", "config_version": 1234567890 } } <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configuration changes now trigger webhook notifications, enabling real-time integration with external systems when configurations are created, updated, deleted, or modified in bulk. * **Chores** * Refactored internal header construction logic for improved consistency and maintainability across modules. <!-- end of auto-generated comment: release notes by coderabbit.ai -->