Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
114 changes: 107 additions & 7 deletions crates/rustmotion-cli/src/commands/validate.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -30,18 +30,21 @@ fn announced_duration(scenario: &ResolvedScenario) -> f64 {
/// Why `--fix` must not write over this input.
///
/// `--fix` serialises `LoadedScenario::raw`, which is the document *after*
/// variable substitution and `include` resolution — not the document on disk. For
/// a plain JSON scenario the two coincide and writing back is faithful. For
/// anything templated they do not, and the write silently replaces the source
/// with its own expansion: the `config` block and every `$var` disappear, includes
/// get inlined into the parent, and an HTML input is replaced by JSON outright.
/// variable substitution, `for-each`/`use` expansion, and `include` resolution
/// — not the document on disk. For a plain JSON scenario the two coincide and
/// writing back is faithful. For anything templated they do not, and the write
/// silently replaces the source with its own expansion: the `config` block and
/// every `$var` disappear, includes get inlined into the parent, `for-each`/
/// `use` get inlined into their repeated/instantiated output, and an HTML
/// input is replaced by JSON outright.
///
/// One rule covers all three: only write back a source `--fix` can reproduce.
/// One rule covers all four: only write back a source `--fix` can reproduce.
#[derive(Debug, PartialEq, Eq)]
enum FixRefusal {
HtmlSource,
Templated,
UsesInclude,
UsesTemplateDirectives,
}

impl FixRefusal {
Expand All@@ -63,6 +66,13 @@ impl FixRefusal {
resolved tree — inlining the included files into the parent and patching by a \
path that no longer means the same node. Fix the included file directly."
),
Self::UsesTemplateDirectives => format!(
"--fix cannot rewrite {p}: it uses `for-each`/`use` (or declares `components`), \
and the fixer would write back the expanded tree — inlining every repeated \
instance and patching by a path that no longer means the same source node, \
exactly like `include`. Fix the `components` definition or the `for-each` \
template directly."
),
}
}
}
Expand All@@ -85,6 +95,16 @@ fn refuse_fix(input: &Path, raw_source: &str) -> Option<FixRefusal> {
if raw_source.contains("\"include\"") {
return Some(FixRefusal::UsesInclude);
}
// Same conservative, raw-substring detection as `UsesInclude` above (not
// a full walk of the tree): `components`/`for-each`/`use` can appear at
// any depth, and `--fix` must refuse before it ever gets far enough to
// find out whether they're actually reachable.
if source.get("components").is_some()
|| raw_source.contains("\"for-each\"")
|| raw_source.contains("\"use\"")
{
return Some(FixRefusal::UsesTemplateDirectives);
}
None
}

Expand DownExpand Up@@ -583,7 +603,7 @@ mod tests {
}

/// `--fix` writes back the *resolved* tree. Anything the resolution erased is
/// erased on disk too, so these three inputs must be refused rather than
/// erased on disk too, so these inputs must be refused rather than
/// silently rewritten.
mod fix_refusals {
use super::super::{refuse_fix, FixRefusal};
Expand DownExpand Up@@ -637,13 +657,40 @@ mod tests {
);
}

#[test]
fn a_scenario_using_for_each_is_refused() {
// No `$` anywhere in this fixture on purpose — proves the
// detection is driven by the `for-each` marker itself, not by
// piggybacking on the pre-existing `$`-content check.
let with_for_each = r##"{"video":{"width":320,"height":240,"fps":30},
"scenes":[{"duration":1.0,"children":[
{"for-each":[1,2],"template":{"type":"text","content":"static"}}
]}]}"##;
assert_eq!(
refuse_fix(Path::new("s.json"), with_for_each),
Some(FixRefusal::UsesTemplateDirectives)
);
}

#[test]
fn a_scenario_declaring_components_is_refused_even_with_no_use_site_yet() {
let with_components = r##"{"video":{"width":320,"height":240,"fps":30},
"components":{"card":{"params":{},"template":{"type":"text","content":"hi"}}},
"scenes":[{"duration":1.0,"children":[]}]}"##;
assert_eq!(
refuse_fix(Path::new("s.json"), with_components),
Some(FixRefusal::UsesTemplateDirectives)
);
}

