Extract reusable round-trip helper for the #356 decoupling

The serialize/mutate/graft round-trip that decouples lxml from xmlsec
(issue #356) was inlined in template.add_reference and would have been
copy-pasted into every remaining node-passing function.

Pull it into PyXmlSec_LxmlAddChildViaXmlSec in src/lxml.c: it serializes
the element, runs a caller-supplied `op` on a throwaway xmlsec-owned copy,
dumps the whole mutated document, re-parses it with lxml, locates the new
node by its element-index path, and grafts it into the live tree. Dumping
the whole document (not the lone subtree) is what keeps the result
byte-identical without manual namespace/tail fix-up.

Converting a function is now just writing an `op`; add_reference shrinks
to arg-parse + one helper call. The helper covers the xmlSecTmpl*Add*
family (one new node appended under an existing parent); find-or-create
and intermediate-ancestor shapes are documented for later.

Validated under a real 2.14<->2.15 libxml2 mismatch: full suite green,
plus a 10k-iteration leak loop with no crash/leak and byte-identical
signature fixtures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: mxamin

CLAUDE.md

@@ -47,9 +47,10 @@ allocated:
    (`etree.tostring`). Input is never mutated by xmlsec.
 2. **bytes → xmlsec node**: re-parse with xmlsec's libxml2 (`xmlReadMemory`).
 3. Run the xmlsec operation on that fresh, xmlsec-owned node.
-4. **xmlsec node → bytes → lxml**: serialize the result with xmlsec's libxml2
-   (`xmlNodeDump`), re-parse with lxml (`etree.fromstring`), and graft it into
-   the original lxml tree.
+4. **xmlsec doc → bytes → lxml**: serialize the whole mutated document with
+   xmlsec's libxml2 (`xmlDocDumpMemory`), re-parse with lxml
+   (`etree.fromstring`), locate the new node, and graft it into the original
+   lxml tree.
 
 Only bytes cross between the two libxml2 worlds — never a pointer.
 
@@ -60,61 +61,108 @@ Only bytes cross between the two libxml2 worlds — never a pointer.
 
 ### Bridge helpers
 
-Two small helpers in [src/lxml.c](src/lxml.c) (declared in
-[src/lxml.h](src/lxml.h)) are the only sanctioned crossing points. They go
-through lxml's Python API so the tree is always walked by lxml's libxml2:
+Three helpers in [src/lxml.c](src/lxml.c) (declared in [src/lxml.h](src/lxml.h))
+are the only sanctioned crossing points. The first two go through lxml's Python
+API so the tree is always walked by lxml's libxml2:
 
 - `PyXmlSec_LxmlElementToBytes(element)` → `etree.tostring(element, with_tail=False)`
 - `PyXmlSec_LxmlElementFromBytes(data)` → `etree.fromstring(data)`
 
+- **`PyXmlSec_LxmlAddChildViaXmlSec(element, op, ctx, error)`** — the reusable
+  round-trip. It is the ABI-safe drop-in for the old "call an `xmlSecTmpl*Add*`
+  function on `element->_c_node`, then `elementFactory` the result" one-liner.
+  It owns the entire serialize → mutate → reflect dance (below); a converted
+  function supplies only the `op` callback (the `xmlSecTmpl*` call + its args via
+  `ctx`) and an error string. **Convert new functions by writing an `op`, not by
+  re-implementing the round-trip.**
+
 ## Reference implementation: `template.add_reference`
 
 `PyXmlSec_TemplateAddReference` in [src/template.c](src/template.c) is the first
-function converted and the **template for converting the rest**. Read it
-alongside this section. The flow:
+function converted and the **template for converting the rest**. After the
+refactor it is tiny: parse args, build a `ctx`, and call the helper. The
+xmlsec-specific part is the `op`:
+
+```c
+static xmlNodePtr PyXmlSec_TemplateAddReferenceOp(xmlNodePtr root, void* ctx) {
+    struct ... * c = ctx;
+    return xmlSecTmplSignatureAddReference(root, c->digest, XSTR(c->id), XSTR(c->uri), XSTR(c->type));
+}
+```
+
+`op` receives `root` = the copy's root element, i.e. the equivalent of the
+original `element->_c_node`, and returns the node it created (same contract as
+the old direct call). Everything else is the helper's job.
+
+### What the helper does (and the key decision)
 
-1. `PyXmlSec_LxmlElementToBytes(node)` — serialize the `<Signature>` element.
+1. `PyXmlSec_LxmlElementToBytes(element)` — serialize the subtree.
 2. `xmlReadMemory(...)` — parse into a throwaway xmlsec-owned `xmlDocPtr`.
-3. `xmlSecTmplSignatureAddReference(xmlDocGetRootElement(doc), ...)` — xmlsec
-   adds the `<Reference>` to the copy and returns it (`res`).
-4. Reflect back: dump `res`, `PyXmlSec_LxmlElementFromBytes(...)`, then
-   `find` the `SignedInfo` of the *original* node and `append` the new element.
-5. Return the grafted lxml element (a live node in the user's tree, so the
+3. `res = op(xmlDocGetRootElement(doc), ctx)` — run the caller's xmlsec
+   mutation on the copy. `res` stays attached to `doc`.
+4. Record `res`'s **element-index path** from the root (so it can be re-found
+   after a round-trip), then dump the **whole** mutated document with
+   `xmlDocDumpMemory` — *not* the lone `res` subtree.
+5. `PyXmlSec_LxmlElementFromBytes(...)` the dump, walk the recorded path to the
+   new node, walk the same path (minus the last step) in the *original*
+   `element` to reach the parent, and `append` the new node there.
+6. Return the grafted lxml node (a live node in the user's tree, so the
    incremental builder — `add_transform(ref, ...)` — keeps working).
 
-### Two non-obvious gotchas (the reason this took iterations)
-
-These will recur in every function we convert, so they're worth understanding:
-
-**(a) Namespaces are lost on a naive node dump.** `xmlNodeDump` of a subtree does
-*not* emit namespace declarations that live on ancestors. The `<Reference>`
-uses the dsig namespace declared up on `<Signature>`, so dumping it alone yields
-`<Reference>` with no `xmlns` → re-parses into the *wrong* (empty) namespace.
-Fix: **`xmlUnlinkNode(res)` first, then `xmlReconciliateNs(doc, res)`.** Order
-matters — while the node is still attached, the namespace is reachable via its
-ancestors, so reconcile thinks nothing is wrong and does nothing. Only after
-unlinking does reconcile redeclare the namespace onto the node itself.
-
-**(b) Whitespace changes the signature.** xmlsec pretty-prints by inserting
-newline text nodes between children. The newline it puts *after* the
-`<Reference>` element is a *sibling* tail node, not part of the dumped subtree,
-so the round-trip drops it.
-That single missing `\n` changes the canonicalized `SignedInfo` bytes, which
-changes the computed `SignatureValue` and breaks byte-exact signature fixtures.
-Fix: capture `res->next`'s text (the tail) *before* unlinking, and reapply it as
-the grafted element's `.tail`. Any converted function that relies on xmlsec's
-formatting must preserve these tail text nodes to stay byte-compatible.
-
-### Memory / ref-count notes
-
-- `res` is unlinked from `doc`, so it is **not** freed by `xmlFreeDoc(doc)` — it
-  must be `xmlFreeNode`'d separately.
-- The captured `tail` is `xmlStrdup`'d → `xmlFree` it (NULL-safe on the success
-  path).
-- Every `PyObject_CallMethod`/`PyUnicode_*` result is a new reference and is
-  `Py_DECREF`'d, including the `None` returned by `.append(...)`.
-- The pure-C libxml2 work runs inside `Py_BEGIN_ALLOW_THREADS`; all the lxml
-  Python-API calls run with the GIL held, outside it.
+Dumping the **entire** document in step 4 (rather than just `res`) is the load-
+bearing decision: it keeps the round-trip byte-identical to the old raw-pointer
+code *without* any manual fix-up, because two things that would otherwise
+diverge only exist in the surrounding document:
+
+**(a) Namespaces.** `xmlNodeDump` of a *subtree* does not emit namespace
+declarations that live on ancestors. `<Reference>` uses the dsig namespace
+declared up on `<Signature>`; dump it alone and you get `<Reference>` with no
+`xmlns` → it re-parses into the *wrong* (empty) namespace, and xmlsec no longer
+recognizes it. Dumping the whole document keeps the `<Signature>` declaration in
+the bytes, so lxml re-parses the reference into the correct dsig namespace.
+
+**(b) Whitespace.** xmlsec pretty-prints by inserting newline text nodes between
+children. The `\n` it puts *after* `<Reference>` is a *sibling* tail node, not
+part of the `<Reference>` subtree. Drop it and the canonicalized `<SignedInfo>`
+bytes change → the computed `SignatureValue` changes → byte-exact signature
+fixtures break. In a whole-document dump that `\n` is just bytes between two
+elements; lxml re-parses it as the new element's `.tail`, and `append` carries
+the tail along.
+
+> An earlier (pre-helper) version dumped only the `res` subtree and hit both as
+> bugs, patching them by hand (`xmlUnlinkNode` + `xmlReconciliateNs` for the
+> namespace; `xmlStrdup`-ing `res->next`'s text and reapplying it as `.tail` for
+> the whitespace). The whole-document dump deletes all of that. The cost is that
+> the new node must be *located* after the round-trip instead of handed back as
+> `res`; the helper does this generically via the element-index path.
+
+### Helper assumptions & when *not* to use it
+
+The path-based reflect assumes `op` **appends exactly one new node beneath a
+parent that already exists in `element`**, and that the templates contain no
+comment/PI siblings (so a libxml2 element index equals an lxml child index).
+This holds for the `xmlSecTmpl*Add*` family. Two shapes need more than the
+current helper:
+
+- **find-or-create** (`xmlSecTmpl*Ensure*`, e.g. `ensure_key_info`): `op` may
+  return an *existing* node without adding anything → appending it would
+  duplicate. Needs a "did the tree actually grow?" check before grafting.
+- **intermediate ancestors** (e.g. `add_transform` may create a `<Transforms>`
+  wrapper *and* the `<Transform>`): the topmost new node, not `res`, is what
+  must be grafted.
+
+Extend the helper (or branch inside it) when you reach those; don't force-fit.
+
+### Memory / ref-count notes (inside the helper)
+
+- `res` stays attached to `doc`, so `xmlFreeDoc(doc)` reclaims it — it is **not**
+  freed separately.
+- `xmlDocDumpMemory`'s output buffer is `xmlFree`'d (NULL-safe on failure).
+- Every `PySequence_GetItem` / `PyObject_CallMethod` result is a new reference
+  and is `Py_DECREF`'d, including the `None` returned by `.append(...)` and the
+  reparsed whole-document tree (dropped once the new node is grafted out of it).
+- `op` runs inside `Py_BEGIN_ALLOW_THREADS` (pure libxml2/xmlsec, no Python);
+  all the lxml Python-API calls run with the GIL held, outside it.
 
 ## Version guard / opt-in escape hatch
 
