From bcd1f2604a8641e2f45217955a88c568bcd112eb Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Wed, 30 Sep 2026 16:33:19 +0200 Subject: [PATCH 1/4] fix: decoder keeps the construction kind of REDUCE-created instances (#32) `InstanceData` gains `newobj` (true for `NEWOBJ`, the default everywhere). The `BUILD` arm sets it from the popped `Reduce` when the arguments are empty, so `GLOBAL () REDUCE state BUILD` and `GLOBAL () NEWOBJ state BUILD` no longer collapse into the same node; a second `BUILD` keeps the flag. Nothing reads it yet. Assisted-by: Claude Fable 5.1 --- src/decode.rs | 37 +++++++++++++++++++++++++++++++++++++ src/encode.rs | 5 ++++- src/json.rs | 14 +++++++++++++- src/known_types.rs | 2 ++ src/pyconv.rs | 7 ++++++- src/types.rs | 4 ++++ 6 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/decode.rs b/src/decode.rs index beab141..bf5e24e 100644 --- a/src/decode.rs +++ b/src/decode.rs @@ -679,6 +679,7 @@ impl<'a> Decoder<'a> { state: Box::new(state), dict_items: None, list_items: None, + newobj: true, })), build_depth); } PickleValue::Instance(inst) => { @@ -689,6 +690,7 @@ impl<'a> Decoder<'a> { state: Box::new(state), dict_items: inst.dict_items, list_items: inst.list_items, + newobj: inst.newobj, })), build_depth); } PickleValue::Reduce { @@ -711,6 +713,7 @@ impl<'a> Decoder<'a> { state: Box::new(state), dict_items, list_items, + newobj, })), build_depth); } else if newobj { // NEWOBJ with constructor args: the stored shape keeps @@ -725,6 +728,7 @@ impl<'a> Decoder<'a> { ])), dict_items, list_items, + newobj: true, })), Self::nest(build_depth)?); } else { // REDUCE with args then BUILD: re-emitting as NEWOBJ would @@ -766,6 +770,7 @@ impl<'a> Decoder<'a> { ])), dict_items: None, list_items: None, + newobj: true, })), Self::nest(build_depth)?); } } @@ -1434,6 +1439,7 @@ mod tests { )])), dict_items: None, list_items: None, + newobj: true, })); // First element: the Instance from BUILD (stack top) @@ -1773,6 +1779,37 @@ mod tests { } } + #[test] + fn test_reduce_empty_args_build_keeps_reduce_kind() { + // GLOBAL m.C, EMPTY_TUPLE, REDUCE, {"x": 1}, BUILD: a __reduce__ returning (cls, (), state) + let data = b"\x80\x03cm\nC\n)R}X\x01\x00\x00\x00xK\x01sb."; + match decode_pickle(data).unwrap() { + PickleValue::Instance(inst) => { + assert_eq!(inst.module, "m"); + assert!(!inst.newobj, "REDUCE-created instance must not be marked newobj"); + } + other => panic!("expected Instance, got {other:?}"), + } + // NEWOBJ stays newobj + let data = b"\x80\x03cm\nC\n)\x81}X\x01\x00\x00\x00xK\x01sb."; + match decode_pickle(data).unwrap() { + PickleValue::Instance(inst) => assert!(inst.newobj), + other => panic!("expected Instance, got {other:?}"), + } + // a second BUILD keeps the kind + let data = b"\x80\x03cm\nC\n)R}X\x01\x00\x00\x00xK\x01sb}X\x01\x00\x00\x00yK\x02sb."; + match decode_pickle(data).unwrap() { + PickleValue::Instance(inst) => { + assert!(!inst.newobj); + assert_eq!( + *inst.state, + PickleValue::Dict(vec![(PickleValue::String("y".into()), PickleValue::Int(2))]) + ); + } + other => panic!("expected Instance, got {other:?}"), + } + } + #[test] fn test_newobj_without_build_is_flagged() { let data: &[u8] = &[0x80, 0x03, b'c', b'm', b'\n', b'C', b'\n', 0x8c, 0x01, b'a', 0x85, 0x81, b'.']; diff --git a/src/encode.rs b/src/encode.rs index 2802797..d4fa3c8 100644 --- a/src/encode.rs +++ b/src/encode.rs @@ -339,7 +339,7 @@ impl Encoder { self.write_u8(b'\n'); } PickleValue::Instance(inst) => { - let InstanceData { module, name, state, dict_items, list_items } = inst.as_ref(); + let InstanceData { module, name, state, dict_items, list_items, newobj } = inst.as_ref(); if module.is_empty() && name.is_empty() { return self.encode_anonymous_instance(state, depth); } @@ -586,6 +586,7 @@ mod tests { )])), dict_items: Some(Box::new(vec![(PickleValue::String("k".into()), PickleValue::Int(2))])), list_items: None, + newobj: true, })); let bytes = encode_pickle(&val).unwrap(); // CPython's save_reduce: NEWOBJ, then the items (MARK ... SETITEMS), then state + BUILD. @@ -609,6 +610,7 @@ mod tests { state: Box::new(PickleValue::Dict(vec![])), dict_items: Some(Box::new(vec![(PickleValue::Int(1), PickleValue::Int(2))])), list_items: Some(Box::new(vec![PickleValue::Int(3)])), + newobj: true, })); let bytes = encode_pickle(&val).unwrap(); let newobj = bytes.iter().position(|&b| b == NEWOBJ).unwrap(); @@ -659,6 +661,7 @@ mod tests { ])), dict_items: None, list_items: None, + newobj: true, })); let bytes = encode_pickle(&val).unwrap(); let newobj = bytes.iter().position(|&b| b == NEWOBJ).unwrap(); diff --git a/src/json.rs b/src/json.rs index 926bfd8..5a990bd 100644 --- a/src/json.rs +++ b/src/json.rs @@ -109,7 +109,7 @@ fn pickle_value_to_json_impl( Ok(json!({"@cls": [module, name]})) } PickleValue::Instance(inst) => { - let InstanceData { module, name, state, dict_items, list_items } = inst.as_ref(); + let InstanceData { module, name, state, dict_items, list_items, newobj } = inst.as_ref(); if let Some(typed) = known_types::try_instance_to_typed_json(module, name, state, &to_json)? { @@ -442,6 +442,7 @@ fn write_value_pg_depth(w: &mut JsonWriter, val: &PickleValue, depth: usize) -> state, dict_items, list_items, + newobj, } = inst.as_ref(); // Try known type handlers first @@ -768,6 +769,7 @@ pub fn json_to_pickle_value(val: &Value) -> Result { state: Box::new(json_to_pickle_value(v)?), dict_items: None, list_items: None, + newobj: true, }))); } if map.contains_key("@cls") && map.contains_key("@s") { @@ -815,6 +817,7 @@ pub fn json_to_pickle_value(val: &Value) -> Result { state: Box::new(state), dict_items, list_items, + newobj: true, }))); } } @@ -959,6 +962,7 @@ mod tests { )])), dict_items: None, list_items: None, + newobj: true, })); let json = pickle_value_to_json(&val).unwrap(); assert_eq!(json["@cls"][0], "myapp"); @@ -979,6 +983,7 @@ mod tests { (PickleValue::String("b".to_string()), PickleValue::Int(2)), ])), list_items: None, + newobj: true, })); let json = pickle_value_to_json(&val).unwrap(); assert_eq!(json["@cls"][0], "collections"); @@ -1000,6 +1005,7 @@ mod tests { state: Box::new(PickleValue::None), dict_items: None, list_items: Some(Box::new(vec![PickleValue::Int(10), PickleValue::Int(20)])), + newobj: true, })); let json = pickle_value_to_json(&val).unwrap(); assert!(json.get("@appends").is_some()); @@ -1365,6 +1371,7 @@ mod tests { )])), dict_items: None, list_items: None, + newobj: true, })); assert_pg_paths_match(&inst, "", ""); } @@ -1379,6 +1386,7 @@ mod tests { (PickleValue::String("a".into()), PickleValue::Int(1)), ])), list_items: None, + newobj: true, })); assert_pg_paths_match(&inst, "", ""); } @@ -1391,6 +1399,7 @@ mod tests { state: Box::new(PickleValue::None), dict_items: None, list_items: Some(Box::new(vec![PickleValue::Int(10)])), + newobj: true, })); assert_pg_paths_match(&inst, "", ""); } @@ -1717,6 +1726,7 @@ mod tests { )])), dict_items: None, list_items: None, + newobj: true, })); assert_pg_paths_match(&val, "", ""); } @@ -1733,6 +1743,7 @@ mod tests { )])), dict_items: None, list_items: None, + newobj: true, })); assert_pg_paths_match(&val, "", ""); } @@ -1983,6 +1994,7 @@ mod tests { state: Box::new(PickleValue::Int(42)), dict_items: None, list_items: None, + newobj: true, })); assert_pg_paths_match(&inst, "", ""); } diff --git a/src/known_types.rs b/src/known_types.rs index 3d5316e..ac7394a 100644 --- a/src/known_types.rs +++ b/src/known_types.rs @@ -1173,6 +1173,7 @@ fn try_decode_uuid(val: &Value) -> Result { )])), dict_items: None, list_items: None, + newobj: true, }))) } @@ -1683,6 +1684,7 @@ mod tests { )])), dict_items: None, list_items: None, + newobj: true, })); let json = pickle_value_to_json(&instance).unwrap(); assert_eq!(json, json!({"@uuid": "12345678-1234-5678-1234-567812345678"})); diff --git a/src/pyconv.rs b/src/pyconv.rs index 3ab0e01..6b2c037 100644 --- a/src/pyconv.rs +++ b/src/pyconv.rs @@ -309,7 +309,7 @@ fn pickle_value_to_pyobject_impl( Ok(dict.into_any().unbind()) } PickleValue::Instance(inst) => { - let InstanceData { module, name, state, dict_items, list_items } = inst.as_ref(); + let InstanceData { module, name, state, dict_items, list_items, newobj } = inst.as_ref(); // Try known type handlers first (e.g., uuid.UUID) if let Some(obj) = try_instance_to_pyobject(py, module, name, state, compact_refs)? @@ -1156,6 +1156,7 @@ fn pydict_to_pickle_value( state: Box::new(state), dict_items: dict_items_from_pyobject(dict.get_item(intern!(py, "@items"))?, expand_refs, "@items")?, list_items: list_items_from_pyobject(dict.get_item(intern!(py, "@appends"))?, expand_refs, "@appends")?, + newobj: true, }))); } if dict.get_item(intern!(py, "@items"))?.is_some() @@ -1357,6 +1358,7 @@ fn try_decode_single_key_marker( state: Box::new(pyobject_to_pickle_value(v, expand_refs)?), dict_items: None, list_items: None, + newobj: true, })))); } "@set" => { @@ -1848,6 +1850,7 @@ fn decode_uuid_from_str(s: &str) -> PyResult { )])), dict_items: None, list_items: None, + newobj: true, }))) } @@ -2409,6 +2412,7 @@ fn try_encode_marker_to_pickle( state: Box::new(pyobject_to_pickle_value(v, expand_refs)?), dict_items: None, list_items: None, + newobj: true, })); encode_value_into(&pv, buf)?; Ok(true) @@ -2960,6 +2964,7 @@ mod tests { ])), dict_items: None, list_items: None, + newobj: true, })); let mut refs = Vec::new(); collect_refs_from_pickle_value(&val, &mut refs); diff --git a/src/types.rs b/src/types.rs index bbcbefb..163344a 100644 --- a/src/types.rs +++ b/src/types.rs @@ -11,6 +11,10 @@ pub struct InstanceData { pub dict_items: Option>>, /// List items appended via APPENDS/APPEND after BUILD (list subclasses) pub list_items: Option>>, + /// False when the pickle created the object with `REDUCE` (a call of the + /// class, so `__init__` runs on unpickling) rather than `NEWOBJ`; the + /// encoders emit the same opcode again (#32). + pub newobj: bool, } /// Intermediate representation of a pickle value. From 78bb82165137bf9c16625953bc36867f1fedf1fc Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Wed, 30 Sep 2026 16:36:34 +0200 Subject: [PATCH 2/4] fix: REDUCE-created instances re-encode with REDUCE, @newobj flag in JSON (#32) Both JSON writers add `"@newobj": false` next to `@cls`/`@s` when the instance was created by calling the class; the JSON reader and the Python dict reader take an absent key (stored data) or any value but `false` as `NEWOBJ`. Both pickle encoders emit `REDUCE` for the false kind, items before `BUILD` as before; the direct dict encoder routes such dicts through the `PickleValue` path. The 1,692-record sample produces identical PG JSON, dict-path output and re-encoded pickles (no such instance in it). Assisted-by: Claude Fable 5.1 --- CHANGES.md | 8 +++ src/encode.rs | 32 +++++++++- src/json.rs | 10 +++- src/pyconv.rs | 15 +++-- tests/test_reduce_kind.py | 123 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 tests/test_reduce_kind.py diff --git a/CHANGES.md b/CHANGES.md index f968cd4..874b040 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,14 @@ ## unreleased +- Instances that a pickler wrote as `REDUCE(cls, ()) + BUILD` (a `__reduce__` + returning the class with empty arguments, for example `OrderedDict` + subclasses with a `__dict__`) re-encode as `REDUCE` again, so `__init__` + runs on unpickling as it did for the original pickle; they used to come + back as `NEWOBJ`. The JSON carries `"@newobj": false` next to `@cls`/`@s` + for such instances; absent means `NEWOBJ`, so stored data is unaffected + [#32] + - Docs: journal entry 22 explains the PGO slowdown of the PG JSON path (the profile-guided build put the end-of-slice check on the pointer step of the string loop) and the performance page is re-measured with the fixed diff --git a/src/encode.rs b/src/encode.rs index d4fa3c8..38a3ecd 100644 --- a/src/encode.rs +++ b/src/encode.rs @@ -359,7 +359,7 @@ impl Encoder { Some(args) => self.encode_value(args, depth + 1)?, None => self.write_u8(EMPTY_TUPLE), } - self.write_u8(NEWOBJ); + self.write_u8(if *newobj { NEWOBJ } else { REDUCE }); // Dict/list subclass items go before the state, in the order CPython's // save_reduce writes them: APPENDS, then SETITEMS, then BUILD. if let Some(items) = list_items { @@ -622,6 +622,36 @@ mod tests { assert_eq!(decode_pickle(&bytes).unwrap(), val); } + #[test] + fn test_reduce_kind_instance_emits_reduce() { + let val = PickleValue::Instance(Box::new(InstanceData { + module: "m".into(), + name: "C".into(), + state: Box::new(PickleValue::Dict(vec![])), + dict_items: None, + list_items: None, + newobj: false, + })); + let bytes = encode_pickle(&val).unwrap(); + assert!(bytes.contains(&REDUCE) && !bytes.contains(&NEWOBJ)); + // with the @args/@state shape: GLOBAL args REDUCE state BUILD + let val = PickleValue::Instance(Box::new(InstanceData { + module: "m".into(), + name: "C".into(), + state: Box::new(PickleValue::Dict(vec![ + (PickleValue::String("@args".into()), PickleValue::Tuple(vec![PickleValue::Int(1)])), + (PickleValue::String("@state".into()), PickleValue::Dict(vec![])), + ])), + dict_items: None, + list_items: None, + newobj: false, + })); + let bytes = encode_pickle(&val).unwrap(); + let r = bytes.iter().position(|&b| b == REDUCE).unwrap(); + let b = bytes.iter().position(|&b| b == BUILD).unwrap(); + assert!(r < b && !bytes.contains(&NEWOBJ)); + } + fn reduce(newobj: bool, state: Option) -> PickleValue { PickleValue::Reduce { callable: Box::new(PickleValue::Global { module: "m".into(), name: "C".into() }), diff --git a/src/json.rs b/src/json.rs index 5a990bd..91b2098 100644 --- a/src/json.rs +++ b/src/json.rs @@ -127,6 +127,9 @@ fn pickle_value_to_json_impl( "@cls": [module, name], "@s": state_json, }); + if !newobj { + obj.as_object_mut().unwrap().insert("@newobj".to_string(), json!(false)); + } if let Some(pairs) = dict_items { let items_json: Result, CodecError> = pairs .iter() @@ -479,6 +482,11 @@ fn write_value_pg_depth(w: &mut JsonWriter, val: &PickleValue, depth: usize) -> } else { recurse(w, state)?; } + if !newobj { + w.write_comma(); + w.write_key_literal("@newobj"); + w.write_bool(false); + } if let Some(pairs) = dict_items { w.write_comma(); w.write_key_literal("@items"); @@ -817,7 +825,7 @@ pub fn json_to_pickle_value(val: &Value) -> Result { state: Box::new(state), dict_items, list_items, - newobj: true, + newobj: !matches!(map.get("@newobj"), Some(Value::Bool(false))), }))); } } diff --git a/src/pyconv.rs b/src/pyconv.rs index 6b2c037..22b34e2 100644 --- a/src/pyconv.rs +++ b/src/pyconv.rs @@ -332,6 +332,9 @@ fn pickle_value_to_pyobject_impl( let dict = PyDict::new(py); dict.set_item(intern!(py, "@cls"), cls_list)?; dict.set_item(intern!(py, "@s"), state_obj)?; + if !newobj { + dict.set_item(intern!(py, "@newobj"), false)?; + } // SETITEMS/APPENDS data of dict/list subclasses (same keys as the JSON writer) if let Some(pairs) = dict_items { dict.set_item(intern!(py, "@items"), pairs_to_pylist(py, pairs, compact_refs, sanitize_nulls, depth)?)?; @@ -1156,7 +1159,10 @@ fn pydict_to_pickle_value( state: Box::new(state), dict_items: dict_items_from_pyobject(dict.get_item(intern!(py, "@items"))?, expand_refs, "@items")?, list_items: list_items_from_pyobject(dict.get_item(intern!(py, "@appends"))?, expand_refs, "@appends")?, - newobj: true, + newobj: !matches!( + dict.get_item(intern!(py, "@newobj"))?.map(|v| v.extract::()), + Some(Ok(false)) + ), }))); } if dict.get_item(intern!(py, "@items"))?.is_some() @@ -2266,12 +2272,13 @@ fn encode_pydict_to_pickle( let module = mod_py.to_str()?; let name = name_py.to_str()?; - // Instances with SETITEMS/APPENDS data need the PickleValue path, - // which emits items before BUILD like CPython does. + // Instances with SETITEMS/APPENDS data or the REDUCE kind (#32) need the + // PickleValue path, which emits items before BUILD like CPython does. // Only a 3- or 4-key dict can carry them; skip the lookups otherwise. if len > 2 && (dict.get_item(intern!(py, "@items"))?.is_some() - || dict.get_item(intern!(py, "@appends"))?.is_some()) + || dict.get_item(intern!(py, "@appends"))?.is_some() + || dict.get_item(intern!(py, "@newobj"))?.is_some()) { let pv = pydict_to_pickle_value(dict, expand_refs)?; encode_value_into(&pv, buf)?; diff --git a/tests/test_reduce_kind.py b/tests/test_reduce_kind.py new file mode 100644 index 0000000..08edc49 --- /dev/null +++ b/tests/test_reduce_kind.py @@ -0,0 +1,123 @@ +"""Instances created by REDUCE(cls, ()) + BUILD re-encode with REDUCE, so __init__ runs again (#32).""" + +import json +import pickle +import pickletools +from collections import OrderedDict + +import zodb_json_codec + +INIT_CALLS = [] + + +class Counted: + """__reduce__ returns (cls, (), state): unpickling calls cls(), which runs __init__.""" + + def __init__(self): + INIT_CALLS.append(1) + self.x = 0 + + def __reduce__(self): + return (Counted, (), {"x": self.x}) + + def __setstate__(self, state): + self.x = state["x"] + + +class Plain: + """Default reduce: NEWOBJ + BUILD.""" + + +class ODSub(OrderedDict): + """Inherits OrderedDict.__reduce__: REDUCE + SETITEMS + BUILD when it has a __dict__.""" + + +def record(value): + cls = pickle.dumps(("persistent.mapping", "PersistentMapping"), protocol=3) + return cls + pickle.dumps({"data": {"v": value}}, protocol=3) + + +def opcodes(data): + return [op.name for op, _, _ in pickletools.genops(data)] + + +def test_pg_json_marks_reduce_kind(): + c = Counted() + c.x = 7 + _, _, js, _ = zodb_json_codec.decode_zodb_record_for_pg_json(record(c)) + inner = json.loads(js)["data"]["v"] + assert inner["@cls"] == [__name__, "Counted"] + assert inner["@s"] == {"x": 7} + assert inner["@newobj"] is False + + +def test_newobj_instance_has_no_flag(): + p = Plain() + p.x = 1 + _, _, js, _ = zodb_json_codec.decode_zodb_record_for_pg_json(record(p)) + inner = json.loads(js)["data"]["v"] + assert inner["@cls"] == [__name__, "Plain"] + assert "@newobj" not in inner + + +def test_json_reencode_runs_init(): + c = Counted() + c.x = 3 + data = pickle.dumps(c, protocol=3) + assert "REDUCE" in opcodes(data) and "NEWOBJ" not in opcodes(data) + back = zodb_json_codec.json_to_pickle(zodb_json_codec.pickle_to_json(data)) + ops = opcodes(back) + assert "REDUCE" in ops and "NEWOBJ" not in ops + INIT_CALLS.clear() + restored = pickle.loads(back) + assert restored.x == 3 + assert INIT_CALLS == [1], ( + "cls() must run __init__ on unpickling, as the original pickle does" + ) + + +def test_dict_path_reencode_runs_init(): + c = Counted() + c.x = 5 + data = pickle.dumps(c, protocol=3) + as_dict = zodb_json_codec.pickle_to_dict(data) + assert as_dict["@newobj"] is False + back = zodb_json_codec.dict_to_pickle(as_dict) + ops = opcodes(back) + assert "REDUCE" in ops and "NEWOBJ" not in ops + INIT_CALLS.clear() + assert pickle.loads(back).x == 5 + assert INIT_CALLS == [1] + # the ZODB record path (direct encoder) too + rec = zodb_json_codec.decode_zodb_record(record(c)) + assert rec["@s"]["data"]["v"]["@newobj"] is False + back_rec = zodb_json_codec.encode_zodb_record(rec) + assert "NEWOBJ" not in opcodes(back_rec) + + +def test_reduce_kind_with_items(): + od = ODSub([("z", 1), ("a", 2)]) + od.attr = "kept" + data = pickle.dumps(od, protocol=3) + back = zodb_json_codec.json_to_pickle(zodb_json_codec.pickle_to_json(data)) + ops = opcodes(back) + assert "REDUCE" in ops and "NEWOBJ" not in ops + assert ops.index("SETITEMS") < ops.index("BUILD") + restored = pickle.loads(back) + assert list(restored.items()) == [("z", 1), ("a", 2)] + assert restored.attr == "kept" + + +def test_stored_json_without_flag_stays_newobj(): + stored = '{"@cls": ["%s", "Counted"], "@s": {"x": 1}}' % __name__ + ops = opcodes(zodb_json_codec.json_to_pickle(stored)) + assert "NEWOBJ" in ops and "REDUCE" not in ops + + +def test_flag_other_than_false_means_newobj(): + for flag in ("true", '"no"', "null", "1"): + stored = '{"@cls": ["%s", "Counted"], "@s": {"x": 1}, "@newobj": %s}' % ( + __name__, + flag, + ) + assert "NEWOBJ" in opcodes(zodb_json_codec.json_to_pickle(stored)) From 3b51aea0a79b3a58df58552089be52483d57ddc7 Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Wed, 30 Sep 2026 16:36:35 +0200 Subject: [PATCH 3/4] docs: @newobj marker (#32) Assisted-by: Claude Fable 5.1 --- docs/sources/reference/json-format.md | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/sources/reference/json-format.md b/docs/sources/reference/json-format.md index d810a04..0f3ab8d 100644 --- a/docs/sources/reference/json-format.md +++ b/docs/sources/reference/json-format.md @@ -215,6 +215,24 @@ ZODB record. } ``` +### `@newobj` -- Construction Kind + +Written as `false`, next to `@cls`/`@s`, when the pickle created the instance +by calling the class (`REDUCE` with empty arguments, the shape of a +`__reduce__` that returns `(cls, (), state)`, for example `OrderedDict` +subclasses with a `__dict__`). Absent means the instance was created with +`NEWOBJ` (`cls.__new__(cls)`), the common case. The encoders emit the same +opcode again, so `__init__` runs on unpickling exactly when it did for the +original pickle. + +```json +{ + "@cls": ["myapp.models", "Registry"], + "@s": {"entries": 3}, + "@newobj": false +} +``` + ### `@ref` -- Persistent Reference ZODB persistent object reference, using hex OID format (16 hex digits, @@ -290,7 +308,7 @@ a state that is exactly `{"@args": ..., "@state": ...}` is always read as constructor arguments plus state. A `@reduce` with both `newobj` and `state` is written only when the class itself is not a global (`NEWOBJ` on a class pickled by value, then `BUILD`); for a global class the `@cls`/`@s` form above -is used. Readers accept both. +is used, with `@newobj` marking the `REDUCE` kind. Readers accept both. ### `@ns` -- String with NUL bytes (PostgreSQL storage) @@ -350,8 +368,8 @@ order: **Multi-key markers:** -`@cls` + `@s` (instance with BTree detection), `@dt` + `@tz` -(timezone-aware datetime) +`@cls` + `@s` (instance with BTree detection; `@items`, `@appends` and +`@newobj` ride along), `@dt` + `@tz` (timezone-aware datetime) **Fallback:** Plain JSON object becomes a Python dict. From 7aecfc02e202f1de02d310133f9afe7a499881fc Mon Sep 17 00:00:00 2001 From: "Jens W. Klein" Date: Wed, 30 Sep 2026 16:53:31 +0200 Subject: [PATCH 4/4] review pass: five-key instance dicts, instance keys without @s, comments (#32) Fresh review findings: the Python-dict readers took any dict of more than four keys for a plain dict, and `@newobj` makes an instance with `@items` and `@appends` five keys, so such an object came back as a dict of five string keys on the dict path and the record path (silent). Both guards now allow five keys, the test pushes the shape through `dict_to_pickle` and `encode_zodb_record`. Every reader rejects `@newobj`, `@items` or `@appends` on a bare class reference (the JSON reader used to drop them). Minor: stale encoder comments, the docs name the callable and the opposite default of the `@reduce` key, the dict path's non-bool flag handling and a PG writer case are tested, "unpickling" is in the spell-check vocabulary. Assisted-by: Claude Fable 5.1 --- CHANGES.md | 7 +- .../config/vocabularies/Project/accept.txt | 2 +- docs/sources/reference/json-format.md | 16 ++-- src/encode.rs | 7 +- src/json.rs | 17 ++++ src/pyconv.rs | 18 ++-- tests/test_reduce_kind.py | 91 ++++++++++++++++++- 7 files changed, 137 insertions(+), 21 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 874b040..41aae9f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -7,8 +7,11 @@ subclasses with a `__dict__`) re-encode as `REDUCE` again, so `__init__` runs on unpickling as it did for the original pickle; they used to come back as `NEWOBJ`. The JSON carries `"@newobj": false` next to `@cls`/`@s` - for such instances; absent means `NEWOBJ`, so stored data is unaffected - [#32] + for such instances; absent means `NEWOBJ`, so stored data is unaffected. + The Python-dict readers now recognise instance dicts of five keys + (`@cls`, `@s`, `@newobj`, `@items`, `@appends`), and every reader rejects + `@newobj`, `@items` or `@appends` on a bare class reference without `@s` + (the JSON reader used to drop them silently) [#32] - Docs: journal entry 22 explains the PGO slowdown of the PG JSON path (the profile-guided build put the end-of-slice check on the pointer step of diff --git a/docs/.vale/styles/config/vocabularies/Project/accept.txt b/docs/.vale/styles/config/vocabularies/Project/accept.txt index da71da7..95ba93f 100644 --- a/docs/.vale/styles/config/vocabularies/Project/accept.txt +++ b/docs/.vale/styles/config/vocabularies/Project/accept.txt @@ -144,7 +144,7 @@ special_types wide_dict [Bb]ackend[s]? [Aa]uto(detect(ed|s|ion)?|discover(ed|s|y)|creat(ed|es)) -unpickle(s|d)? +unpickl(e|es|ed|ing) mimalloc glibc pymalloc diff --git a/docs/sources/reference/json-format.md b/docs/sources/reference/json-format.md index 0f3ab8d..7b5641b 100644 --- a/docs/sources/reference/json-format.md +++ b/docs/sources/reference/json-format.md @@ -218,12 +218,16 @@ ZODB record. ### `@newobj` -- Construction Kind Written as `false`, next to `@cls`/`@s`, when the pickle created the instance -by calling the class (`REDUCE` with empty arguments, the shape of a -`__reduce__` that returns `(cls, (), state)`, for example `OrderedDict` -subclasses with a `__dict__`). Absent means the instance was created with -`NEWOBJ` (`cls.__new__(cls)`), the common case. The encoders emit the same -opcode again, so `__init__` runs on unpickling exactly when it did for the -original pickle. +by calling the callable named by `@cls`, usually the class (`REDUCE` with +empty arguments, the shape of a `__reduce__` that returns `(cls, (), state)`, +for example `OrderedDict` subclasses with a `__dict__`). Absent means the +instance was created with `NEWOBJ` (`cls.__new__(cls)`), the common case. The +encoders emit the same opcode again, so `__init__` runs on unpickling exactly +when it did for the original pickle. Note the opposite default of the +`newobj` key inside `@reduce` below: there, absent means `REDUCE`; each +default is the common case of its form. `@newobj`, like `@items` and +`@appends`, is only valid next to `@s`; readers reject it on a bare class +reference. ```json { diff --git a/src/encode.rs b/src/encode.rs index 38a3ecd..8361625 100644 --- a/src/encode.rs +++ b/src/encode.rs @@ -343,13 +343,14 @@ impl Encoder { if module.is_empty() && name.is_empty() { return self.encode_anonymous_instance(state, depth); } - // Emit as: GLOBAL module\nname\n args NEWOBJ [items] state BUILD, where args is - // EMPTY_TUPLE unless the state carries constructor args (@args/@state, #12). + // Emit as: GLOBAL module\nname\n args NEWOBJ|REDUCE [items] state BUILD, where args + // is EMPTY_TUPLE unless the state carries constructor args (@args/@state, #12) + // and the opcode follows the construction kind (#32). let (args, state) = match newobj_args_state(state) { Some((args, inner)) => (Some(args), inner), None => (None, state.as_ref()), }; - self.buf.reserve(5 + module.len() + name.len()); // GLOBAL+mod+\n+name+\n+EMPTY_TUPLE+NEWOBJ + self.buf.reserve(5 + module.len() + name.len()); // GLOBAL+mod+\n+name+\n+EMPTY_TUPLE+opcode self.write_u8(GLOBAL); self.write_bytes(module.as_bytes()); self.write_u8(b'\n'); diff --git a/src/json.rs b/src/json.rs index 91b2098..8346c03 100644 --- a/src/json.rs +++ b/src/json.rs @@ -833,6 +833,11 @@ pub fn json_to_pickle_value(val: &Value) -> Result { // Check for standalone @cls (Global reference) if let Some(Value::Array(cls)) = map.get("@cls") { if cls.len() == 2 && !map.contains_key("@s") { + if ["@items", "@appends", "@newobj"].iter().any(|k| map.contains_key(*k)) { + return Err(CodecError::InvalidData( + "@items/@appends/@newobj require an instance state (@s)".to_string(), + )); + } let module = cls[0].as_str().unwrap_or("").to_string(); let name = cls[1].as_str().unwrap_or("").to_string(); return Ok(PickleValue::Global { module, name }); @@ -1212,6 +1217,18 @@ mod tests { #[test] fn test_direct_none() { assert_pg_paths_match(&PickleValue::None, "", ""); + assert_pg_paths_match( + &PickleValue::Instance(Box::new(InstanceData { + module: "m".into(), + name: "C".into(), + state: Box::new(PickleValue::Dict(vec![(PickleValue::String("x".into()), PickleValue::Int(1))])), + dict_items: None, + list_items: None, + newobj: false, + })), + "", + "", + ); } #[test] diff --git a/src/pyconv.rs b/src/pyconv.rs index 22b34e2..17a43b1 100644 --- a/src/pyconv.rs +++ b/src/pyconv.rs @@ -1064,9 +1064,10 @@ pub fn pyobject_to_pickle_value( /// Convert a PyDict to PickleValue, checking for marker keys. /// /// Optimized dispatch: -/// - len > 4: skip marker checks (no marker dict has >4 keys) +/// - len > 5: skip marker checks (no marker dict has more than 5 keys: +/// `@cls`, `@s`, `@newobj`, `@items`, `@appends`) /// - len == 1: direct key match (avoids all hash-based get_item lookups) -/// - len 2-4: single-pass '@' scan, then targeted marker checks +/// - len 2-5: single-pass '@' scan, then targeted marker checks fn pydict_to_pickle_value( dict: &Bound<'_, PyDict>, expand_refs: bool, @@ -1074,8 +1075,8 @@ fn pydict_to_pickle_value( let py = dict.py(); let len = dict.len(); - // Fast path: no JSON marker dict has more than 4 keys. - if len > 4 { + // Fast path: no JSON marker dict has more than 5 keys (#32). + if len > 5 { return plain_dict_to_pickle_value(dict, expand_refs); } @@ -1167,9 +1168,10 @@ fn pydict_to_pickle_value( } if dict.get_item(intern!(py, "@items"))?.is_some() || dict.get_item(intern!(py, "@appends"))?.is_some() + || dict.get_item(intern!(py, "@newobj"))?.is_some() { return Err(CodecError::InvalidData( - "@items/@appends require an instance state (@s)".into(), + "@items/@appends/@newobj require an instance state (@s)".into(), ) .into()); } @@ -2221,8 +2223,8 @@ fn encode_pydict_to_pickle( ) -> PyResult<()> { let len = dict.len(); - // Fast path: no marker dict has more than 4 keys - if len > 4 { + // Fast path: no marker dict has more than 5 keys (#32) + if len > 5 { return encode_plain_dict_to_pickle(dict, buf, expand_refs); } @@ -2274,7 +2276,7 @@ fn encode_pydict_to_pickle( // Instances with SETITEMS/APPENDS data or the REDUCE kind (#32) need the // PickleValue path, which emits items before BUILD like CPython does. - // Only a 3- or 4-key dict can carry them; skip the lookups otherwise. + // Only a 3- to 5-key dict can carry them; skip the lookups otherwise. if len > 2 && (dict.get_item(intern!(py, "@items"))?.is_some() || dict.get_item(intern!(py, "@appends"))?.is_some() diff --git a/tests/test_reduce_kind.py b/tests/test_reduce_kind.py index 08edc49..b801227 100644 --- a/tests/test_reduce_kind.py +++ b/tests/test_reduce_kind.py @@ -3,6 +3,8 @@ import json import pickle import pickletools + +import pytest from collections import OrderedDict import zodb_json_codec @@ -41,6 +43,14 @@ def opcodes(data): return [op.name for op, _, _ in pickletools.genops(data)] +def state_pickle(rec): + """The second pickle of a ZODB record (genops stops at the class pickle's STOP).""" + for op, _, pos in pickletools.genops(rec): + if op.name == "STOP": + return rec[pos + 1 :] + raise AssertionError("no STOP in the class pickle") + + def test_pg_json_marks_reduce_kind(): c = Counted() c.x = 7 @@ -92,7 +102,7 @@ def test_dict_path_reencode_runs_init(): rec = zodb_json_codec.decode_zodb_record(record(c)) assert rec["@s"]["data"]["v"]["@newobj"] is False back_rec = zodb_json_codec.encode_zodb_record(rec) - assert "NEWOBJ" not in opcodes(back_rec) + assert "NEWOBJ" not in opcodes(state_pickle(back_rec)) def test_reduce_kind_with_items(): @@ -121,3 +131,82 @@ def test_flag_other_than_false_means_newobj(): flag, ) assert "NEWOBJ" in opcodes(zodb_json_codec.json_to_pickle(stored)) + + +class Both(list): + """__reduce__ with state, list items and dict items: five marker keys in JSON (#32 review).""" + + def __init__(self): + INIT_CALLS.append("both") + self.extra = {} + + def __setitem__(self, key, value): + self.extra[key] = value + + def __reduce__(self): + return (Both, (), {"tag": self.tag}, iter(list(self)), iter(self.extra.items())) + + def __setstate__(self, state): + self.tag = state["tag"] + + +def test_five_key_instance_dict_takes_the_marker_path(): + b = Both() + b.append(1) + b["k"] = "v" + b.tag = "t" + data = pickle.dumps(b, protocol=3) + as_dict = zodb_json_codec.pickle_to_dict(data) + assert set(as_dict) == {"@cls", "@s", "@newobj", "@items", "@appends"} + for back in ( + zodb_json_codec.dict_to_pickle(as_dict), + zodb_json_codec.json_to_pickle(zodb_json_codec.pickle_to_json(data)), + ): + ops = opcodes(back) + assert "NEWOBJ" not in ops + assert ( + ops.index("REDUCE") + < ops.index("APPENDS") + < ops.index("SETITEMS") + < ops.index("BUILD") + ) + INIT_CALLS.clear() + loaded = pickle.loads(back) + assert isinstance(loaded, Both) + assert (list(loaded), loaded.extra, loaded.tag, INIT_CALLS) == ( + [1], + {"k": "v"}, + "t", + ["both"], + ) + rec = zodb_json_codec.decode_zodb_record(record(b)) + assert set(rec["@s"]["data"]["v"]) == { + "@cls", + "@s", + "@newobj", + "@items", + "@appends", + } + back_rec = zodb_json_codec.encode_zodb_record(rec) + assert "NEWOBJ" not in opcodes(state_pickle(back_rec)) and "SETITEMS" in opcodes( + state_pickle(back_rec) + ) + + +def test_dict_path_flag_other_than_false_means_newobj(): + for flag in (True, None, 0, "false"): + d = {"@cls": [__name__, "Counted"], "@s": {"x": 1}, "@newobj": flag} + assert "NEWOBJ" in opcodes(zodb_json_codec.dict_to_pickle(d)) + rec = {"@cls": ["persistent.mapping", "PersistentMapping"], "@s": {"v": d}} + assert "NEWOBJ" in opcodes( + state_pickle(zodb_json_codec.encode_zodb_record(rec)) + ) + + +def test_instance_keys_without_state_are_rejected(): + for key, value in (("@newobj", False), ("@items", []), ("@appends", [])): + bad = {"@cls": [__name__, "Counted"], key: value} + with pytest.raises(ValueError): + zodb_json_codec.json_to_pickle(json.dumps(bad)) + with pytest.raises(ValueError): + zodb_json_codec.dict_to_pickle(bad)