mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-18 16:02:10 +00:00
fix(forks): keep trigger/schedule operational state owned by the parent - WIN-2019 (#9476)
* fix(forks): defer trigger/schedule state to parent for clean git merge Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(forks): read parent trigger/schedule state on non-RLS pool for complete substitution Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(forks): read schedule fork-ness on non-RLS pool; clarify mutator-rule wording Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -242,6 +242,23 @@ async fn create_schedule(
|
||||
|
||||
let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?;
|
||||
|
||||
// A git-sync/merge/create write into a fork never sets operational state:
|
||||
// force `enabled = false` so a cloned / synced / merged / UI-created schedule
|
||||
// can't fire alongside the parent's. The fork owner re-enables locally via
|
||||
// `setenabled`. Schedule analog of the trigger rule in
|
||||
// `windmill-trigger::handler::workspace_is_fork`; the read half (parent-value
|
||||
// substitution on fork export) lives in `workspaces_export.rs`. Read fork-ness
|
||||
// on the non-RLS `db` pool (like the other two sites) so the determination is
|
||||
// complete regardless of the caller's folder perms.
|
||||
let target_is_fork: bool = sqlx::query_scalar!(
|
||||
"SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1",
|
||||
w_id
|
||||
)
|
||||
.fetch_optional(&db)
|
||||
.await?
|
||||
.flatten()
|
||||
.unwrap_or(false);
|
||||
|
||||
// Check schedule for error
|
||||
ScheduleType::from_str(&ns.schedule, ns.cron_version.as_deref(), true)?;
|
||||
|
||||
@@ -341,7 +358,12 @@ async fn create_schedule(
|
||||
// flows (CLI merge, UI merge, `wmill push` of a fork tarball) — which
|
||||
// either send the source's actual flag (create case) or omit `enabled`
|
||||
// entirely (update case, where `EditSchedule` lacks the field).
|
||||
ns.enabled.unwrap_or(true),
|
||||
// A write into a fork always lands disabled regardless of the request.
|
||||
if target_is_fork {
|
||||
false
|
||||
} else {
|
||||
ns.enabled.unwrap_or(true)
|
||||
},
|
||||
resolved_email,
|
||||
resolved_permissioned_as,
|
||||
ns.on_failure,
|
||||
@@ -413,7 +435,7 @@ async fn create_schedule(
|
||||
.await?;
|
||||
}
|
||||
|
||||
if ns.enabled.unwrap_or(true) {
|
||||
if !target_is_fork && ns.enabled.unwrap_or(true) {
|
||||
tx = push_scheduled_job(&db, tx, &schedule, Some(&authed.clone().into()), None).await?
|
||||
}
|
||||
tx.commit().await?;
|
||||
|
||||
@@ -125,10 +125,18 @@ pub fn is_none_or_false(val: &Option<bool>) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns the keys to strip from trigger/schedule serialization when the
|
||||
/// source workspace is a fork. Stripping these keys avoids propagating
|
||||
/// fork-local operational state (enabled flag, runtime listener identifiers)
|
||||
/// back to the parent workspace through the git-sync round-trip.
|
||||
/// A fork's git-sync export rewrites each trigger's `mode` (and each schedule's
|
||||
/// `enabled`) to the *parent* workspace's value, instead of emitting the fork's
|
||||
/// own (clone-disabled / locally-toggled) state. This keeps the fork's synced
|
||||
/// file byte-identical to the parent on the operational-state field, so a
|
||||
/// normal-git PR merge has nothing to resolve — no dropped `mode:` line, no
|
||||
/// flipped parent trigger. Fork-only paths (absent from the parent) keep the
|
||||
/// fork's own value: there's no parent state to defer to, so the trigger lands
|
||||
/// with whatever the fork creator set. The write half of the same rule lives in
|
||||
/// `windmill-trigger::handler::workspace_is_fork`.
|
||||
///
|
||||
/// Maps trigger `path` → parent `mode` (as the lowercase enum text that matches
|
||||
/// `TriggerMode`'s serde representation). Empty when not a fork.
|
||||
#[cfg(any(
|
||||
feature = "http_trigger",
|
||||
feature = "websocket",
|
||||
@@ -148,20 +156,78 @@ pub fn is_none_or_false(val: &Option<bool>) -> bool {
|
||||
feature = "private"
|
||||
)
|
||||
))]
|
||||
fn fork_trigger_ignore_keys(is_fork: bool) -> Option<Vec<&'static str>> {
|
||||
if is_fork {
|
||||
Some(vec!["mode", "enabled"])
|
||||
} else {
|
||||
None
|
||||
}
|
||||
async fn fork_parent_trigger_modes(
|
||||
db: &DB,
|
||||
table_name: &str,
|
||||
parent_workspace_id: Option<&str>,
|
||||
) -> Result<HashMap<String, String>> {
|
||||
let Some(parent) = parent_workspace_id else {
|
||||
return Ok(HashMap::new());
|
||||
};
|
||||
// Read the parent's rows on the non-RLS pool (like `workspace_is_fork`): the
|
||||
// substitution must be complete regardless of the exporter's folder perms,
|
||||
// otherwise a parent path the exporter can't read would fall back to the
|
||||
// fork's own value and silently re-introduce the divergence we're fixing.
|
||||
// No leak: only values for paths the fork already has (it's a clone) are used.
|
||||
// SAFETY: `table_name` is a compile-time `TriggerCrud::TABLE_NAME` constant.
|
||||
let rows: Vec<(String, String)> = sqlx::query_as(&format!(
|
||||
"SELECT path, mode::text FROM {} WHERE workspace_id = $1",
|
||||
table_name
|
||||
))
|
||||
.bind(parent)
|
||||
.fetch_all(db)
|
||||
.await?;
|
||||
Ok(rows.into_iter().collect())
|
||||
}
|
||||
|
||||
fn fork_schedule_ignore_keys(is_fork: bool) -> Option<Vec<&'static str>> {
|
||||
if is_fork {
|
||||
Some(vec!["enabled"])
|
||||
} else {
|
||||
None
|
||||
}
|
||||
/// Build the `{ "mode": <parent value> }` override for a single trigger, or
|
||||
/// `None` (keep the fork's own value) when the path is fork-only.
|
||||
#[cfg(any(
|
||||
feature = "http_trigger",
|
||||
feature = "websocket",
|
||||
feature = "postgres_trigger",
|
||||
feature = "mqtt_trigger",
|
||||
feature = "native_trigger",
|
||||
all(
|
||||
feature = "enterprise",
|
||||
any(
|
||||
feature = "kafka",
|
||||
feature = "sqs_trigger",
|
||||
feature = "gcp_trigger",
|
||||
feature = "azure_trigger",
|
||||
feature = "nats",
|
||||
feature = "smtp",
|
||||
),
|
||||
feature = "private"
|
||||
)
|
||||
))]
|
||||
fn trigger_mode_override(
|
||||
parent_modes: &HashMap<String, String>,
|
||||
path: &str,
|
||||
) -> Option<serde_json::Map<String, Value>> {
|
||||
parent_modes.get(path).map(|mode| {
|
||||
let mut o = serde_json::Map::new();
|
||||
o.insert("mode".to_string(), Value::String(mode.clone()));
|
||||
o
|
||||
})
|
||||
}
|
||||
|
||||
/// Schedule analog of [`fork_parent_trigger_modes`]: maps schedule `path` →
|
||||
/// parent `enabled`. Empty when not a fork.
|
||||
async fn fork_parent_schedule_enabled(
|
||||
db: &DB,
|
||||
parent_workspace_id: Option<&str>,
|
||||
) -> Result<HashMap<String, bool>> {
|
||||
let Some(parent) = parent_workspace_id else {
|
||||
return Ok(HashMap::new());
|
||||
};
|
||||
// Non-RLS pool, same rationale as `fork_parent_trigger_modes`.
|
||||
let rows: Vec<(String, bool)> =
|
||||
sqlx::query_as("SELECT path, enabled FROM schedule WHERE workspace_id = $1")
|
||||
.bind(parent)
|
||||
.fetch_all(db)
|
||||
.await?;
|
||||
Ok(rows.into_iter().collect())
|
||||
}
|
||||
|
||||
enum ArchiveImpl {
|
||||
@@ -260,6 +326,25 @@ pub fn to_string_without_metadata<T>(
|
||||
extra_perms: ExtraPermsBehavior,
|
||||
ignore_keys: Option<Vec<&str>>,
|
||||
) -> Result<String>
|
||||
where
|
||||
T: ?Sized + Serialize,
|
||||
{
|
||||
to_string_without_metadata_inner(value, extra_perms, ignore_keys, None)
|
||||
}
|
||||
|
||||
/// Like [`to_string_without_metadata`] but additionally lets the caller
|
||||
/// override top-level keys after stripping. Used for fork trigger/schedule
|
||||
/// exports, where `mode`/`enabled` is rewritten to the *parent* workspace's
|
||||
/// value so the fork's synced file is byte-identical to the parent on those
|
||||
/// fields — a clean 3-way git merge instead of a dropped line. See the write
|
||||
/// half of the rule in `windmill-trigger::handler::workspace_is_fork`.
|
||||
#[inline]
|
||||
pub fn to_string_without_metadata_inner<T>(
|
||||
value: &T,
|
||||
extra_perms: ExtraPermsBehavior,
|
||||
ignore_keys: Option<Vec<&str>>,
|
||||
overrides: Option<&serde_json::Map<String, Value>>,
|
||||
) -> Result<String>
|
||||
where
|
||||
T: ?Sized + Serialize,
|
||||
{
|
||||
@@ -324,6 +409,12 @@ where
|
||||
obj.remove("default_permissioned_as");
|
||||
}
|
||||
|
||||
if let Some(overrides) = overrides {
|
||||
for (k, v) in overrides {
|
||||
obj.insert(k.clone(), v.clone());
|
||||
}
|
||||
}
|
||||
|
||||
serde_json::to_string_pretty(&obj).ok()
|
||||
})
|
||||
.flatten()
|
||||
@@ -504,18 +595,18 @@ pub(crate) async fn tarball_workspace(
|
||||
|
||||
let mut tx = user_db.begin(&authed).await?;
|
||||
|
||||
// Source-of-truth check for fork-ness: the workspace's parent_workspace_id
|
||||
// column. The wm-fork-* prefix is a creation-time naming convention that
|
||||
// could in principle drift (rename, manual SQL); the column is the
|
||||
// contract that matches what the conflict-warning gates read.
|
||||
let is_fork: bool = sqlx::query_scalar!(
|
||||
"SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1",
|
||||
&w_id
|
||||
// Source-of-truth for fork-ness: the workspace's parent_workspace_id column.
|
||||
// The wm-fork-* prefix is a creation-time naming convention that could in
|
||||
// principle drift (rename, manual SQL); the column is the contract that
|
||||
// matches what the conflict-warning gates read. The id is also the workspace
|
||||
// whose trigger `mode` / schedule `enabled` a fork export defers to.
|
||||
let parent_workspace_id: Option<String> = sqlx::query_scalar::<_, Option<String>>(
|
||||
"SELECT parent_workspace_id FROM workspace WHERE id = $1",
|
||||
)
|
||||
.bind(&w_id)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?
|
||||
.flatten()
|
||||
.unwrap_or(false);
|
||||
.flatten();
|
||||
|
||||
let tmp_dir = TempDir::new_in(&*WINDMILL_DIR)?;
|
||||
|
||||
@@ -799,12 +890,21 @@ pub(crate) async fn tarball_workspace(
|
||||
.fetch_all(&mut *tx)
|
||||
.await?;
|
||||
|
||||
let schedule_ignore_keys = fork_schedule_ignore_keys(is_fork);
|
||||
// For a fork, defer each schedule's `enabled` to the parent so the
|
||||
// synced file matches the parent and the merge doesn't flip it.
|
||||
let parent_enabled =
|
||||
fork_parent_schedule_enabled(&db, parent_workspace_id.as_deref()).await?;
|
||||
for schedule in schedules {
|
||||
let app_str = &to_string_without_metadata(
|
||||
let enabled_override = parent_enabled.get(&schedule.path).map(|enabled| {
|
||||
let mut o = serde_json::Map::new();
|
||||
o.insert("enabled".to_string(), Value::Bool(*enabled));
|
||||
o
|
||||
});
|
||||
let app_str = &to_string_without_metadata_inner(
|
||||
&schedule,
|
||||
ExtraPermsBehavior::Drop,
|
||||
schedule_ignore_keys.clone(),
|
||||
None,
|
||||
enabled_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -814,38 +914,25 @@ pub(crate) async fn tarball_workspace(
|
||||
}
|
||||
|
||||
if include_triggers.unwrap_or(false) {
|
||||
#[cfg(any(
|
||||
feature = "http_trigger",
|
||||
feature = "websocket",
|
||||
feature = "postgres_trigger",
|
||||
feature = "mqtt_trigger",
|
||||
feature = "native_trigger",
|
||||
all(
|
||||
feature = "enterprise",
|
||||
any(
|
||||
feature = "kafka",
|
||||
feature = "sqs_trigger",
|
||||
feature = "gcp_trigger",
|
||||
feature = "azure_trigger",
|
||||
feature = "nats",
|
||||
feature = "smtp",
|
||||
),
|
||||
feature = "private"
|
||||
)
|
||||
))]
|
||||
let trigger_ignore_keys = fork_trigger_ignore_keys(is_fork);
|
||||
|
||||
#[cfg(feature = "http_trigger")]
|
||||
{
|
||||
use crate::triggers::http::HttpTrigger;
|
||||
let handler = HttpTrigger;
|
||||
let http_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<HttpTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in http_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -862,12 +949,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::websocket::WebsocketTrigger;
|
||||
let handler = WebsocketTrigger;
|
||||
let websocket_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<WebsocketTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in websocket_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -884,12 +979,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::kafka::KafkaTrigger;
|
||||
let handler = KafkaTrigger;
|
||||
let kafka_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<KafkaTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in kafka_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -906,12 +1009,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::sqs::SqsTrigger;
|
||||
let handler = SqsTrigger;
|
||||
let sqs_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<SqsTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in sqs_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -928,12 +1039,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::gcp::GcpTrigger;
|
||||
let handler = GcpTrigger;
|
||||
let gcp_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<GcpTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in gcp_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -950,12 +1069,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::azure::AzureTrigger;
|
||||
let handler = AzureTrigger;
|
||||
let azure_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<AzureTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in azure_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -972,12 +1099,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::nats::NatsTrigger;
|
||||
let handler = NatsTrigger;
|
||||
let nats_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<NatsTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in nats_triggers {
|
||||
let trigger_str: &String = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str: &String = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -994,12 +1129,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::postgres::PostgresTrigger;
|
||||
let handler = PostgresTrigger;
|
||||
let postgres_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<PostgresTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in postgres_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -1016,12 +1159,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::mqtt::MqttTrigger;
|
||||
let handler = MqttTrigger;
|
||||
let mqtt_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<MqttTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in mqtt_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -1038,12 +1189,20 @@ pub(crate) async fn tarball_workspace(
|
||||
use crate::triggers::email::EmailTrigger;
|
||||
let handler = EmailTrigger;
|
||||
let email_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?;
|
||||
let parent_modes = fork_parent_trigger_modes(
|
||||
&db,
|
||||
<EmailTrigger as TriggerCrud>::TABLE_NAME,
|
||||
parent_workspace_id.as_deref(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
for trigger in email_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path);
|
||||
let trigger_str = &to_string_without_metadata_inner(
|
||||
&trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
trigger_ignore_keys.clone(),
|
||||
None,
|
||||
mode_override.as_ref(),
|
||||
)
|
||||
.unwrap();
|
||||
archive
|
||||
@@ -1065,10 +1224,12 @@ pub(crate) async fn tarball_workspace(
|
||||
list_native_triggers(&mut *tx, &w_id, service_name, None, None, None, None)
|
||||
.await?;
|
||||
|
||||
let mut native_ignore_keys = vec!["webhook_token_hash"];
|
||||
if let Some(ref extra) = trigger_ignore_keys {
|
||||
native_ignore_keys.extend_from_slice(extra);
|
||||
}
|
||||
// Native triggers (Nextcloud, Google Drive, GitHub) are never
|
||||
// cloned into a fork — a fork only has one if its owner created
|
||||
// it there, so it's always "fork-only" and keeps its own mode.
|
||||
// No parent-value substitution applies; we only strip the
|
||||
// webhook token hash.
|
||||
let native_ignore_keys = vec!["webhook_token_hash"];
|
||||
|
||||
for trigger in native_triggers {
|
||||
let trigger_str = &to_string_without_metadata(
|
||||
@@ -1359,3 +1520,67 @@ pub(crate) async fn tarball_workspace(
|
||||
];
|
||||
Ok((headers, body))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod fork_export_tests {
|
||||
use super::*;
|
||||
use serde_json::json;
|
||||
|
||||
/// A fork export rewrites `mode` to the parent's value: the serialized file
|
||||
/// carries the parent's state (`enabled`), not the fork's clone-disabled DB
|
||||
/// value — so a normal-git merge sees no change on that line.
|
||||
#[test]
|
||||
fn override_substitutes_parent_mode() {
|
||||
let fork_trigger = json!({
|
||||
"path": "f/triggers/x",
|
||||
"script_path": "f/scripts/x",
|
||||
"mode": "disabled", // fork's local (clone-disabled) state
|
||||
"is_flow": false,
|
||||
});
|
||||
let mut overrides = serde_json::Map::new();
|
||||
overrides.insert("mode".to_string(), Value::String("enabled".to_string()));
|
||||
|
||||
let out = to_string_without_metadata_inner(
|
||||
&fork_trigger,
|
||||
ExtraPermsBehavior::Drop,
|
||||
None,
|
||||
Some(&overrides),
|
||||
)
|
||||
.unwrap();
|
||||
let parsed: Value = serde_json::from_str(&out).unwrap();
|
||||
|
||||
assert_eq!(parsed["mode"], json!("enabled"), "parent mode substituted");
|
||||
// `path` is in the metadata strip list, so it should be removed.
|
||||
assert!(parsed.get("path").is_none());
|
||||
}
|
||||
|
||||
/// A fork-only trigger (no parent counterpart, so no override) keeps the
|
||||
/// fork creator's chosen state.
|
||||
#[test]
|
||||
fn no_override_keeps_fork_value() {
|
||||
let fork_only = json!({ "mode": "enabled", "script_path": "f/scripts/x" });
|
||||
let out =
|
||||
to_string_without_metadata_inner(&fork_only, ExtraPermsBehavior::Drop, None, None)
|
||||
.unwrap();
|
||||
let parsed: Value = serde_json::from_str(&out).unwrap();
|
||||
assert_eq!(parsed["mode"], json!("enabled"));
|
||||
}
|
||||
|
||||
/// `trigger_mode_override` builds an override only when the parent has the
|
||||
/// path; fork-only paths return `None` (keep the fork's own value).
|
||||
#[cfg(feature = "http_trigger")]
|
||||
#[test]
|
||||
fn trigger_mode_override_defers_to_parent_or_self() {
|
||||
let mut parent_modes = HashMap::new();
|
||||
parent_modes.insert("f/triggers/shared".to_string(), "enabled".to_string());
|
||||
|
||||
let shared = trigger_mode_override(&parent_modes, "f/triggers/shared");
|
||||
assert_eq!(
|
||||
shared.as_ref().and_then(|o| o.get("mode")),
|
||||
Some(&Value::String("enabled".to_string())),
|
||||
);
|
||||
|
||||
// Fork-only path: no parent entry → no override → keep fork's own value.
|
||||
assert!(trigger_mode_override(&parent_modes, "f/triggers/fork_only").is_none());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -32,6 +32,27 @@ use std::sync::Arc;
|
||||
use windmill_audit::{audit_oss::audit_log, ActionKind};
|
||||
use windmill_git_sync::handle_deployment_metadata;
|
||||
|
||||
/// True when the workspace is a fork (`parent_workspace_id IS NOT NULL`).
|
||||
///
|
||||
/// Operational state (`mode`) belongs to the parent workspace: a git-sync /
|
||||
/// merge / clone / UI-create write into a fork must never set it. On create we
|
||||
/// force `disabled` so a fork trigger can't compete with the parent's listener;
|
||||
/// on update we preserve the fork's existing value. `setmode` is the intended
|
||||
/// explicit mutator of a fork's mode (and carries its own conflict warning) —
|
||||
/// runtime error handling may still auto-disable an errored trigger, which is
|
||||
/// orthogonal to this rule. This is the write half whose read half lives in
|
||||
/// `workspaces_export.rs` (parent-value substitution on fork export), and it is
|
||||
/// the single authority shared by both the git-sync round-trip and the in-app
|
||||
/// compare-workspaces merge.
|
||||
async fn workspace_is_fork(db: &DB, workspace_id: &str) -> Result<bool> {
|
||||
let is_fork: Option<bool> =
|
||||
sqlx::query_scalar("SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1")
|
||||
.bind(workspace_id)
|
||||
.fetch_optional(db)
|
||||
.await?;
|
||||
Ok(is_fork.unwrap_or(false))
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
pub trait TriggerCrud: Send + Sync + 'static {
|
||||
type Trigger: Serialize
|
||||
@@ -440,6 +461,13 @@ async fn create_trigger<T: TriggerCrud>(
|
||||
|
||||
let mut tx = user_db.begin(&authed).await?;
|
||||
|
||||
// Writing into a fork never sets operational state: force `disabled` so a
|
||||
// cloned / synced / merged / UI-created trigger can't compete with the
|
||||
// parent's listener. The fork owner re-enables locally via `setmode`.
|
||||
if workspace_is_fork(&db, &workspace_id).await? {
|
||||
new_trigger.base.set_mode(TriggerMode::Disabled);
|
||||
}
|
||||
|
||||
let new_path = new_trigger.base.path.clone();
|
||||
let labels = new_trigger.base.labels.clone();
|
||||
|
||||
@@ -591,11 +619,16 @@ async fn update_trigger<T: TriggerCrud>(
|
||||
|
||||
let mut tx = user_db.begin(&authed).await?;
|
||||
|
||||
// When the request omits `mode`/`enabled`, preserve the existing DB value
|
||||
// instead of falling back to the BaseTriggerData default (Enabled). This
|
||||
// keeps fork→parent git-sync round-trips from flipping the parent's
|
||||
// operational state — see fork_trigger_ignore_keys in workspaces_export.rs.
|
||||
if edit_trigger.base.is_mode_unspecified() {
|
||||
// Preserve the existing DB `mode` instead of writing the incoming value
|
||||
// when either:
|
||||
// * the target is a fork — a fork's operational state is fork-local and
|
||||
// is never set through a git-sync/merge write (only via `setmode`); or
|
||||
// * the request omits `mode`/`enabled` (legacy clients / YAML round-trip),
|
||||
// where falling back to the BaseTriggerData default (Enabled) would flip
|
||||
// the parent on a fork→parent merge.
|
||||
// Read half of the rule: parent-value substitution on fork export in
|
||||
// workspaces_export.rs.
|
||||
if workspace_is_fork(&db, &workspace_id).await? || edit_trigger.base.is_mode_unspecified() {
|
||||
let existing_mode: Option<TriggerMode> = sqlx::query_scalar(&format!(
|
||||
"SELECT mode FROM {} WHERE workspace_id = $1 AND path = $2",
|
||||
T::TABLE_NAME
|
||||
|
||||
+58
-31
@@ -51,27 +51,53 @@ parent and incoming mail would be delivered arbitrarily. The clone filter
|
||||
copies email triggers only when `workspaced_local_part IS TRUE` (or
|
||||
`CLOUD_HOSTED`, since cloud scopes email lookup by `workspace_id` natively).
|
||||
|
||||
## Merge-direction filter (always on)
|
||||
## Operational state is owned by the parent
|
||||
|
||||
Whenever the source workspace has `parent_workspace_id IS NOT NULL` (i.e.
|
||||
it's a fork), the tarball export at `/api/w/{workspace}/workspaces/tarball`
|
||||
strips fork-local fields:
|
||||
The rule that makes both the normal-git PR merge and the in-app merge behave:
|
||||
|
||||
- `mode` from every `*_trigger` row
|
||||
- `enabled` from every `schedule` row
|
||||
> **A trigger's `mode` (and a schedule's `enabled`) belongs to the parent
|
||||
> workspace. Git-sync *reads* the parent's value into a fork's synced file and
|
||||
> *never writes* a fork's value back. No git-sync / merge / create / update
|
||||
> write sets a fork's operational state; the `setmode` / `setenabled` endpoint
|
||||
> is the intended explicit mutator.**
|
||||
|
||||
The fork-detection key is the column, not the `wm-fork-*` naming convention,
|
||||
so it stays consistent with the conflict-warning gates in `set_trigger_mode`
|
||||
and `set_schedule_enabled` and survives any future ID rename.
|
||||
(Runtime error handling can still auto-disable an errored trigger or schedule —
|
||||
that's orthogonal to this rule, which governs git-sync/merge/create/update
|
||||
writes.)
|
||||
|
||||
The trigger update handler complements this: when an incoming `update_trigger`
|
||||
request omits both `mode` and `enabled`, the existing DB value is preserved
|
||||
instead of falling back to the BaseTriggerData default of `Enabled`. This
|
||||
means the fork→parent merge cannot flip the parent's operational state, even
|
||||
if the fork has an explicit (locally-disabled) state for that path.
|
||||
This is enforced in two halves, keyed off `parent_workspace_id IS NOT NULL`
|
||||
(the column, not the `wm-fork-*` naming convention — it stays consistent with
|
||||
the conflict-warning gates and survives any future ID rename):
|
||||
|
||||
The schedule `EditSchedule` payload already lacks an `enabled` field, so its
|
||||
update path is naturally safe.
|
||||
**Read half — parent-value substitution on export.** When the source workspace
|
||||
is a fork, the tarball export at `/api/w/{workspace}/workspaces/tarball`
|
||||
rewrites each trigger's `mode` (and each schedule's `enabled`) to the
|
||||
*parent's* value for the same path, looked up at export time. A fork-only path
|
||||
(absent from the parent) keeps the fork's own value — there's no parent state
|
||||
to defer to, so it lands with whatever the fork creator set.
|
||||
|
||||
The earlier design *stripped* these fields instead. That broke a normal-git PR
|
||||
merge: the parent branch (and the merge base) carries the line, the fork branch
|
||||
dropped it, so the 3-way merge either silently deleted `mode`/`enabled` from
|
||||
the parent — corrupting the source of truth — or conflicted outright when the
|
||||
parent had also edited it. Substituting the parent's value makes the fork's
|
||||
file byte-identical to the parent on that field, so the merge has nothing to
|
||||
resolve.
|
||||
|
||||
**Write half — fork writes never set operational state.** A write into a fork
|
||||
(git-sync push, merge deploy, clone, or a plain UI create) must not set the
|
||||
state, otherwise pulling the substituted parent value straight back into the
|
||||
fork would re-enable it. So `create_trigger`/`create_schedule` force `disabled`
|
||||
for a fork target, and `update_trigger` preserves the fork's existing `mode`
|
||||
(`workspace_is_fork` in `windmill-trigger/src/handler.rs`; schedule `enabled` is
|
||||
naturally preserved because `EditSchedule` has no `enabled` field). The same
|
||||
handlers serve both merge paths, so the two can't diverge. The fork owner
|
||||
re-enables locally via `setmode`/`setenabled` (which carry the conflict
|
||||
warning below).
|
||||
|
||||
For a non-fork target the incoming value is applied as given — so a fork→parent
|
||||
merge of an existing trigger writes the parent's own (substituted) value (a
|
||||
no-op), and a fork-only trigger lands with the fork creator's chosen state.
|
||||
|
||||
## Conflict warning on enable
|
||||
|
||||
@@ -161,22 +187,23 @@ but `tally_deployed_object_changes` still records mutations against them; the
|
||||
deploy will fail at the workspace-collision check if the user tries to
|
||||
deploy a non-workspaced row to a fork.
|
||||
|
||||
Operational state (`mode` for triggers, `enabled` for schedules) is handled
|
||||
asymmetrically between update and create:
|
||||
Operational state (`mode` for triggers, `enabled` for schedules) follows the
|
||||
same "owned by the parent" rule as the git round-trip (see *Operational state
|
||||
is owned by the parent* above) — the two paths share the backend `create`/
|
||||
`update` handlers, so they can't diverge.
|
||||
|
||||
- **Update**: the merge deploy strips `mode`/`enabled` so the target's existing
|
||||
state is preserved. Triggers rely on the backend's `is_mode_unspecified()`
|
||||
safeguard in `update_trigger`; schedules rely on `EditSchedule` lacking the
|
||||
`enabled` field. Both deploy paths perform the strip
|
||||
(`stripOperationalState` in `utils_deployable.ts`, `preparePayload` in
|
||||
`merge.ts`). This matches the YAML round-trip's `fork_trigger_ignore_keys`
|
||||
/ `fork_schedule_ignore_keys`.
|
||||
- **Create**: the source's `mode`/`enabled` is passed through. There's no
|
||||
target row to preserve, so a fork-only trigger or schedule lands with the
|
||||
state the fork creator chose. When the source omits the flag entirely (e.g.
|
||||
legacy clients), the backend defaults to `enabled` for both kinds —
|
||||
`BaseTriggerData::mode()` returns `Enabled` and the schedule insert defaults
|
||||
to `true` to match.
|
||||
- **Update**: the merge deploy strips `mode`/`enabled`
|
||||
(`stripOperationalStateOnUpdate` in the shared `windmill-utils-internal`
|
||||
package, `cli/windmill-utils-internal/src/deploy.ts`), so the target's
|
||||
existing value is preserved — equivalent to substituting the target's value.
|
||||
For a fork target the backend preserves it regardless (`workspace_is_fork`);
|
||||
for a parent target the `is_mode_unspecified()` safeguard does. Schedules also
|
||||
rely on `EditSchedule` lacking the `enabled` field.
|
||||
- **Create**: the source's `mode`/`enabled` is passed through. Into a **parent**
|
||||
there's no row to preserve, so a fork-only trigger/schedule lands with the
|
||||
state the fork creator chose (omitting the flag defaults to `enabled`:
|
||||
`BaseTriggerData::mode()` → `Enabled`, schedule insert → `true`). Into a
|
||||
**fork** the backend forces `disabled` — a fork write never enables anything.
|
||||
|
||||
## Future work — runtime listener suffix
|
||||
|
||||
|
||||
Reference in New Issue
Block a user