@@ -176,14 +224,18 @@ note `ru_maxrss` is **bytes** on macOS, kB on Linux).
 ## Status & rollout
 
 - ✅ `template.add_reference` — converted and validated (full suite green +
-  10k-iteration loop, no crash/leak, under a real 2.14↔2.15 mismatch).
+  10k-iteration loop, no crash/leak, under a real 2.14↔2.15 mismatch). The
+  reusable round-trip lives in `PyXmlSec_LxmlAddChildViaXmlSec`.
 - ⬜ Everything else still passes raw lxml nodes to xmlsec and is unsafe on a
   mismatch: the rest of `src/template.c`, `src/ds.c` (sign/verify),
   `src/enc.c` (encrypt/decrypt), `src/tree.c`.
 
-When converting another function, reuse the `add_reference` pattern and watch
-for the same two gotchas (namespace reconciliation after unlink; preserving
-xmlsec's formatting tail nodes). Each function differs in *where* the result
-grafts back (e.g. `add_reference` → `SignedInfo`; `ensure_key_info` →
-`Signature`). The default version guard can only be relaxed once **all**
-node-passing paths are converted.
+When converting another function, **write an `op` and call
+`PyXmlSec_LxmlAddChildViaXmlSec`** rather than re-implementing the round-trip;
+the helper handles serialization, the whole-document dump, and grafting (which
+sidesteps the namespace/whitespace divergences for free). For the simple
+`xmlSecTmpl*Add*` family that is the whole change. Watch for the two shapes the
+current helper does not yet cover — find-or-create (`ensure_key_info`) and
+intermediate-ancestor creation (`add_transform`) — and extend the helper when
+you reach them (see "Helper assumptions & when *not* to use it"). The default
+version guard can only be relaxed once **all** node-passing paths are converted.

src/lxml.c

@@ -137,6 +137,12 @@ int PyXmlSec_LxmlElementConverter(PyObject* o, PyXmlSec_LxmlElementPtr* p) {
 }
 
 PyObject* PyXmlSec_LxmlElementToBytes(PyObject* element) {
+    // Calls lxml.etree.tostring(element, with_tail=False).
+    // Input:  `element` - an lxml _Element (borrowed reference).
+    // Output: a new reference to a Python bytes object with the element's
+    //         serialized XML, or NULL with an exception set on failure.
+    // Example: an lxml element <Reference URI="#foo"/> returns the bytes
+    //          b'<Reference URI="#foo"/>'.
     // Done entirely through lxml's Python API so that the tree is walked by the
     // same libxml2 that allocated it. `with_tail=False` keeps the output to the
     // element itself, which xmlsec's libxml2 then re-parses from bytes.
@@ -171,3 +177,150 @@ PyObject* PyXmlSec_LxmlElementFromBytes(PyObject* data) {
     Py_DECREF(etree);
     return result;
 }
+
+// dsig templates never nest anywhere near this deep; the bound just keeps the
+// path buffer on the stack and guards against a pathological tree.
+#define PYXMLSEC_MAX_TEMPLATE_DEPTH 64
+
+PyObject* PyXmlSec_LxmlAddChildViaXmlSec(
+    PyXmlSec_LxmlElementPtr element, PyXmlSec_LxmlXmlSecOp op, void* ctx, const char* error) {
+
+    PyObject* serialized = NULL;
+    PyObject* result_bytes = NULL;
+    PyObject* new_tree = NULL;
+    PyObject* new_node = NULL;
+    PyObject* parent = NULL;
+    PyObject* appended = NULL;
+
+    xmlDocPtr doc = NULL;
+    xmlNodePtr res = NULL;
+    xmlChar* dump = NULL;
+    int dump_size = 0;
+
+    char* xml_data = NULL;
+    Py_ssize_t xml_size = 0;
+
+    // Element-only child indices from the copy's root down to the node `op`
+    // produced, stored root-first; `depth` is its length (>= 1 on success).
+    int path[PYXMLSEC_MAX_TEMPLATE_DEPTH];
+    int depth = 0;
+    int overflow = 0;
+    int i;
+
+    serialized = PyXmlSec_LxmlElementToBytes((PyObject*)element);
+    if (serialized == NULL || PyBytes_AsStringAndSize(serialized, &xml_data, &xml_size) < 0) {
+        goto ON_FAIL;
+    }
+
+    Py_BEGIN_ALLOW_THREADS;
+    doc = xmlReadMemory(xml_data, (int)xml_size, NULL, NULL, XML_PARSE_NONET);
+    if (doc != NULL) {
+        res = op(xmlDocGetRootElement(doc), ctx);
+        if (res != NULL) {
+            // Record res's element-index path up to the root: first measure the
+            // depth, then walk again filling `path` from the back so it ends up
+            // ordered root-first.
+            xmlNodePtr cur = res;
+            int d = 0;
+            while (cur->parent != NULL && cur->parent->type == XML_ELEMENT_NODE) {
+                ++d;
+                cur = cur->parent;
+            }
+            if (d < 1 || d > PYXMLSEC_MAX_TEMPLATE_DEPTH) {
+                overflow = 1;
+            } else {
+                depth = d;
+                for (cur = res; d > 0; cur = cur->parent) {
+                    int idx = 0;
+                    xmlNodePtr s;
+                    for (s = cur->prev; s != NULL; s = s->prev) {
+                        if (s->type == XML_ELEMENT_NODE) { ++idx; }
+                    }
+                    path[--d] = idx;
+                }
+                // Dump the whole mutated document, not just res: the surrounding
+                // context carries the new node's ancestor-declared namespace and
+                // the formatting whitespace xmlsec appends after it, so lxml
+                // reconstructs both on re-parse — no manual reconcile/tail fix-up.
+                xmlDocDumpMemory(doc, &dump, &dump_size);
+            }
+        }
+    }
+    Py_END_ALLOW_THREADS;
+
+    if (doc == NULL) {
+        PyErr_SetString(PyXmlSec_InternalError, "cannot parse serialized template.");
+        goto ON_FAIL;
+    }
+    if (res == NULL) {
+        PyXmlSec_SetLastError(error);
+        goto ON_FAIL;
+    }
+    if (overflow) {
+        PyErr_SetString(PyXmlSec_InternalError, "unexpected template structure.");
+        goto ON_FAIL;
+    }
+    if (dump == NULL || dump_size <= 0) {
+        PyErr_SetString(PyXmlSec_InternalError, "cannot serialize result.");
+        goto ON_FAIL;
+    }
+
+    // Re-parse the whole result with lxml so the nodes are lxml-owned.
+    result_bytes = PyBytes_FromStringAndSize((const char*)dump, (Py_ssize_t)dump_size);
+    if (result_bytes == NULL) {
+        goto ON_FAIL;
+    }
+    new_tree = PyXmlSec_LxmlElementFromBytes(result_bytes);
+    if (new_tree == NULL) {
+        goto ON_FAIL;
+    }
+
+    // Locate the produced node in the re-parsed tree by walking the recorded path.
+    new_node = new_tree;
+    Py_INCREF(new_node);
+    for (i = 0; i < depth; ++i) {
+        PyObject* child = PySequence_GetItem(new_node, path[i]);
+        Py_DECREF(new_node);
+        new_node = child;
+        if (new_node == NULL) {
+            goto ON_FAIL;
+        }
+    }
+
+    // Walk the same path (minus the final step) in the *original* tree to reach
+    // the parent the new node belongs under, then graft it onto the live tree.
+    parent = (PyObject*)element;
+    Py_INCREF(parent);
+    for (i = 0; i < depth - 1; ++i) {
+        PyObject* child = PySequence_GetItem(parent, path[i]);
+        Py_DECREF(parent);
+        parent = child;
+        if (parent == NULL) {
+            goto ON_FAIL;
+        }
+    }
+    appended = PyObject_CallMethod(parent, "append", "O", new_node);
+    if (appended == NULL) {
+        goto ON_FAIL;
+    }
+
+    xmlFreeDoc(doc);
+    xmlFree(dump);
+    Py_DECREF(serialized);
+    Py_DECREF(result_bytes);
+    Py_DECREF(new_tree);
+    Py_DECREF(parent);
+    Py_DECREF(appended);
+    return new_node;
+
+ON_FAIL:
+    if (doc != NULL) { xmlFreeDoc(doc); }
+    if (dump != NULL) { xmlFree(dump); }
+    Py_XDECREF(serialized);
+    Py_XDECREF(result_bytes);
+    Py_XDECREF(new_tree);
+    Py_XDECREF(new_node);
+    Py_XDECREF(parent);
+    Py_XDECREF(appended);
+    return NULL;
+}

src/lxml.h

@@ -40,6 +40,28 @@ PyObject* PyXmlSec_LxmlElementToBytes(PyObject* element);
 // (with an exception set) on failure.
 PyObject* PyXmlSec_LxmlElementFromBytes(PyObject* data);
 
+// Performs the actual xmlsec mutation on a private, xmlsec-owned copy of an
+// element. `root` is the copy's root element (the equivalent of the original
+// `element->_c_node`); `ctx` carries the call's extra arguments. Must return the
+// node it created, or NULL on failure (with the xmlsec error already recorded).
+typedef xmlNodePtr (*PyXmlSec_LxmlXmlSecOp)(xmlNodePtr root, void* ctx);
+
+// ABI-safe replacement for the "call an xmlSecTmpl*Add* function on `element`'s
+// raw node, then wrap the returned node with elementFactory" pattern (see
+// https://github.com/xmlsec/python-xmlsec/issues/356). Serializes `element`,
+// runs `op` on a throwaway xmlsec-owned copy, then reflects the node `op`
+// produced back into `element`'s live lxml tree and returns it as a new lxml
+// _Element. Only bytes cross the lxml/xmlsec boundary, never raw pointers.
+// Returns a new reference, or NULL with an exception set; `error` is the message
+// raised when `op` returns NULL.
+//
+// Assumes `op` appends exactly one new node beneath a parent that already exists
+// in `element` (true for the xmlSecTmpl*Add* family). Find-or-create ops
+// (xmlSecTmpl*Ensure*) and ops that also create intermediate ancestors need
+// extra handling — see CLAUDE.md.
+PyObject* PyXmlSec_LxmlAddChildViaXmlSec(
+    PyXmlSec_LxmlElementPtr element, PyXmlSec_LxmlXmlSecOp op, void* ctx, const char* error);
+
 // get version numbers for libxml2 both compiled and loaded
 long PyXmlSec_GetLibXmlVersionMajor();
 long PyXmlSec_GetLibXmlVersionMinor();