#[test]
fn every_refusal_names_the_file_and_says_what_to_do_instead() {
let p = Path::new("scenes/hero.json");
for r in [
FixRefusal::HtmlSource,
FixRefusal::Templated,
FixRefusal::UsesInclude,
FixRefusal::UsesTemplateDirectives,
] {
let msg = r.explain(p);
assert!(msg.contains("scenes/hero.json"), "{msg}");
Expand DownExpand Up@@ -788,5 +835,58 @@ mod tests {
);
assert_eq!(part_after, part, "included file must be untouched too");
}

/// Same failure mode as `include`, for the sibling mechanism: `for-each`
/// expanding to more than one node shifts every later `children[N]`
/// index, so a path-based `--fix` patch would land on the wrong
/// (or a nonexistent) sibling if it were allowed to write back the
/// expanded tree. It must be refused outright instead.
#[test]
fn cmd_validate_fix_refuses_to_overwrite_a_scenario_using_for_each_and_leaves_the_file_untouched(
) {
let path = std::env::temp_dir().join(format!(
"rm_validate_fix_for_each_{}.json",
std::process::id()
));
let original = r##"{
"video": { "width": 1920, "height": 1080 },
"scenes": [{
"duration": 1.0,
"children": [{
"for-each": [
{ "label": "short" },
{ "label": "this string is too long to fit in its card" }
],
"template": {
"type": "card",
"x": 100, "y": 100,
"style": { "width": "200px", "height": "200px", "background": "#222244" },
"children": [{
"type": "text",
"content": "$label",
"style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" }
}]
}
}]
}]
}"##;
std::fs::write(&path, original).expect("write fixture");

let result = cmd_validate(&path, None, /*fix=*/ true, false, false, false, None);

let after = std::fs::read_to_string(&path).expect("read back fixture");
std::fs::remove_file(&path).ok();

assert!(
result.is_err(),
"--fix on a for-each-using scenario with a real violation must be refused"
);
assert_eq!(
after, original,
"the file must be byte-identical after a refused --fix — the two `for-each` \
iterations expand into two card siblings, so a path-based patch would not even \
land on the right one"
);
}
}
}
87 changes: 87 additions & 0 deletions crates/rustmotion-cli/src/commands/validation.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,6 +12,7 @@

use rustmotion::engine;
use rustmotion::error::{Result, RustmotionError};
use rustmotion::expand;
use rustmotion::include::{self, IncludeSource};
use rustmotion::schema::{ResolvedScenario, Scenario};
use rustmotion::variables;
Expand DownExpand Up@@ -170,6 +171,14 @@ pub fn load_with_vars(
.map(|p| p.display().to_string())
.unwrap_or_else(|| "<inline>".to_string());
variables::apply_variables(&mut json_value, overrides, &label)?;
// Expand `for-each`/`use` (and consume `components`) *before* `raw` is
// captured below, so `LoadedScenario::raw` — what geometry checks walk
// and what `--fix` would serialise — is already the expanded tree. This
// is the same reason `include::resolve_includes` runs before this
// function returns: a validator that reasons about the pre-expansion
// document would be validating something other than what actually
// renders.
expand::expand_directives(&mut json_value, &label)?;

let scenario: Scenario = serde_json::from_value(json_value.clone())?;
let resolved = include::resolve_includes(scenario, &include_source)?;
Expand DownExpand Up@@ -512,6 +521,84 @@ pub fn warn_strict_attrs_is_now_default() {
);
}

