Skip to content

Validate explicit targets in layer row commands (part of #785). #1255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bflatastic merged 1 commit into from
Oct 12, 2026
Merged

Validate explicit targets in layer row commands (part of #785). #1255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions crates/engine/src/cmd/layer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,15 +189,16 @@ pub fn specs() -> Vec<CommandSpec> {

/// The rows a panel command acts on: `ids` or `id`, else the highlighted rows, else the current
/// layer (capped at [`MAX_ROWS`]).
pub(crate) fn rows_param(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
let mut ids = match (ids_param(p, "ids"), id_param(p, "id")) {
(Some(ids), _) => ids,
(None, Some(id)) => vec![id],
(None, None) => {
let st = s.doc()?;
let rows = st.highlighted_rows();
if rows.is_empty() { st.current_layer().into_iter().collect() } else { rows }
}
pub(crate) fn rows_param(s: &Session, p: &Value, command: &str) -> Result<Vec<NodeId>> {
// Presence is explicit intent: validate every target before fallback, truncation or edits.
let mut ids = if p.get("ids").is_some() {
checked_ids_param(s, p, "ids", command)?
} else if let Some(id) = p.get("id") {
vec![checked_id(s, id, command)?]
} else {
let st = s.doc()?;
let rows = st.highlighted_rows();
if rows.is_empty() { st.current_layer().into_iter().collect() } else { rows }
};
ids.truncate(MAX_ROWS);
Ok(ids)
Expand Down Expand Up @@ -457,7 +458,7 @@ fn new_sublayer(s: &mut Session, p: &Value) -> Result<Value> {
}

fn delete_rows(s: &mut Session, p: &Value) -> Result<Value> {
let ids = rows_param(s, p)?;
let ids = rows_param(s, p, "layer.delete")?;
let st = s.doc()?;
let ids = row_roots(&st.doc, &ids);
if ids.is_empty() {
Expand All @@ -484,7 +485,7 @@ fn delete_rows(s: &mut Session, p: &Value) -> Result<Value> {
}

fn duplicate_rows(s: &mut Session, p: &Value) -> Result<Value> {
let ids = rows_param(s, p)?;
let ids = rows_param(s, p, "layer.duplicate")?;
let ids = row_roots(&s.doc()?.doc, &ids);
if ids.is_empty() {
return Err(bad("layer.duplicate", "no such rows"));
Expand Down Expand Up @@ -551,7 +552,7 @@ fn highlight(s: &mut Session, p: &Value) -> Result<Value> {
fn set_props(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "layer.setProps";
check_options(C, p)?;
let ids = rows_param(s, p)?;
let ids = rows_param(s, p, C)?;
if ids.is_empty() {
return Err(bad(C, "no rows"));
}
Expand All @@ -567,12 +568,11 @@ fn set_props(s: &mut Session, p: &Value) -> Result<Value> {

fn collect(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "layer.collectInNew";
let ids = match ids_param(p, "ids") {
Some(ids) => ids,
None => {
let rows = s.doc()?.highlighted_rows();
if rows.is_empty() { super::edit::selected_roots(s)? } else { rows }
}
let ids = if p.get("ids").is_some() {
checked_ids_param(s, p, "ids", C)?
} else {
let rows = s.doc()?.highlighted_rows();
if rows.is_empty() { super::edit::selected_roots(s)? } else { rows }
};
let ids = row_roots(&s.doc()?.doc, &ids);
let Some(top) = ids.last().copied() else { return Err(bad(C, "select rows or objects to collect")) };
Expand Down
25 changes: 14 additions & 11 deletions crates/engine/src/cmd/layerpanel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ pub fn specs() -> Vec<CommandSpec> {
None,
"{ids?} hide every top-level layer but those holding the highlighted rows (else the current layer), which show. One undo step → {count}",
has_doc,
|s, p| others(s, p, Flag::Visible, false)
|s, p| others(s, p, Flag::Visible, false, "layer.hideOthers")
),
cmd!("layer.showAll", "Show All Layers", ["Window", "Layers"], None, "{} show every layer and sublayer → {count}", has_doc, |s, _| all(
s,
Expand All @@ -98,7 +98,7 @@ pub fn specs() -> Vec<CommandSpec> {
None,
"{ids?} turn Preview off (outline view) for every top-level layer but those holding the highlighted rows (else the current layer), which preview → {count}",
has_doc,
|s, p| others(s, p, Flag::Preview, false)
|s, p| others(s, p, Flag::Preview, false, "layer.outlineOthers")
),
cmd!(
"layer.previewAll",
Expand All @@ -116,7 +116,7 @@ pub fn specs() -> Vec<CommandSpec> {
None,
"{ids?} lock every top-level layer but those holding the highlighted rows (else the current layer), which unlock → {count}",
has_doc,
|s, p| others(s, p, Flag::Locked, true)
|s, p| others(s, p, Flag::Locked, true, "layer.lockOthers")
),
cmd!(
"layer.unlockAll",
Expand Down Expand Up @@ -196,16 +196,16 @@ fn set_flag(s: &mut Session, changes: Vec<(NodeId, bool)>, flag: Flag, label: &s
}

/// The top-level layers holding the rows a command acts on.
fn kept_layers(s: &Session, p: &Value) -> Result<HashSet<NodeId>> {
let ids = rows_param(s, p)?;
fn kept_layers(s: &Session, p: &Value, command: &str) -> Result<HashSet<NodeId>> {
let ids = rows_param(s, p, command)?;
let d = &s.doc()?.doc;
Ok(ids.iter().filter_map(|id| d.layer_of(*id)).collect())
}

/// Every top-level layer but the kept ones set to `v`, and the kept ones to the opposite: Alt-click
/// on a hidden layer's eye shows just that layer (#1158).
fn others(s: &mut Session, p: &Value, flag: Flag, v: bool) -> Result<Value> {
let keep = kept_layers(s, p)?;
fn others(s: &mut Session, p: &Value, flag: Flag, v: bool, command: &str) -> Result<Value> {
let keep = kept_layers(s, p, command)?;
let changes = s.doc()?.doc.layers.iter().map(|l| (l.id, !keep.contains(&l.id) == v)).collect();
set_flag(s, changes, flag, flag.label(v))
}
Expand All @@ -221,7 +221,7 @@ fn all(s: &mut Session, flag: Flag, v: bool) -> Result<Value> {
}

fn template(s: &mut Session, p: &Value) -> Result<Value> {
let ids = rows_param(s, p)?;
let ids = rows_param(s, p, "layer.template")?;
let d = &s.doc()?.doc;
let ids: Vec<NodeId> = ids.into_iter().filter(|id| d.node(*id).is_some_and(Node::is_layer)).collect();
let Some(first) = ids.first().and_then(|id| d.node(*id)) else { return Err(bad("layer.template", "select a layer")) };
Expand Down Expand Up @@ -268,7 +268,7 @@ fn merge_into(d: &mut Document, into: NodeId, others: &[NodeId]) -> Result<()> {

fn merge(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "layer.merge";
let ids = rows_param(s, p)?;
let ids = rows_param(s, p, C)?;
let st = s.doc()?;
let d = &st.doc;
let mut seen = HashSet::new();
Expand Down Expand Up @@ -300,7 +300,10 @@ fn flatten(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "layer.flatten";
let st = s.doc()?;
let d = &st.doc;
let from = id_param(p, "id").or_else(|| st.highlighted_rows().last().copied()).or(st.current_layer());
let from = match p.get("id") {
Some(id) => Some(checked_id(s, id, C)?),
None => st.highlighted_rows().last().copied().or(st.current_layer()),
};
let into = from.and_then(|id| d.layer_of(id)).ok_or_else(|| bad(C, "no layer"))?;
if !d.node(into).is_some_and(|n| n.visible) {
return Err(EngineError::Other("the layer to flatten into is hidden".into()));
Expand Down Expand Up @@ -389,7 +392,7 @@ fn release(s: &mut Session, p: &Value) -> Result<Value> {

fn reverse(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "layer.reverse";
let ids = rows_param(s, p)?;
let ids = rows_param(s, p, C)?;
let d = &s.doc()?.doc;
let ids = row_roots(d, &ids);
// Rows grouped by the layer or group holding them (None: top-level layers).
Expand Down
157 changes: 157 additions & 0 deletions crates/engine/src/tests_layers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,3 +614,160 @@ fn the_clipping_mask_button_acts_on_the_highlighted_group() {
assert_eq!(run(&mut s, "layer.clippingMask.toggle", json!({}))["clip"], true);
assert!(node(&s, g).clips());
}

/// #785: explicit malformed layer targets must never become the current-layer default.
#[test]
fn layer_delete_rejects_invalid_explicit_targets() {
let mut unexpected = vec![];
for params in [json!({"id": "1"}), json!({"ids": [1, "2"]}), json!({"id": "$1.id"})] {
let mut s = session();
run(&mut s, "layer.new", json!({}));
run(&mut s, "layer.new", json!({}));
let before = layer_target_snapshot(&s);
let result = s.execute("layer.delete", &params);
if result.is_ok() {
let names: Vec<_> = s.doc().unwrap().doc.layers.iter().map(|l| l.display_name()).collect();
unexpected.push(format!("{params}: {result:?}; remaining layers {names:?}"));
} else {
assert_eq!(layer_target_snapshot(&s), before, "{params}");
}
}
assert!(unexpected.is_empty(), "malformed requests succeeded: {}", unexpected.join("; "));
}

fn layer_target_snapshot(s: &Session) -> Value {
let st = s.doc().unwrap();
let history =
|entries: &[HistoryEntry]| entries.iter().map(|e| json!({"label": e.label, "doc": e.doc, "selection": e.selection})).collect::<Vec<_>>();
json!({"doc": st.doc, "selection": st.selection, "revision": st.revision, "activeLayer": st.active_layer,
"rows": st.layer_rows, "undo": history(&st.history.undo), "redo": history(&st.history.redo)})
}

fn layer_target_fixture() -> (Session, Vec<NodeId>) {
let mut s = session();
let first = s.doc().unwrap().doc.layers[0].id;
rect(&mut s, 0.0);
let second = id(&run(&mut s, "layer.new", json!({})));
rect(&mut s, 40.0);
let third = id(&run(&mut s, "layer.new", json!({})));
rect(&mut s, 80.0);
run(&mut s, "layer.highlight", json!({"ids": [first.0, second.0]}));
(s, vec![first, second, third])
}

fn malformed_layer_targets(first: NodeId, second: NodeId) -> Vec<Value> {
let mut params = vec![];
for bad in [json!("1"), json!("$1.id"), json!(1.0), json!(1.5), json!(-1), Value::Null, json!(true), json!({}), json!([])] {
params.push(json!({"id": bad}));
params.push(json!({"ids": [first.0, bad]}));
}
for bad in [Value::Null, json!("1"), json!(1), json!(false), json!({})] {
params.push(json!({"ids": bad, "id": first.0}));
}
for missing in [0, u64::MAX] {
params.push(json!({"id": missing}));
params.push(json!({"ids": [first.0, second.0, missing]}));
}
params
}

fn assert_layer_target_error(s: &mut Session, command: &str, params: Value) {
let before = layer_target_snapshot(s);
let error = s.execute(command, &params).unwrap_err();
assert!(error.to_string().contains(command), "{command} {params}: {error}");
assert_eq!(layer_target_snapshot(s), before, "{command} {params}: rejected targets must leave all state unchanged");
}

/// #785: shared row commands reject malformed or unknown targets before any partial edits.
#[test]
fn layer_target_validation_keeps_document_selection_revision_and_history_unchanged() {
let (_, rows) = layer_target_fixture();
for command in [
"layer.delete",
"layer.duplicate",
"layer.setProps",
"layer.template",
"layer.merge",
"layer.reverse",
"layer.hideOthers",
"layer.outlineOthers",
"layer.lockOthers",
] {
for mut params in malformed_layer_targets(rows[0], rows[1]) {
let (mut s, _) = layer_target_fixture();
if command == "layer.setProps" {
params["name"] = json!("Changed");
}
assert_layer_target_error(&mut s, command, params);
}
}
}

/// These commands have their own selectors, not the shared row-default parser.
#[test]
fn layer_collect_and_flatten_validate_their_own_explicit_targets() {
let (_, rows) = layer_target_fixture();
for params in malformed_layer_targets(rows[0], rows[1]) {
let command = if params.get("ids").is_some() { "layer.collectInNew" } else { "layer.flatten" };
let (mut s, _) = layer_target_fixture();
assert_layer_target_error(&mut s, command, params);
}
}

/// Truncation limits work, not validation: a malformed or unknown tail cannot be ignored.
#[test]
fn layer_target_arrays_are_fully_validated_before_the_row_limit() {
// The existing panel row limit is 100,000; exercise the public command beyond it.
const ROW_LIMIT: usize = 100_000;
for tail in [json!("$1.id"), json!(u64::MAX)] {
let (mut s, rows) = layer_target_fixture();
let mut values = vec![json!(rows[0].0); ROW_LIMIT + 1];
*values.last_mut().unwrap() = tail;
assert_layer_target_error(&mut s, "layer.delete", json!({"ids": values}));
}
let (mut s, rows) = layer_target_fixture();
let mut values = vec![json!(rows[0].0); ROW_LIMIT + 1];
*values.last_mut().unwrap() = json!(rows[1].0);
run(&mut s, "layer.setProps", json!({"ids": values, "name": "Within limit"}));
assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Within limit"));
assert_ne!(node(&s, rows[1]).name.as_deref(), Some("Within limit"), "the valid tail remains beyond the work cap");
}

#[test]
fn layer_target_defaults_and_valid_explicit_precedence_are_preserved() {
let (mut s, rows) = layer_target_fixture();
run(&mut s, "layer.setProps", json!({"name": "Highlighted"}));
assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Highlighted"));
assert_eq!(node(&s, rows[1]).name.as_deref(), Some("Highlighted"));
run(&mut s, "layer.setProps", json!({"id": rows[2].0, "name": "Single"}));
assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Single"));
let before = undos(&s);
run(&mut s, "layer.setProps", json!({"ids": [rows[1].0, rows[0].0, rows[1].0], "id": "$1.id", "name": "Explicit"}));
assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Explicit"));
assert_eq!(node(&s, rows[1]).name.as_deref(), Some("Explicit"));
assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Single"));
assert_eq!(undos(&s), before + 1, "duplicates remain one edit");
assert_layer_target_error(&mut s, "layer.setProps", json!({"ids": [], "id": rows[2].0, "name": "Must not fall back"}));
run(&mut s, "layer.setCurrent", json!({"id": rows[2].0}));
run(&mut s, "layer.highlight", json!({"ids": []}));
run(&mut s, "layer.setProps", json!({"name": "Current"}));
assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Current"));
let before = undos(&s);
run(&mut s, "layer.delete", json!({}));
assert!(s.doc().unwrap().doc.node(rows[2]).is_none());
assert_eq!(undos(&s), before + 1);
}

#[test]
fn layer_collect_and_flatten_keep_valid_and_omitted_target_behavior() {
let (mut s, rows) = layer_target_fixture();
let r = run(&mut s, "layer.flatten", json!({"id": rows[2].0}));
assert_eq!(r["id"], json!(rows[2].0));
assert_eq!(s.doc().unwrap().doc.layers.len(), 1);
let (mut s, _) = layer_target_fixture();
let selected = s.doc().unwrap().selection.objects.clone();
s.doc_mut().unwrap().layer_rows.clear();
let r = run(&mut s, "layer.collectInNew", json!({}));
let collected = id(&r);
assert!(selected.iter().all(|id| parent(&s, *id) == Some(collected)));
}
Loading