1b-agent/OneCAD-Draft
1
0 Fork 0
forked from 1b-agent/opencadstudio-upstream
Code Pull Requests Activity

fix(doc-api): preserve XDATA and XRecord integrity

Browse Source
This commit is contained in:
Hakan Seven committed 2026-09-10 18:00:07 +03:00
1 parent acba9f87bd
commit 4de2792798
9 files changed
+367 -108
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

crates/ocs_doc_api/build.rs
+3 -2
View File
@@ -172,8 +172,9 @@ Every typed handle (`Solid`, `Line`, `Circle`, `Polyline`, `Point`, `ArcCurve`,
- **`set_layer(layer)`** — move the entity to an existing layer (one undo step;
blocked if the entity or target layer is locked).
`Entity` additionally has `view()` (id + kind + bounds) and `as_solid()` (typed
downcast when `kind == "Solid3D"`).
`Entity` additionally has `view()` (id + kind + bounds), `as_solid()` (typed
downcast when `kind == "Solid3D"`), and XDATA access through
`xdata(application_name)` / `set_xdata(application_name, record)`.
## Layer table
crates/ocs_doc_api/spec/entities.toml
+1 -16
View File
@@ -471,7 +471,7 @@ description = "Extended record (XRecord): arbitrary DXF group-code/value pairs.
name = "create_xrecord"
args = [{ name = "spec", ty = "XRecordSpec" }]
op = "CreateXRecord"
returns = "Entity"
returns = "XRecord"
[[family.method]]
name = "payload"
@@ -486,21 +486,6 @@ op = "SetXRecord"
args = [{ name = "spec", ty = "XRecordSpec" }]
returns = "()"
# Cross-cutting: every Entity handle can read/write its own XDATA.
[[family.method]]
name = "xdata"
kind = "query"
query = "GetXData"
args = [{ name = "application_name", ty = "&str" }]
returns = "Option<XDataRecord>"
[[family.method]]
name = "set_xdata"
kind = "op"
op = "SetXData"
args = [{ name = "application_name", ty = "&str" }, { name = "record", ty = "Option<XDataRecord>" }]
returns = "()"
# ============================================================================
# Cross-cutting ops (generic handles + bulk) — documented in build.rs
# ============================================================================
crates/ocs_doc_api/src/executor.rs
+9 -6
View File
@@ -411,12 +411,15 @@ fn apply_op_inner<B: DocApiBackend>(b: &mut B, op: Operation) -> ApiResult<Recei
OpOutcome::Updated(*id)
}
Operation::SetXDataMany(records) => {
// Pre-validate every entity id before any mutation.
for (id, app, _) in records {
require_exists(b, *id, name)?;
if app.is_empty() {
return Err(ApiError::validation(name, "empty application_name"));
}
// A later locked entity must not leave earlier records changed.
for (i, (id, _, _)) in records.iter().enumerate() {
b.can_modify(*id).map_err(|e| match e {
ApiError::UnknownId(_) => stale_index(name, i, *id),
ApiError::Unsupported(reason) => {
ApiError::validation(name, format!("index {i}: {reason}"))
}
other => other,
})?;
}
b.push_undo(name);
for (id, application_name, record) in records {
crates/ocs_doc_api/src/facade.rs
+23 -17
View File
@@ -386,6 +386,12 @@ macro_rules! handle {
self.id
}
}
};
}
macro_rules! entity_handle {
($name:ident) => {
handle!($name);
impl $name {
pub fn bounds(&self) -> ApiResult<Aabb> {
match self.session.one_query(Query::GetBounds { id: self.id })? {
@@ -427,20 +433,20 @@ macro_rules! handle {
};
}
handle!(Entity);
handle!(Solid);
handle!(Line);
handle!(Circle);
handle!(Polyline);
handle!(Point);
handle!(ArcCurve);
handle!(Ellipse);
handle!(Spline);
handle!(Ray);
handle!(XLine);
handle!(Text);
handle!(MText);
handle!(Dimension);
entity_handle!(Entity);
entity_handle!(Solid);
entity_handle!(Line);
entity_handle!(Circle);
entity_handle!(Polyline);
entity_handle!(Point);
entity_handle!(ArcCurve);
entity_handle!(Ellipse);
entity_handle!(Spline);
entity_handle!(Ray);
entity_handle!(XLine);
entity_handle!(Text);
entity_handle!(MText);
entity_handle!(Dimension);
handle!(XRecord);
impl XRecord {
@@ -1308,8 +1314,8 @@ impl EntityCollection {
Ok(())
}
/// Create a standalone `XRECORD` object (returns an `Entity` handle).
pub fn create_xrecord(&self, spec: &XRecordSpec) -> ApiResult<Entity> {
/// Create a standalone `XRECORD` object.
pub fn create_xrecord(&self, spec: &XRecordSpec) -> ApiResult<XRecord> {
let receipt = self
.session
.apply_op(Operation::CreateXRecord(spec.clone()))?;
@@ -1317,7 +1323,7 @@ impl EntityCollection {
.outcome
.and_then(|o| o.new_id())
.ok_or_else(|| ApiError::Transport("create_xrecord returned no id".into()))?;
Ok(Entity::new(self.session.clone(), id))
Ok(XRecord::new(self.session.clone(), id))
}
/// Lookup an `XRECORD` object by id and return a typed accessor handle.
crates/ocs_doc_api/src/gen/api_reference.md
+4 -5
View File
@@ -198,14 +198,12 @@ Extended record (XRecord): arbitrary DXF group-code/value pairs. Roundtripped ve
### Constructors (`doc.entities()`)
- **create_xrecord**(spec: XRecordSpec) -> `Entity` — `CreateXRecord`
- **create_xrecord**(spec: XRecordSpec) -> `XRecord` — `CreateXRecord`
### Methods
- **payload**() -> `XRecordSpec` — query `GetXRecord`
- **set_payload**(spec: XRecordSpec) -> `()` — op `SetXRecord`
- **xdata**(application_name: &str) -> `Option<XDataRecord>` — query `GetXData`
- **set_xdata**(application_name: &str, record: Option<XDataRecord>) -> `()` — op `SetXData`
## `Layer` (acadrust `-`, collection `layers`)
@@ -234,8 +232,9 @@ Every typed handle (`Solid`, `Line`, `Circle`, `Polyline`, `Point`, `ArcCurve`,
- **`set_layer(layer)`** — move the entity to an existing layer (one undo step;
blocked if the entity or target layer is locked).
`Entity` additionally has `view()` (id + kind + bounds) and `as_solid()` (typed
downcast when `kind == "Solid3D"`).
`Entity` additionally has `view()` (id + kind + bounds), `as_solid()` (typed
downcast when `kind == "Solid3D"`), and XDATA access through
`xdata(application_name)` / `set_xdata(application_name, record)`.
## Layer table
crates/ocs_doc_api/src/gen/binding_schema.json
+1 -37
View File
@@ -1140,7 +1140,7 @@
"fixed": null,
"name": "create_xrecord",
"op": "CreateXRecord",
"returns": "Entity"
"returns": "XRecord"
}
],
"handle": "XRecord",
@@ -1171,42 +1171,6 @@
"query": null,
"read_only": false,
"returns": "()"
},
{
"args": [
{
"name": "application_name",
"ty": "&str"
}
],
"bulk": false,
"fixed": null,
"kind": "query",
"name": "xdata",
"op": null,
"query": "GetXData",
"read_only": true,
"returns": "Option<XDataRecord>"
},
{
"args": [
{
"name": "application_name",
"ty": "&str"
},
{
"name": "record",
"ty": "Option<XDataRecord>"
}
],
"bulk": false,
"fixed": null,
"kind": "op",
"name": "set_xdata",
"op": "SetXData",
"query": null,
"read_only": false,
"returns": "()"
}
],
"name": "XRecord"
crates/ocs_doc_api/src/validation.rs
+112 -5
View File
@@ -21,6 +21,88 @@ fn check(valid: bool, op: &'static str) -> ApiResult<()> {
}
}
fn check_payload(valid: bool, op: &'static str, reason: &'static str) -> ApiResult<()> {
if valid {
Ok(())
} else {
Err(ApiError::validation(op, reason))
}
}
fn valid_application_name(name: &str) -> bool {
!name.trim().is_empty() && !name.contains('\0')
}
fn valid_xdata_record(app: &str, record: Option<&XDataRecord>) -> bool {
valid_application_name(app)
&& record.is_none_or(|record| {
record.application_name == app
&& record.values.iter().all(|value| match value {
XDataValue::String(value)
| XDataValue::ControlString(value)
| XDataValue::LayerName(value) => !value.contains('\0'),
XDataValue::Point3D(value)
| XDataValue::Position3D(value)
| XDataValue::Displacement3D(value)
| XDataValue::Direction3D(value) => finite(value),
XDataValue::Real(value)
| XDataValue::Distance(value)
| XDataValue::ScaleFactor(value) => value.is_finite(),
XDataValue::BinaryData(_)
| XDataValue::Handle(_)
| XDataValue::Integer16(_)
| XDataValue::Integer32(_) => true,
})
})
}
fn valid_xrecord(spec: &XRecordSpec) -> bool {
!spec.name.trim().is_empty()
&& !spec.name.contains('\0')
&& spec.entries.len() <= BULK_ITEM_CAP
&& spec.entries.iter().all(|entry| {
let code = entry.code;
match &entry.value {
XRecordValue::String(value) => {
matches!(
code,
0..=4
| 6..=9
| 100..=102
| 300..=309
| 410..=419
| 430..=439
| 470..=479
| 999
| 1000..=1003
) && !value.contains('\0')
&& value.encode_utf16().count() <= u16::MAX as usize
}
XRecordValue::Point3D(value) => {
matches!(code, 10..=37 | 110..=139 | 210..=269 | 1010..=1039 | 1043..=1069)
&& finite(value)
}
XRecordValue::Double(value) => {
matches!(code, 38..=59 | 140..=149 | 460..=469 | 1040..=1042)
&& value.is_finite()
}
XRecordValue::Int16(_) => {
matches!(code, 60..=79 | 170..=179 | 270..=279 | 370..=389 | 400..=409 | 1070)
}
XRecordValue::Int32(_) => matches!(code, 80..=99 | 420..=429 | 440..=459 | 1071),
XRecordValue::Int64(_) => matches!(code, 150..=169),
XRecordValue::Byte(_) => matches!(code, 280..=289),
XRecordValue::Bool(_) => matches!(code, 290..=299),
XRecordValue::Handle(_) => {
code < 0 || matches!(code, 5 | 105 | 320..=369 | 390..=399 | 480..=481 | 1005)
}
XRecordValue::Chunk(value) => {
matches!(code, 310..=319 | 1004) && value.len() <= u8::MAX as usize
}
}
})
}
pub(crate) fn curve(spec: &Curve2Spec) -> ApiResult<()> {
use Curve2Spec::*;
let valid = match spec {
@@ -116,11 +198,36 @@ pub(crate) fn operation(op: &Operation) -> ApiResult<()> {
}
CreateText(s) => finite(&s.insertion_point) && positive(s.height) && s.rotation.is_finite(),
CreateMText(s) => finite(&s.insertion_point) && positive(s.height),
SetXDataMany(records) => records.iter().all(|(id, app, record)| {
let app_ok = !app.is_empty();
let record_ok = record.as_ref().map_or(true, |r| !r.application_name.is_empty() && !r.application_name.contains('\0'));
*id != ObjectId::NULL && app_ok && record_ok
}),
SetXData {
id,
application_name,
record,
} => {
return check_payload(
*id != ObjectId::NULL && valid_xdata_record(application_name, record.as_ref()),
"SetXData",
"invalid XDATA record",
)
}
SetXDataMany(records) => {
return check_payload(
records.iter().all(|(id, app, record)| {
*id != ObjectId::NULL && valid_xdata_record(app, record.as_ref())
}),
"SetXDataMany",
"invalid XDATA record",
)
}
CreateXRecord(spec) => {
return check_payload(valid_xrecord(spec), "CreateXRecord", "invalid XRecord payload")
}
SetXRecord { id, spec } => {
return check_payload(
*id != ObjectId::NULL && valid_xrecord(spec),
"SetXRecord",
"invalid XRecord payload",
)
}
CreateHatch(s) => {
(3..=BULK_ITEM_CAP).contains(&s.boundary.len()) && s.boundary.iter().all(|p| finite(p))
}
crates/ocs_doc_api/tests/mock_backend.rs
+33
View File
@@ -985,6 +985,39 @@ fn set_xdata_many_with_unknown_id_fails_atomically() {
assert!(doc.entities().get(line.id()).unwrap().xdata("APP1").unwrap().is_none());
}
#[test]
fn xdata_rejects_a_mismatched_record_name() {
let (api, _tp) = api();
let doc = api.document(api.active_tab());
let line = doc.curves().create_line([0.0; 3], [1.0; 3]).unwrap();
let entity = doc.entities().get(line.id()).unwrap();
let result = entity.set_xdata(
"EXPECTED",
Some(XDataRecord {
application_name: "OTHER".into(),
values: vec![],
}),
);
assert!(matches!(result, Err(ApiError::Validation { .. })));
assert!(entity.xdata("EXPECTED").unwrap().is_none());
}
#[test]
fn create_xrecord_returns_the_typed_handle() {
let (api, _tp) = api();
let doc = api.document(api.active_tab());
let spec = ocs_doc_api::XRecordSpec {
name: "PLUGIN_DATA".into(),
cloning_flags: ocs_doc_api::XRecordCloningFlags::NotApplicable,
entries: vec![ocs_doc_api::XRecordEntry {
code: 1,
value: ocs_doc_api::XRecordValue::String("value".into()),
}],
};
let record = doc.entities().create_xrecord(&spec).unwrap();
assert_eq!(record.payload().unwrap(), spec);
}
#[test]
fn create_many_applies_per_curve_layer() {
let (api, _tp) = api();
src/app/doc_api.rs
+181 -20
View File
@@ -48,6 +48,28 @@ fn xrecord_dictionary_handle(doc: &mut acadrust::CadDocument) -> Handle {
}
}
fn xrecord_owner_dictionary(
doc: &acadrust::CadDocument,
record_handle: Handle,
owner: Handle,
) -> Option<Handle> {
let owns_record = |handle: Handle| {
matches!(doc.objects.get(&handle), Some(ObjectType::Dictionary(dict)) if
dict.entries.iter().any(|(_, child)| *child == record_handle))
};
if owns_record(owner) {
return Some(owner);
}
doc.objects.iter().find_map(|(handle, object)| match object {
ObjectType::Dictionary(dict)
if dict.entries.iter().any(|(_, child)| *child == record_handle) =>
{
Some(*handle)
}
_ => None,
})
}
/// Entry point called by the `HostApi::doc_api_dispatch` override (below) and by
// the in-process path. Deserializes the envelope, runs the crate executor,
// serializes the `Receipt` (or `ApiError`) back to bytes.
@@ -771,7 +793,17 @@ impl DocApiBackend for HostSession<'_> {
fn add_xrecord(&mut self, spec: &ocs_doc_api::XRecordSpec) -> ApiResult<ObjectId> {
use acadrust::objects::{ObjectType, XRecord};
let dict_h = xrecord_dictionary_handle(self.document_mut());
if matches!(self.document().objects.get(&dict_h), Some(ObjectType::Dictionary(dict)) if
dict.entries.iter().any(|(name, _)| name.eq_ignore_ascii_case(&spec.name)))
{
return Err(ApiError::validation(
"CreateXRecord",
format!("XRecord '{}' already exists", spec.name),
));
}
let mut record = XRecord::named(spec.name.clone());
record.owner = dict_h;
record.cloning_flags = cloning_flags_to_acadrust(spec.cloning_flags);
record.entries = spec
.entries
@@ -789,9 +821,7 @@ impl DocApiBackend for HostSession<'_> {
};
// Attach the new XRecord to the stable named-object dictionary so it is
// reachable as a standalone named object.
let dict_h = xrecord_dictionary_handle(self.document_mut());
if let Some(ObjectType::Dictionary(dict)) = self.document_mut().objects.get_mut(&dict_h) {
dict.entries.retain(|(name, _)| !name.eq_ignore_ascii_case(&spec.name));
dict.add_entry(spec.name.clone(), record_handle);
}
Ok(handle_to_obj(record_handle))
@@ -808,9 +838,21 @@ impl DocApiBackend for HostSession<'_> {
let Some(ObjectType::XRecord(record)) = self.document().objects.get(&handle) else {
return Err(ApiError::Unsupported(format!("ObjectId {id:?} is not an XRecord")));
};
let old_name = record.name.clone();
let mut updated = record.clone();
let dict_h = xrecord_owner_dictionary(self.document(), handle, record.owner)
.unwrap_or_else(|| xrecord_dictionary_handle(self.document_mut()));
if matches!(self.document().objects.get(&dict_h), Some(ObjectType::Dictionary(dict)) if
dict.entries.iter().any(|(name, child)| {
*child != handle && name.eq_ignore_ascii_case(&spec.name)
}))
{
return Err(ApiError::validation(
"SetXRecord",
format!("XRecord '{}' already exists", spec.name),
));
}
updated.name = spec.name.clone();
updated.owner = dict_h;
updated.cloning_flags = cloning_flags_to_acadrust(spec.cloning_flags);
updated.entries = spec
.entries
@@ -821,16 +863,9 @@ impl DocApiBackend for HostSession<'_> {
self.document_mut()
.objects
.insert(handle, acadrust::objects::ObjectType::XRecord(updated));
// If the name changed, rename the dictionary entry so the XRecord remains
// reachable under its new name.
if !old_name.eq_ignore_ascii_case(&spec.name) {
let dict_h = xrecord_dictionary_handle(self.document_mut());
if let Some(ObjectType::Dictionary(dict)) =
self.document_mut().objects.get_mut(&dict_h)
{
dict.entries.retain(|(name, _)| !name.eq_ignore_ascii_case(&old_name));
dict.add_entry(spec.name.clone(), handle);
}
if let Some(ObjectType::Dictionary(dict)) = self.document_mut().objects.get_mut(&dict_h) {
dict.entries.retain(|(_, child)| *child != handle);
dict.add_entry(spec.name.clone(), handle);
}
Ok(())
}
@@ -906,7 +941,7 @@ impl DocApiBackend for HostSession<'_> {
Ok(())
}
fn delete_layer(&mut self, name: &str) -> ApiResult<()> {
let name_upper = acadrust::tables::normalize_name(name);
let name_upper = acadrust::tables::normalize_name(name.trim());
if name_upper == "0" {
return Err(ApiError::validation("DeleteLayer", "cannot delete layer 0"));
}
@@ -1008,7 +1043,9 @@ impl DocApiBackend for HostSession<'_> {
}
}
if let Some(l) = layer {
if !entity.common().layer.eq_ignore_ascii_case(l) {
if acadrust::tables::normalize_name(&entity.common().layer)
!= acadrust::tables::normalize_name(l.trim())
{
continue;
}
}
@@ -1648,8 +1685,14 @@ mod tests {
use super::*;
use crate::app::plugin_host::HostSession;
use crate::app::OpenCADStudio;
use ocs_doc_api::ops::{BoolOp, Color, LayerFlags, LayerInfo, LineWeight, SolidPrimitive};
use ocs_doc_api::{DocApiEnvelope, HasId, ObjectId, Operation, Query, QueryResult, Receipt};
use ocs_doc_api::ops::{
BoolOp, Color, LayerFlags, LayerInfo, LineWeight, SolidPrimitive, XDataRecord,
XDataValue, XRecordEntry, XRecordSpec, XRecordValue,
};
use ocs_doc_api::{
DocApiEnvelope, HasId, ObjectId, Operation, Query, QueryResult, Receipt,
XRecordCloningFlags,
};
fn dispatch(host: &mut HostSession<'_>, env: DocApiEnvelope) -> ApiResult<Receipt> {
let bytes = bincode::serialize(&env).unwrap();
@@ -4166,7 +4209,7 @@ mod tests {
dispatch(
&mut host,
DocApiEnvelope::op(Operation::UpdateLayer {
name: "Walls".into(),
name: " Walls ".into(),
info: updated.clone(),
}),
)
@@ -4210,7 +4253,7 @@ mod tests {
dispatch(
&mut host,
DocApiEnvelope::op(Operation::DeleteLayer {
name: "Walls".into(),
name: " Walls ".into(),
}),
)
.unwrap();
@@ -4221,6 +4264,124 @@ mod tests {
assert!(!list.iter().any(|l| l.name == "Walls"));
}
#[test]
fn doc_api_xdata_batch_rejects_a_locked_entity_before_mutating() {
let mut app = OpenCADStudio::new_for_test();
let mut host = HostSession::new(&mut app, 0);
let line = |host: &mut HostSession<'_>| {
new_id(&dispatch(
host,
DocApiEnvelope::op(Operation::CreateCurve(ocs_doc_api::Curve2Spec::Line {
start: [0.0; 3],
end: [1.0; 3],
layer: None,
})),
).unwrap())
};
let first = line(&mut host);
let locked = line(&mut host);
let mut layer = LayerInfo::new("Locked");
dispatch(&mut host, DocApiEnvelope::op(Operation::CreateLayer(layer.clone()))).unwrap();
dispatch(
&mut host,
DocApiEnvelope::op(Operation::SetEntityLayer {
id: locked,
layer: layer.name.clone(),
}),
)
.unwrap();
layer.flags.locked = true;
dispatch(
&mut host,
DocApiEnvelope::op(Operation::UpdateLayer {
name: layer.name.clone(),
info: layer,
}),
)
.unwrap();
let record = |application_name: &str| XDataRecord {
application_name: application_name.into(),
values: vec![XDataValue::Integer32(42)],
};
let result = dispatch(
&mut host,
DocApiEnvelope::op(Operation::SetXDataMany(vec![
(first, "FIRST".into(), Some(record("FIRST"))),
(locked, "LOCKED".into(), Some(record("LOCKED"))),
])),
);
assert!(matches!(result, Err(ApiError::Validation { .. })));
assert!(host.xdata(first, "FIRST").unwrap().is_none());
}
#[test]
fn doc_api_xrecords_keep_one_named_owner() {
let mut app = OpenCADStudio::new_for_test();
let mut host = HostSession::new(&mut app, 0);
let spec = |name: &str| XRecordSpec {
name: name.into(),
cloning_flags: XRecordCloningFlags::NotApplicable,
entries: vec![XRecordEntry {
code: 1,
value: XRecordValue::String("value".into()),
}],
};
let first = new_id(&dispatch(
&mut host,
DocApiEnvelope::op(Operation::CreateXRecord(spec("FIRST"))),
).unwrap());
let first_handle = obj_to_handle(first);
let ObjectType::XRecord(first_record) = host.document().objects.get(&first_handle).unwrap()
else {
panic!("expected XRecord")
};
let owner = first_record.owner;
assert!(matches!(host.document().objects.get(&owner), Some(ObjectType::Dictionary(dict)) if
dict.entries.iter().any(|(name, child)| name == "FIRST" && *child == first_handle)));
let object_count = host.document().objects.len();
assert!(matches!(
dispatch(
&mut host,
DocApiEnvelope::op(Operation::CreateXRecord(spec("first"))),
),
Err(ApiError::Validation { .. })
));
assert_eq!(host.document().objects.len(), object_count);
let second = new_id(&dispatch(
&mut host,
DocApiEnvelope::op(Operation::CreateXRecord(spec("SECOND"))),
).unwrap());
assert!(matches!(
dispatch(
&mut host,
DocApiEnvelope::op(Operation::SetXRecord {
id: second,
spec: spec("FIRST"),
}),
),
Err(ApiError::Validation { .. })
));
assert_eq!(host.xrecord(second).unwrap().unwrap().name, "SECOND");
let mut invalid = spec("INVALID");
invalid.entries[0] = XRecordEntry {
code: 1,
value: XRecordValue::Int32(7),
};
assert!(matches!(
dispatch(
&mut host,
DocApiEnvelope::op(Operation::CreateXRecord(invalid)),
),
Err(ApiError::Validation { .. })
));
}
#[test]
fn doc_api_enumerate_entities_and_geometry_queries() {
let mut app = OpenCADStudio::new_for_test();
@@ -4297,7 +4458,7 @@ mod tests {
&mut host,
DocApiEnvelope::queries(vec![Query::EnumerateEntities {
kind: None,
layer: Some("Markers".into()),
layer: Some(" Markers ".into()),
include_bounds: true,
}]),
)