/// Proves `validate` reasons about the *expanded* tree, not the
/// pre-expansion `for-each`/`use` directives — the same requirement the
/// workstream brief states for `include`-produced scenes ("le validateur
/// doit voir l'arbre expansé"). If `load_with_vars` only expanded directives
/// for rendering but validated the raw, unexpanded document, a geometry
/// violation baked into one of several `for-each`-generated items would be
/// invisible: the un-expanded document has no `text`/`card` components at
/// all at that position, only a directive object geometry checks don't know
/// how to measure.
#[cfg(test)]
mod expanded_tree_is_what_gets_validated {
use super::*;

#[test]
fn a_geometry_violation_inside_a_for_each_generated_item_is_detected() {
// Two iterations: the first is short and fits, the second is a
// narrow-card/nowrap-text combination guaranteed to overflow — the
// same violation shape `NARROW_CARD_JSON` uses elsewhere in this
// crate's tests.
let json = serde_json::json!({
"video": { "width": 1920, "height": 1080 },
"scenes": [{
"duration": 1.0,
"children": [{
"for-each": [
{ "label": "ok" },
{ "label": "this string is far too long to fit in this narrow card" }
],
"template": {
"type": "card",
"x": 100, "y": 100,
"style": { "width": "200px", "height": "200px", "background": "#222244" },
"children": [{
"type": "text",
"content": "$label",
"style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" }
}]
}
}]
}]
})
.to_string();

let loaded = load(ValidationSource::Inline(&json)).expect("scenario loads");

// The raw tree `--fix` would act on must already be expanded: no
// `for-each` directive marker survives, and there are 2 concrete
// children where the source only wrote 1 directive.
let raw_children = loaded.raw["scenes"][0]["children"].as_array().unwrap();
assert_eq!(
raw_children.len(),
2,
"loaded.raw must hold the 2 expanded cards, not the 1 for-each directive"
);
assert!(
raw_children.iter().all(|c| c.get("for-each").is_none()),
"no for-each directive marker must survive into loaded.raw: {raw_children:?}"
);

let report = run_checks(&loaded, false);
assert_eq!(
report.geom_violations.len(),
1,
"exactly one of the two for-each-generated cards overflows; a validator that only \
saw the pre-expansion directive could not have found this at all: {:?}",
report.geom_violations
);
// The violation's path must point at the *second* expanded card
// (children[1]), proving the geometry walker is indexing into the
// expanded array, not some placeholder.
assert!(
report.geom_violations[0].path.contains("children[1]"),
"expected the violation to be attributed to the second expanded card: {}",
report.geom_violations[0].path
);
}
}

#[cfg(test)]
mod check_crf_tests {
use super::check_crf;
Expand Down
55 changes: 55 additions & 0 deletions crates/rustmotion-core/src/error.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -148,6 +148,61 @@ pub enum RustmotionError {
#[error("Cannot interpolate non-string variable '${name}' into string in '{path}'")]
VariableInterpolationTypeError { name: String, path: String },

// --- Templates: `components` / `use` / `for-each` (see rustmotion_core::expand) ---
#[error("'components' at '{path}' must be an object mapping names to definitions")]
ComponentsBlockNotObject { path: String },

#[error("Component definition '{name}' at '{path}' is invalid: {reason}")]
ComponentDefinitionInvalid {
name: String,
path: String,
reason: String,
},

#[error("'use' directive at '{path}' is invalid: {reason}")]
UseDirectiveInvalid { path: String, reason: String },

#[error(
"Unknown component '{name}' referenced via 'use' at '{path}' — no such name in this \
file's 'components' block"
)]
UnknownComponent { name: String, path: String },

#[error(
"Missing required parameter '{param}' for component '{component}' at '{path}' — it has \
no default and was not supplied via 'props'"
)]
ComponentParamMissing {
component: String,
param: String,
path: String,
},

#[error(
"Unknown parameter '{param}' passed to component '{component}' at '{path}' — not \
declared in its 'params'"
)]
UnknownComponentParam {
component: String,
param: String,
path: String,
},

#[error("Component instantiation cycle at '{path}': {chain}")]
ComponentCycle { chain: String, path: String },

#[error("'for-each' directive at '{path}' is invalid: {reason}")]
ForEachDirectiveInvalid { path: String, reason: String },

#[error("'for-each' at '{path}' must resolve to an array; found {found}")]
ForEachNotArray { path: String, found: String },

#[error(
"Template/component expansion depth limit ({limit}) exceeded at '{path}' — likely a \
runaway nested 'use'/'for-each' template"
)]
ExpansionDepthExceeded { limit: u32, path: String },

// --- Encoding ---
#[error("No frames to render (total duration is 0)")]
NoFrames,
Expand Down
Loading
Loading