forked from 1b-agent/opencadstudio-upstream
fix(python): validate nested input (vertices, edges) instead of defaulting silently
The generated dict-to-struct conversions for nested records defaulted every
absent field and ignored unknown keys, so an LwPolyline built from
{'x': .., 'y': ..} vertices became zero-coordinate vertices. They now refuse an
unknown key (naming the keys the record takes) and a manifest list,
required_struct_fields, enforces the fields that define a nested record's
geometry (vertex locations, hatch edge ends and centres, section symbol points).
Regression test over real IPC; every existing lifecycle audit still passes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
360dd853ed
commit
81251d9cf3
4 files changed
+150
-2
No files matched your search
@@ -284,3 +284,16 @@ the joined line without its parts, the two pieces after a break, the stretched a
|
||||
`create_entity("Hatch")`: its command needs an interactive boundary pick that no scripted step can supply. Not wrapped: PEDIT,
|
||||
path and 3-D arrays, and the remaining commands, which stay reachable through `doc.command` and `start_command`.
|
||||
|
||||
### 2026-09-21: nested input is validated (vertex validation gap)
|
||||
|
||||
Found while writing the `explode` audit: an `LwPolyline` created from vertices that used the wrong key (`{'x': .., 'y': ..}` instead of
|
||||
`{'location': ..}`) was accepted and became zero-coordinate vertices. The generated dict-to-struct conversions for nested records
|
||||
defaulted every absent field and ignored unknown keys, so any typo inside a list of vertices, edges or faces vanished silently.
|
||||
The generator now (1) refuses an unknown key in every nested record, naming the keys it takes, and (2) enforces a manifest list,
|
||||
`required_struct_fields`, of the fields that define a nested record's geometry: `LwVertex.location`, `Vertex2D.location`,
|
||||
`Vertex3D.location`, `Vertex3DPolyline.position`, `PolygonMeshVertex.location`, `PolyfaceVertex.location`, `MLineVertex.position`,
|
||||
`LineEdge.start/end`, `CircularArcEdge.center/radius`, `EllipticArcEdge.center/major_axis_endpoint`, `PolylineEdge.vertices` and
|
||||
`SectionSymbolPoint.point`. A `None` counts as missing. Everything else in a nested record still defaults when absent. Evidence:
|
||||
`audit_python_nested_input_is_validated_over_real_ipc` (eight refusals with the right messages for LwPolyline, Polyline2D and
|
||||
PolygonMesh; nothing invalid reaches the drawing; the valid forms still create), and every existing per-kind lifecycle audit still passes.
|
||||
|
||||
@@ -40,6 +40,11 @@ pub struct Manifest {
|
||||
pub base_fields: Vec<String>,
|
||||
#[serde(default)]
|
||||
pub overrides: std::collections::BTreeMap<String, EntityOverride>,
|
||||
/// Fields of nested structs (a polyline vertex, a hatch edge) that must be
|
||||
/// present in a scripted dict. Everything else in a nested struct still
|
||||
/// defaults when absent, but an unknown key is always refused.
|
||||
#[serde(default)]
|
||||
pub required_struct_fields: std::collections::BTreeMap<String, Vec<String>>,
|
||||
}
|
||||
|
||||
fn default_base_fields() -> Vec<String> {
|
||||
@@ -668,12 +673,24 @@ fn default_{snake}() -> {path} {{
|
||||
)
|
||||
}
|
||||
|
||||
fn gen_struct(name: &str, info: &TypeInfo, registry: &TypeRegistry) -> String {
|
||||
fn gen_struct(name: &str, info: &TypeInfo, registry: &TypeRegistry, required: &[String]) -> String {
|
||||
let path = type_path(name);
|
||||
let snake = snake_case(name);
|
||||
let mut to_dict_body = String::new();
|
||||
let mut from_dict_fields = String::new();
|
||||
let mut default_fields = String::new();
|
||||
let mut allowed: Vec<String> = Vec::new();
|
||||
for f in &info.fields {
|
||||
if f.type_id.as_str() != "EntityCommon" {
|
||||
allowed.push(format!("\"{}\"", f.name));
|
||||
}
|
||||
}
|
||||
let allowed_list = allowed.join(", ");
|
||||
let required_list = required
|
||||
.iter()
|
||||
.map(|field| format!("\"{field}\""))
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
for f in &info.fields {
|
||||
if f.type_id.as_str() == "EntityCommon" {
|
||||
// A sub-record's own common data (layer, XDATA, handles) is not part
|
||||
@@ -717,6 +734,19 @@ fn gen_struct(name: &str, info: &TypeInfo, registry: &TypeRegistry) -> String {
|
||||
|
||||
fn dict_to_{snake}(value: PyObjectRef, vm: &VirtualMachine) -> PyResult<{path}> {{
|
||||
let dict = value.try_into_value::<rustpython_vm::builtins::PyDictRef>(vm)?;
|
||||
let allowed: &[&str] = &[{allowed_list}];
|
||||
for key in dict.keys_vec() {{
|
||||
let key = key.try_into_value::<String>(vm)?;
|
||||
if !allowed.contains(&key.as_str()) {{
|
||||
return Err(vm.new_value_error(format!("{name} has no property {{key:?}}; it takes: {{}}", allowed.join(", "))));
|
||||
}}
|
||||
}}
|
||||
let required_fields: &[&str] = &[{required_list}];
|
||||
for required in required_fields.iter().copied() {{
|
||||
if dict.get_item_opt(required, vm)?.map_or(true, |v| vm.is_none(&v)) {{
|
||||
return Err(vm.new_value_error(format!("{name} needs {{required}}")));
|
||||
}}
|
||||
}}
|
||||
Ok({path} {{
|
||||
{from_dict_fields} }})
|
||||
}}
|
||||
@@ -1050,7 +1080,16 @@ pub fn generate_entity_crud(manifest: &Manifest, registry: &TypeRegistry) -> Str
|
||||
Classify::UnitEnum => out.push_str(&gen_unit_enum(&name, info)),
|
||||
Classify::TaggedEnum => out.push_str(&gen_tagged_enum(&name, info, registry)),
|
||||
Classify::BitFlags => out.push_str(&gen_bitflags(&name, info)),
|
||||
Classify::Struct => out.push_str(&gen_struct(&name, info, registry)),
|
||||
Classify::Struct => {
|
||||
let required = manifest.required_struct_fields.get(&name).cloned().unwrap_or_default();
|
||||
for field in &required {
|
||||
assert!(
|
||||
info.fields.iter().any(|f| &f.name == field),
|
||||
"entity_manifest.json: required_struct_fields: {name} has no field {field}"
|
||||
);
|
||||
}
|
||||
out.push_str(&gen_struct(&name, info, registry, &required))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -973,5 +973,46 @@
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"required_struct_fields": {
|
||||
"LwVertex": [
|
||||
"location"
|
||||
],
|
||||
"Vertex2D": [
|
||||
"location"
|
||||
],
|
||||
"Vertex3D": [
|
||||
"location"
|
||||
],
|
||||
"Vertex3DPolyline": [
|
||||
"position"
|
||||
],
|
||||
"PolygonMeshVertex": [
|
||||
"location"
|
||||
],
|
||||
"PolyfaceVertex": [
|
||||
"location"
|
||||
],
|
||||
"MLineVertex": [
|
||||
"position"
|
||||
],
|
||||
"LineEdge": [
|
||||
"start",
|
||||
"end"
|
||||
],
|
||||
"CircularArcEdge": [
|
||||
"center",
|
||||
"radius"
|
||||
],
|
||||
"EllipticArcEdge": [
|
||||
"center",
|
||||
"major_axis_endpoint"
|
||||
],
|
||||
"PolylineEdge": [
|
||||
"vertices"
|
||||
],
|
||||
"SectionSymbolPoint": [
|
||||
"point"
|
||||
]
|
||||
}
|
||||
}
|
||||
@@ -8159,6 +8159,61 @@ step('undo_move', lambda: M.move([u], (0, 0, 0), (7, 0, 0)))
|
||||
assert!(!moved(&app.tabs[0].scene.document, 1007.0));
|
||||
}
|
||||
|
||||
/// A scripted dict is checked all the way down: a nested record (a polyline
|
||||
/// vertex, a mesh vertex) may not carry an unknown key or omit the field that
|
||||
/// defines its geometry, so a typo never turns into a silent zero coordinate.
|
||||
#[test]
|
||||
fn audit_python_nested_input_is_validated_over_real_ipc() {
|
||||
let Some(plugin_path) = std::env::var_os("OCS_TEST_PYTHON_PLUGIN") else {
|
||||
return;
|
||||
};
|
||||
let mut app = OpenCADStudio::new_for_test();
|
||||
app.tabs[0].is_start = false;
|
||||
let mut host = HostSession::new(&mut app, 0);
|
||||
let process = ocs_plugin_api::process::PluginProcess::spawn(
|
||||
std::path::Path::new(&plugin_path), &mut host, crate::plugin::v4_support::notification_handler(),
|
||||
).unwrap();
|
||||
let script = std::env::temp_dir().join(format!("ocs_nested_{}.py", std::process::id()));
|
||||
std::fs::write(&script, concat!(
|
||||
"def P(x, y, z=0.0): return {'x': x, 'y': y, 'z': z}\n",
|
||||
"doc = ocs.active_document\nL = doc.layers\n",
|
||||
"refused, problems = [], []\n",
|
||||
"def check(tag, fn, expect):\n",
|
||||
" try:\n fn()\n except (RuntimeError, TypeError, ValueError) as error:\n",
|
||||
" if expect in str(error):\n refused.append(tag)\n",
|
||||
" else:\n problems.append('WRONGMESSAGE_' + tag)\n",
|
||||
" else:\n problems.append('ACCEPTED_' + tag)\n",
|
||||
"def lw(vertices): return lambda: doc.create_entity('LwPolyline', vertices=vertices)\n",
|
||||
"ok = {'location': P(0, 0)}\n",
|
||||
"check('lw_xy_keys', lw([ok, {'x': 5, 'y': 5}]), 'no property')\n",
|
||||
"check('lw_empty_vertex', lw([ok, {}]), 'needs location')\n",
|
||||
"check('lw_null_location', lw([ok, {'location': None}]), 'needs location')\n",
|
||||
"check('lw_typo', lw([ok, {'location': P(1, 1), 'bulgee': 0.5}]), 'no property')\n",
|
||||
"check('lw_message_lists_keys', lw([ok, {'lokation': P(1, 1)}]), 'location')\n",
|
||||
"check('poly2d_missing_location', lambda: doc.create_entity('Polyline2D', vertices=[{'flags': 0}, {'location': P(1, 1)}]), 'needs location')\n",
|
||||
"check('poly2d_typo', lambda: doc.create_entity('Polyline2D', vertices=[{'location': P(0, 0)}, {'loc': P(1, 1)}]), 'no property')\n",
|
||||
"check('mesh_missing_location', lambda: doc.create_entity('PolygonMesh', vertices=[{}]), 'needs location')\n",
|
||||
"good = doc.create_entity('LwPolyline', vertices=[ok, {'location': P(10, 0), 'bulge': 0.5}, {'location': P(10, 10)}])\n",
|
||||
"good2 = doc.create_entity('Polyline2D', vertices=[{'location': P(0, 0)}, {'location': P(5, 5)}])\n",
|
||||
"L.create('REPORT ' + str(len(refused)) + ' ~ ' + ' '.join(problems))\n",
|
||||
)).unwrap();
|
||||
assert!(process.dispatch(&mut host, &format!("PY_RUN {}", script.display()), &mut |_| {}).unwrap());
|
||||
let _ = std::fs::remove_file(&script);
|
||||
let name = host.document().layers.iter().map(|l| l.name.clone()).filter(|n| n.starts_with("REPORT")).last().expect("script report layer");
|
||||
let (refused, problems) = name["REPORT".len()..].split_once('~').unwrap();
|
||||
assert_eq!(problems.trim(), "", "problems: {problems}");
|
||||
assert_eq!(refused.trim(), "8");
|
||||
// Nothing invalid reached the drawing; the two valid polylines did.
|
||||
let document = host.document();
|
||||
let lw: Vec<_> = document.entities().filter_map(|e| match e { EntityType::LwPolyline(p) => Some(p.clone()), _ => None }).collect();
|
||||
assert_eq!(lw.len(), 1, "only the valid LwPolyline was created");
|
||||
assert_eq!(lw[0].vertices.len(), 3);
|
||||
assert_eq!(lw[0].vertices[1].bulge, 0.5);
|
||||
assert_eq!(document.entities().filter(|e| matches!(e, EntityType::Polyline2D(_))).count(), 1);
|
||||
assert!(!document.entities().any(|e| matches!(e, EntityType::PolygonMesh(_))));
|
||||
drop(process);
|
||||
}
|
||||
|
||||
/// Exploration harness (not a gate): prints how the real commands answer
|
||||
/// each step so the wrappers are written against observed prompts.
|
||||
#[test]
|
||||
|
||||
Reference in new issue
Block a user