feat: added ConfigChanged webhook event for config mutation notifications#877
feat: added ConfigChanged webhook event for config mutation notifications#877
Conversation
|
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 integrates webhook support for configuration mutations across context awareness config, default config, dimension, and experiment modules. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Handler
participant DB as Database
participant WebhookService as execute_webhook_call
participant WebhookAPI as External Webhook
participant Client
Client->>Handler: Mutation Request (create/update/delete)
activate Handler
Handler->>DB: Perform core operation
Handler->>DB: Compute version_id & config_version
Handler->>WebhookService: execute_webhook_call(payload, resource, version_id, ...)
activate WebhookService
WebhookService->>DB: Fetch webhook by event
WebhookService->>WebhookAPI: POST WebhookEventInfo
WebhookAPI-->>WebhookService: Response (success/failure)
alt Webhook Success
WebhookService-->>Handler: true
Handler->>DB: Update Redis (high-performance mode)
Handler-->>Client: 200/201/204 Response
else Webhook Failure
WebhookService-->>Handler: false
Handler-->>Client: 512 Internal Error
end
deactivate WebhookService
deactivate Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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.
| 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.
There was a problem hiding this comment.
Why do you need db conn with async stuff ?
The only async part is API call, which does not even require db conn
| 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
| 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
| } | ||
| } | ||
|
|
||
| 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
| } | ||
| } | ||
|
|
||
| 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
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
2 times, most recently
from
49b2aca to
33bae34
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/context_aware_config/src/helpers.rs (1)
199-216:⚠️ Potential issue | 🟠 MajorConfig hash is nondeterministic due to HashMap key ordering
json!(config).to_string()serializesHashMapfields (inConfig.overridesandConfig.dimensions) with non-deterministic key order, causing identical configs to yield different hashes across runs. Ifconfig_hashis ever used for deduplication or change detection, this will create false mismatches. Preserve JSON key order by convertingHashMapto a sorted structure before hashing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/context_aware_config/src/helpers.rs` around lines 199 - 216, In add_config_version, the config_hash is computed from json!(config).to_string(), but Config.overrides and Config.dimensions are HashMaps whose iteration order is nondeterministic; convert those maps (and any nested maps) to a deterministic ordering (e.g., BTreeMap or a recursive sort of serde_json::Value) before serializing so generate_cac's output yields stable JSON; then compute config_hash from the deterministically-ordered JSON and store that value in ConfigVersion.config_hash to avoid false mismatches during deduplication/change detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/context_aware_config/src/api/context/handlers.rs`:
- Around line 578-589: The current delete handler builds webhook_change_reason
as a raw string and passes it to execute_webhook_call, which causes the payload
to be serialized as a JSON string; change this to send a structured object
(e.g., a serde_json::Value or dedicated struct) containing explicit fields such
as context_id (version_id.to_string()) and change_reason (format!("Deleted
context by {}", user.username)), then pass that structured payload into
execute_webhook_call instead of &webhook_change_reason; update the call site in
the delete handler and ensure execute_webhook_call (and any callers like
create/update) expect/serialize the same structured payload for
Resource::Context so consumers can parse context_id and change_reason reliably.
In `@crates/context_aware_config/src/api/default_config/handlers.rs`:
- Around line 559-571: The delete webhook currently builds webhook_payload as
just { "key": key } which omits the required stable change_reason; update the
payload construction before calling execute_webhook_call so the delete sends a
structured object including both key and change_reason (for example { key,
change_reason }) similar to create/update paths, and pass that payload into
execute_webhook_call used with Resource::DefaultConfig and
WebhookEvent::ConfigChanged (keep the same version_id, workspace_context, state,
conn arguments).
- Around line 582-588: When webhook_status is false the handler returns an
HttpResponse::build(...) without preserving the X-Config-Version header; update
the failure branch so it reads the incoming X-Config-Version (e.g.
req.headers().get("X-Config-Version") or the local config_version variable used
by other handlers) and then call .insert_header(("X-Config-Version", value)) on
the HttpResponseBuilder before .finish() so the 512 response includes the same
X-Config-Version header as other handlers.
In `@crates/context_aware_config/src/api/dimension/handlers.rs`:
- Around line 672-680: The current call to execute_webhook_call passes
dimension_data (which contains the pre-delete change_reason) to notify webhooks;
create a delete-specific payload that contains the deletion change_reason and
the dimension identifier (instead of the old change_reason) and pass that
payload to execute_webhook_call for the delete path (still targeting
Resource::Dimension and the delete event), e.g. build a new payload object or
clone and overwrite change_reason/id before calling execute_webhook_call so
webhook consumers receive the deletion reason rather than the pre-delete reason.
In `@crates/service_utils/src/helpers.rs`:
- Around line 334-343: The current match on webhook_dsl.first::<Webhook>(conn)
treats every Err as "no webhook", causing non-NotFound DB errors to be ignored;
update the error arm to pattern-match Err(diesel::result::Error::NotFound) and
only in that case log "No webhook..." and return true, while for all other Err
variants log the actual error (including context: event,
workspace_context.schema_name) and return false; keep the Ok(webhook) branch
unchanged and continue using webhook_dsl, webhooks::events.contains(vec![event])
and first::<Webhook> as the lookup points.
---
Outside diff comments:
In `@crates/context_aware_config/src/helpers.rs`:
- Around line 199-216: In add_config_version, the config_hash is computed from
json!(config).to_string(), but Config.overrides and Config.dimensions are
HashMaps whose iteration order is nondeterministic; convert those maps (and any
nested maps) to a deterministic ordering (e.g., BTreeMap or a recursive sort of
serde_json::Value) before serializing so generate_cac's output yields stable
JSON; then compute config_hash from the deterministically-ordered JSON and store
that value in ConfigVersion.config_hash to avoid false mismatches during
deduplication/change detection.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/context_aware_config/src/api/context/handlers.rscrates/context_aware_config/src/api/default_config/handlers.rscrates/context_aware_config/src/api/dimension/handlers.rscrates/context_aware_config/src/helpers.rscrates/experimentation_platform/src/api/experiments/cac_api.rscrates/experimentation_platform/src/api/experiments/handlers.rscrates/experimentation_platform/src/api/experiments/helpers.rscrates/service_utils/src/helpers.rscrates/service_utils/src/middlewares/auth_z.rscrates/service_utils/src/middlewares/auth_z/authorization.rscrates/service_utils/src/middlewares/auth_z/no_auth.rscrates/service_utils/src/service/types.rscrates/superposition/src/main.rscrates/superposition_types/src/api/webhook.rscrates/superposition_types/src/database/models/others.rscrates/superposition_types/src/lib.rs
| let DbConnection(mut conn) = db_conn; | ||
| let webhook_change_reason = format!("Deleted context by {}", user.username); | ||
| let webhook_status = execute_webhook_call( | ||
| &webhook_change_reason, | ||
| Resource::Context, | ||
| &Some(version_id.to_string()), | ||
| &workspace_context, | ||
| WebhookEvent::ConfigChanged, | ||
| &state, | ||
| &mut conn, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
Delete webhook payload should be structured (not a raw string).
execute_webhook_call will serialize the string into a JSON string, which is inconsistent with create/update payloads and makes it hard for consumers to parse change_reason or context_id. Consider sending an object payload with explicit fields (e.g., { context_id, change_reason }).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/context/handlers.rs` around lines 578 -
589, The current delete handler builds webhook_change_reason as a raw string and
passes it to execute_webhook_call, which causes the payload to be serialized as
a JSON string; change this to send a structured object (e.g., a
serde_json::Value or dedicated struct) containing explicit fields such as
context_id (version_id.to_string()) and change_reason (format!("Deleted context
by {}", user.username)), then pass that structured payload into
execute_webhook_call instead of &webhook_change_reason; update the call site in
the delete handler and ensure execute_webhook_call (and any callers like
create/update) expect/serialize the same structured payload for
Resource::Context so consumers can parse context_id and change_reason reliably.
| let webhook_payload = serde_json::json!({ | ||
| "key": key, | ||
| }); // passing the deleted key here | ||
| let webhook_status = execute_webhook_call( | ||
| &webhook_payload, | ||
| Resource::DefaultConfig, | ||
| &Some(version_id.to_string()), | ||
| &workspace_context, | ||
| WebhookEvent::ConfigChanged, | ||
| &state, | ||
| &mut conn, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
Delete webhook payload misses a stable change_reason field.
Create/update send structured payloads with change_reason, but delete only sends { "key": ... }. If consumers expect change_reason (per PR objective), this breaks the contract. Consider sending a structured payload (e.g., { key, change_reason }) for deletes too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/default_config/handlers.rs` around lines
559 - 571, The delete webhook currently builds webhook_payload as just { "key":
key } which omits the required stable change_reason; update the payload
construction before calling execute_webhook_call so the delete sends a
structured object including both key and change_reason (for example { key,
change_reason }) similar to create/update paths, and pass that payload into
execute_webhook_call used with Resource::DefaultConfig and
WebhookEvent::ConfigChanged (keep the same version_id, workspace_context, state,
conn arguments).
| if !webhook_status { | ||
| return Ok(HttpResponse::build( | ||
| actix_web::http::StatusCode::from_u16(512) | ||
| .unwrap_or(actix_web::http::StatusCode::INTERNAL_SERVER_ERROR), | ||
| ) | ||
| .finish()); | ||
| } |
There was a problem hiding this comment.
Preserve X-Config-Version on webhook-failure responses.
On delete, the 512 path drops the header while other handlers always include it. Keeping it helps clients reconcile versions even on webhook failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/default_config/handlers.rs` around lines
582 - 588, When webhook_status is false the handler returns an
HttpResponse::build(...) without preserving the X-Config-Version header; update
the failure branch so it reads the incoming X-Config-Version (e.g.
req.headers().get("X-Config-Version") or the local config_version variable used
by other handlers) and then call .insert_header(("X-Config-Version", value)) on
the HttpResponseBuilder before .finish() so the 512 response includes the same
X-Config-Version header as other handlers.
| let webhook_status = execute_webhook_call( | ||
| &dimension_data, | ||
| Resource::Dimension, | ||
| &Some(version_id.to_string()), | ||
| &workspace_context, | ||
| WebhookEvent::ConfigChanged, | ||
| &state, | ||
| &mut conn, | ||
| ) |
There was a problem hiding this comment.
Delete webhook payload reflects the pre-delete change_reason.
dimension_data carries the old change_reason, not the delete action’s reason, so consumers will get misleading data. Consider constructing a delete-specific payload with the deletion change_reason and dimension identifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/context_aware_config/src/api/dimension/handlers.rs` around lines 672 -
680, The current call to execute_webhook_call passes dimension_data (which
contains the pre-delete change_reason) to notify webhooks; create a
delete-specific payload that contains the deletion change_reason and the
dimension identifier (instead of the old change_reason) and pass that payload to
execute_webhook_call for the delete path (still targeting Resource::Dimension
and the delete event), e.g. build a new payload object or clone and overwrite
change_reason/id before calling execute_webhook_call so webhook consumers
receive the deletion reason rather than the pre-delete reason.
| let webhook = match webhook_dsl | ||
| .filter(webhooks::events.contains(vec![event])) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .first::<Webhook>(conn) | ||
| { | ||
| Ok(webhook) => webhook, | ||
| Err(_) => { | ||
| log::info!("No webhook found for this event, skipping event: {}", event); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Diesel .first::<T> error NotFound diesel::result::Error rust
💡 Result:
In Diesel, .first::<T>(conn) (and .get_result::<T>(conn)) expects at least 1 row. If the query returns 0 rows, Diesel returns Err(diesel::result::Error::NotFound). (docs.rs)
Fix options
1) If “0 or 1 row” is valid, use .optional()
use diesel::OptionalExtension;
let user: Option<User> = users
.filter(users::id.eq(some_id))
.first::<User>(conn)
.optional()?; // Result<Option<User>, diesel::result::Error>.optional() converts Err(NotFound) into Ok(None). (stackoverflow.com)
2) If “0 rows” is an error, handle NotFound explicitly
match users.filter(...).first::<User>(conn) {
Ok(u) => Ok(u),
Err(diesel::result::Error::NotFound) => /* handle missing row */,
Err(e) => Err(e),
}NotFound is a dedicated diesel::result::Error variant. (docs.rs)
3) If you want “0..N rows”, use .load() instead
load::<T> returns Ok(Vec<T>) (empty vec if no rows), and does not treat “0 rows” as an error. (docs.rs)
Citations:
- 1: https://docs.rs/diesel/latest/diesel/result/enum.Error.html?utm_source=openai
- 2: https://stackoverflow.com/questions/46297867/how-do-i-get-an-optiont-instead-of-an-optionvect-from-a-diesel-query-which?utm_source=openai
- 3: https://docs.rs/diesel/latest/diesel/result/enum.Error.html?utm_source=openai
- 4: https://docs.rs/diesel/latest/diesel/result/enum.Error.html?utm_source=openai
🏁 Script executed:
cat -n crates/service_utils/src/helpers.rs | sed -n '330,350p'Repository: juspay/superposition
Length of output: 804
Don't treat all DB errors as "no webhook"—only skip on NotFound.
When a database error occurs (connection issues, permission errors, etc.), the current code returns true, causing callers to respond 200 OK when they should fail. In Diesel, .first::<T>() returns the specific variant Err(diesel::result::Error::NotFound) when no rows match. Only this case should log and return true; other errors must return false.
Proposed fix
- let webhook = match webhook_dsl
- .filter(webhooks::events.contains(vec![event]))
- .schema_name(&workspace_context.schema_name)
- .first::<Webhook>(conn)
- {
- Ok(webhook) => webhook,
- Err(_) => {
- log::info!("No webhook found for this event, skipping event: {}", event);
- return true;
- }
- };
+ let webhook = match webhook_dsl
+ .filter(webhooks::events.contains(vec![event]))
+ .schema_name(&workspace_context.schema_name)
+ .first::<Webhook>(conn)
+ {
+ Ok(webhook) => webhook,
+ Err(diesel::result::Error::NotFound) => {
+ log::info!("No webhook found for this event, skipping event: {}", event);
+ return true;
+ }
+ Err(e) => {
+ log::error!("Failed to fetch webhook for event {}: {}", event, e);
+ return false;
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let webhook = match webhook_dsl | |
| .filter(webhooks::events.contains(vec![event])) | |
| .schema_name(&workspace_context.schema_name) | |
| .first::<Webhook>(conn) | |
| { | |
| Ok(webhook) => webhook, | |
| Err(_) => { | |
| log::info!("No webhook found for this event, skipping event: {}", event); | |
| return true; | |
| } | |
| let webhook = match webhook_dsl | |
| .filter(webhooks::events.contains(vec![event])) | |
| .schema_name(&workspace_context.schema_name) | |
| .first::<Webhook>(conn) | |
| { | |
| Ok(webhook) => webhook, | |
| Err(diesel::result::Error::NotFound) => { | |
| log::info!("No webhook found for this event, skipping event: {}", event); | |
| return true; | |
| } | |
| Err(e) => { | |
| log::error!("Failed to fetch webhook for event {}: {}", event, e); | |
| return false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/service_utils/src/helpers.rs` around lines 334 - 343, The current
match on webhook_dsl.first::<Webhook>(conn) treats every Err as "no webhook",
causing non-NotFound DB errors to be ignored; update the error arm to
pattern-match Err(diesel::result::Error::NotFound) and only in that case log "No
webhook..." and return true, while for all other Err variants log the actual
error (including context: event, workspace_context.schema_name) and return
false; keep the Ok(webhook) branch unchanged and continue using webhook_dsl,
webhooks::events.contains(vec![event]) and first::<Webhook> as the lookup
points.
ayushjain17
left a comment
ayushjain17
left a comment
There was a problem hiding this comment.
same set of commets in all effected files
| } | ||
|
|
||
| #[derive(Deref, DerefMut, Clone, Debug)] | ||
| #[derive(Deref, DerefMut, Clone, Debug, Serialize, Deserialize)] |
| let webhook = match webhook_dsl | ||
| .filter(webhooks::events.contains(vec![event])) | ||
| .schema_name(&workspace_context.schema_name) | ||
| .first::<Webhook>(conn) | ||
| { | ||
| Ok(webhook) => webhook, | ||
| Err(_) => { | ||
| log::info!("No webhook found for this event, skipping event: {}", event); | ||
| return true; | ||
| } | ||
| }; |
There was a problem hiding this comment.
| let webhook = match webhook_dsl | |
| .filter(webhooks::events.contains(vec![event])) | |
| .schema_name(&workspace_context.schema_name) | |
| .first::<Webhook>(conn) | |
| { | |
| Ok(webhook) => webhook, | |
| Err(_) => { | |
| log::info!("No webhook found for this event, skipping event: {}", event); | |
| return true; | |
| } | |
| }; | |
| let webhook = match webhook_dsl | |
| .filter(webhooks::events.contains(vec![event])) | |
| .schema_name(&workspace_context.schema_name) | |
| .get_result::<Webhook>(conn) | |
| .optional() | |
| { | |
| Ok(webhook) => { | |
| let Some(webhook) = webhook else { | |
| log::info!("No webhook found for this event, skipping event: {}", event); | |
| return true; | |
| }; | |
| webhook | |
| } | |
| Err(e) => { | |
| log::error!("Failed to fetch webhook for event: {}", e); | |
| return false; | |
| } | |
| }; |
| #[cfg(feature = "high-performance-mode")] | ||
| put_config_in_redis( | ||
| config_version_id, | ||
| state, | ||
| &workspace_context.schema_name, | ||
| &mut conn, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
this should happen before webhook trigger
| #[cfg(feature = "high-performance-mode")] | ||
| put_config_in_redis(version_id, state, &workspace_context.schema_name, &mut conn) | ||
| .await?; |
There was a problem hiding this comment.
same here as well as all other places
| let DbConnection(mut conn) = db_conn; | ||
| let webhook_change_reason = format!("Deleted context by {}", user.username); | ||
| let webhook_status = execute_webhook_call( | ||
| &webhook_change_reason, |
There was a problem hiding this comment.
- there should be a proper payload here, right ?
- can we verify the payload being passed across ?
- also, in cases for delete what should we such that they can differentiate that the trigger is for a delete for updates and creates, the resource data is self sufficient, but for delete, the intention gets grayed out
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
2 times, most recently
from
f0d646e to
3fab366
Compare
| let is_mandatory = workspace_context | ||
| .clone() | ||
| .settings | ||
| .mandatory_dimensions | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
| let is_mandatory = workspace_context | |
| .clone() | |
| .settings | |
| .mandatory_dimensions | |
| .unwrap_or_default() | |
| let is_mandatory = workspace_context | |
| .settings | |
| .mandatory_dimensions | |
| .clone() | |
| .unwrap_or_default() |
| let is_mandatory = workspace_context | ||
| .clone() | ||
| .settings | ||
| .mandatory_dimensions | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
| let is_mandatory = workspace_context | |
| .clone() | |
| .settings | |
| .mandatory_dimensions | |
| .unwrap_or_default() | |
| let is_mandatory = workspace_context | |
| .settings | |
| .mandatory_dimensions | |
| .clone() | |
| .unwrap_or_default() |
| })?; | ||
|
|
||
| let data = WebhookData { | ||
| payload: result.clone(), |
There was a problem hiding this comment.
| payload: result.clone(), | |
| payload: &result |
| let webhook_status = | ||
| execute_webhook_call(data, &workspace_context, &state, &mut conn).await; | ||
|
|
There was a problem hiding this comment.
this is still not fixed here, this is being called before redis update
| })?; | ||
|
|
||
| let data = WebhookData { | ||
| payload: inserted_dimension.clone(), |
There was a problem hiding this comment.
| payload: inserted_dimension.clone(), | |
| payload: &inserted_dimension |
| }; | ||
|
|
||
| let webhook_status = | ||
| execute_webhook_call(data, &workspace_context, &state, &mut conn).await; |
| }; | ||
|
|
||
| let webhook_status = | ||
| execute_webhook_call(data, &workspace_context, &state, &mut conn).await; |
| }; | ||
|
|
||
| let webhook_status = | ||
| execute_webhook_call(data, &workspace_context, &state, &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), | ||
| ) | ||
| .insert_header(( | ||
| AppHeader::XConfigVersion.to_string(), | ||
| version_id.to_string(), | ||
| )) | ||
| .finish()); | ||
| } | ||
| resp | ||
| } else { | ||
| Err(bad_argument!( | ||
| "Given key already in use in contexts: {}", | ||
| context_ids.join(",") | ||
| )) | ||
| } | ||
|
|
||
| Ok(HttpResponse::NoContent() | ||
| .insert_header(( | ||
| AppHeader::XConfigVersion.to_string(), | ||
| version_id.to_string(), | ||
| )) | ||
| .finish()) |
There was a problem hiding this comment.
can we keep the return statement similar like other places, the same format of if-else
| version_id.to_string(), | ||
| )) | ||
| .finish()) | ||
| Ok((version_id, default_config)) |
There was a problem hiding this comment.
this is a very weird code, version_id is a mutable value which is being updated as well as returned
can only return from here and not have it as a mutable variable as well
There was a problem hiding this comment.
Again , this also existed before. We can do the cleanup though.
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
4 times, most recently
from
3e3747b to
4ecc5cc
Compare
sauraww
force-pushed
the
feat/config-change-webhook-event
branch
from
4ecc5cc to
e2a759b
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 with resource type information. * Configuration versions now include a content hash for integrity tracking. * **Refactor** * Simplified header construction in API calls using unified workspace context handling. * Streamlined webhook invocation patterns across configuration handlers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->