ansys / ansys/pydynamicreporting

Add atomic reparenting for serverless templates

Open
#597 0 comments 0 reactions 1 assignee Claimed by @viseshrp View on GitHub
enhancement serverless
Dominant language
Python
Stars
12
Forks
5
Avg merge
3d 14h
Merged PRs (30d)
9

Description

### 📝 Description of the feature

Add a public atomic API for reparenting an existing serverless template while preserving tree integrity and child ordering.

Serverless templates expose mutable `parent` and `children` attributes, and `Template.save()` derives `master` plus `children_order`. `ADR.create_template()` also keeps both sides synchronized when a template is first created:

```python
template = template_type.create(**kwargs)
parent = kwargs.get("parent")
if parent is not None:
parent.children.append(template)
parent.save()
```

After creation, however, there is no equivalent public move operation. Assigning a new `template.parent` and calling `save()` updates only part of the tree contract unless callers also correctly:

- remove the template from the old parent's ordered children;
- save the old parent's `children_order`;
- insert it into the new parent's ordered children;
- save the new parent's `children_order`;
- derive the target's root/master state;
- prevent self-parenting and descendant cycles;
- keep all database changes atomic;
- handle concurrent edits to either parent.

`Template.reorder_child()` only changes position within one parent's in-memory children list. It does not move a node between parents or promote/demote a root.

This orchestration belongs in PyDynamicReporting because it owns the persisted parent foreign key, `master`, and `children_order` invariants. Every downstream adapter should not implement its own tree mutation transaction.

### 💡 Steps for implementing the feature

#### 1. Add one public move operation

Place the orchestration on `ADR`, alongside `create_template()`:

```python
def reparent_template(
self,
template: Template,
new_parent: Template | None,
*,
position: int | None = None,
) -> Template:
"""Move a saved template and its subtree to a new parent atomically."""
```

Contract:

- `new_parent=None` promotes the template to a root report;
- a non-`None` parent demotes or moves it below that saved template;
- `position=None` appends to the new parent's children;
- an integer position inserts at that zero-based index;
- moving within the same parent can reuse reorder semantics;
- the template GUID and its complete descendant subtree are preserved;
- the returned object is refreshed from persisted state.

Use `Template` objects consistently with the existing serverless API. GUID resolution can remain a responsibility of higher-level adapters.

#### 2. Validate the complete operation before mutation

Reject:

- unsaved target or parent objects;
- target and parent from different database aliases;
- `template is new_parent`;
- a parent contained anywhere in the target's descendant subtree;
- invalid positions (`position < 0` or `position > len(new_parent.children)` after removing the target for same-parent moves);
- a non-`None` position when promoting to root, unless root ordering is explicitly supported;
- type-specific parent/child combinations that existing template validation disallows.

Use a product-specific exception such as `TemplateReparentError` with target and parent GUIDs in the detail.

Cycle detection can walk parent links from the proposed parent to the root, avoiding a full subtree load:

```python
ancestor = new_parent
while ancestor is not None:
if ancestor.guid == template.guid:
raise TemplateReparentError("A template cannot be moved below its descendant")
ancestor = ancestor.parent
```

Also guard against pre-existing corrupt cycles by tracking visited GUIDs.

#### 3. Apply all persistence changes in one transaction

Lock the target, old parent, and new parent rows before calculating order. The implementation outline is:

```python
with transaction.atomic(using=database_alias):
target = get_locked_template(template.guid)
old_parent = get_locked_parent(target.parent)
destination = get_locked_parent(new_parent)

validate_reparent(target, destination, position)

if old_parent is not None:
old_parent.children = [
child for child in old_parent.children if child.guid != target.guid
]
old_parent.save(using=database_alias)

target.parent = destination
target.save(using=database_alias)

if destination is not None:
destination.children = [
child for child in destination.children if child.guid != target.guid
]
insertion_index = len(destination.children) if position is None else position
destination.children.insert(insertion_index, target)
destination.save(using=database_alias)
```

Use fresh persisted child collections while holding row locks; do not trust potentially stale in-memory `children` lists passed by the caller.

If any save or validation step fails, roll back the target parent, both parent order strings, and root/master state together.

#### 4. Define same-parent and no-op behavior

- Same parent plus a new position should reorder atomically and return the refreshed target.
- Same parent plus `position=None` should be an idempotent no-op.
- Moving a root to `None` should be an idempotent no-op.
- Duplicate child entries must never be introduced.
- Promoting a nested template to root must remove its GUID from the old parent's order.

Document whether the moved node becomes a report immediately when promoted and confirm that `ADR.get_report(guid=...)` can retrieve it.

#### 5. Add focused tests

Cover:

1. moving a leaf between two parents;
2. moving a subtree and preserving every descendant relation;
3. promoting a child to a root and demoting a root below another report;
4. insertion at the beginning, middle, end, and append default;
5. same-parent reorder and no-op behavior;
6. self-parenting and direct/indirect cycle rejection;
7. unsaved objects and cross-database rejection;
8. injected failure after the old-parent update, proving full rollback;
9. concurrent moves or stale callers, proving row-lock behavior;
10. re-fetching all affected templates and verifying `parent`, `master`, `children`, and `children_order` agree.

#### Acceptance criteria

- Reparenting is available through one documented public PyDynamicReporting call.
- Callers never need to edit both parents and the child manually.
- Cycles, duplicate child entries, stale order strings, and partial updates are prevented.
- Promotion and demotion correctly update root/master behavior.
- The operation preserves the moved subtree and GUIDs.
- Any failure rolls back every affected database change.
- Tests verify persisted state after re-fetch, not only in-memory lists.

### 🔗 Useful links and references

- [Template parent, children, and ordering fields](https://github.com/ansys/pydynamicreporting/blob/d98b45adad663d015d4e17ad93dcd3749f2f3d81/src/ansys/dynamicreporting/core/serverless/template.py#L93-L143)
- [`Template.save()` hierarchy persistence](https://github.com/ansys/pydynamicreporting/blob/d98b45adad663d015d4e17ad93dcd3749f2f3d81/src/ansys/dynamicreporting/core/serverless/template.py#L221-L259)
- [Current child-only reorder helper](https://github.com/ansys/pydynamicreporting/blob/d98b45adad663d015d4e17ad93dcd3749f2f3d81/src/ansys/dynamicreporting/core/serverless/template.py#L537-L567)
- [Creation-time parent synchronization](https://github.com/ansys/pydynamicreporting/blob/d98b45adad663d015d4e17ad93dcd3749f2f3d81/src/ansys/dynamicreporting/core/serverless/adr.py#L1002-L1041)
- Related but non-duplicate issue: #351 broadly mentions missing serverless template attributes/methods but does not identify reparenting, transactions, cycle prevention, or ordering semantics.

An open-and-closed issue audit found no existing issue defining an atomic template reparenting operation.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.