Skip to content

Split TreeContext into TreeConfig and OperationState#424

Draft
steiler wants to merge 1 commit intomainfrom
improveTreeContext
Draft

Split TreeContext into TreeConfig and OperationState#424
steiler wants to merge 1 commit intomainfrom
improveTreeContext

Conversation

@steiler
Copy link
Copy Markdown
Collaborator

@steiler steiler commented May 4, 2026

TreeContext currently holds four fields with fundamentally different lifecycles:

  • Setup fields (SchemaClient, PoolFactory): fixed at tree creation, immutable, shared across all entries and operations
  • Operation fields (ExplicitDeletes, NonRevertiveInfos): reset and accumulated during each operation (import pass, transaction, validation), copied when branching

To callers, all four fields look identical. There is no semantic distinction between "this never changes" and "this accumulates per-operation." This creates three problems:

  1. Unclear semantics: Code reading e.GetTreeContext().NonRevertiveInfo() cannot immediately tell whether this reads global config or operation-specific state.
  2. Silent mutations: Processors and operations mutate ExplicitDeletes and NonRevertiveInfos as side effects of traversal (e.g., ImportConfigProcessor.Run calls e.GetTreeContext().ExplicitDeletes().Add(...)). A reader must trace deep into multiple call stacks to understand what state is being mutated.
  3. Accidental sharing: If a future refactor accidentally fails to deep-copy mutable state during tree branching (DeepCopy), the bug is silent — two branches silently share mutations.

Solution

Split TreeContext into two separate concerns:

  1. TreeConfig interface: immutable, created once per tree root, shared by all entries. Contains SchemaClient and PoolFactory.
  2. OperationState interface: mutable, created once per tree root but deep-copied when the tree is branched. Contains ExplicitDeletes and NonRevertiveInfos.

The TreeContext interface is simplified to a lifecycle marker (DeepCopy plus the two getters), and all flat data-access methods are removed. A big-bang migration updates all call sites at once. DeepCopy reuses the same TreeConfig instance and deep-copies OperationState.

Benefits:

  • Locality: Reading GetTreeConfig() tells you "this is fixed;" reading GetOperationState() tells you "this accumulates."
  • Testability: OperationState can be tested in isolation from schema and pool setup.
  • Safety: The distinction is enforced at the interface level, reducing the risk of accidental mutation sharing.
  • Future leverage: Sets the stage for Phase 2, where OperationState can be threaded explicitly through processors and transaction flows.

`TreeContext` currently holds four fields with fundamentally different lifecycles:
- **Setup fields** (SchemaClient, PoolFactory): fixed at tree creation, immutable, shared across all entries and operations
- **Operation fields** (ExplicitDeletes, NonRevertiveInfos): reset and accumulated during each operation (import pass, transaction, validation), copied when branching

To callers, all four fields look identical. There is no semantic distinction between "this never changes" and "this accumulates per-operation." This creates three problems:

1. **Unclear semantics**: Code reading `e.GetTreeContext().NonRevertiveInfo()` cannot immediately tell whether this reads global config or operation-specific state.
2. **Silent mutations**: Processors and operations mutate `ExplicitDeletes` and `NonRevertiveInfos` as side effects of traversal (e.g., `ImportConfigProcessor.Run` calls `e.GetTreeContext().ExplicitDeletes().Add(...)`). A reader must trace deep into multiple call stacks to understand what state is being mutated.
3. **Accidental sharing**: If a future refactor accidentally fails to deep-copy mutable state during tree branching (DeepCopy), the bug is silent — two branches silently share mutations.

## Solution

Split `TreeContext` into two separate concerns:

1. **TreeConfig interface**: immutable, created once per tree root, shared by all entries. Contains SchemaClient and PoolFactory.
2. **OperationState interface**: mutable, created once per tree root but deep-copied when the tree is branched. Contains ExplicitDeletes and NonRevertiveInfos.

The `TreeContext` interface is simplified to a lifecycle marker (DeepCopy plus the two getters), and all flat data-access methods are removed. A big-bang migration updates all call sites at once. DeepCopy reuses the same TreeConfig instance and deep-copies OperationState.

Benefits:
- **Locality**: Reading `GetTreeConfig()` tells you "this is fixed;" reading `GetOperationState()` tells you "this accumulates."
- **Testability**: OperationState can be tested in isolation from schema and pool setup.
- **Safety**: The distinction is enforced at the interface level, reducing the risk of accidental mutation sharing.
- **Future leverage**: Sets the stage for Phase 2, where OperationState can be threaded explicitly through processors and transaction flows.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 82.45614% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/tree/api/operation_state.go 83.33% 2 Missing ⚠️
pkg/tree/ops/treeexport.go 0.00% 2 Missing ⚠️
pkg/tree/root_entry.go 33.33% 2 Missing ⚠️
pkg/datastore/deviations.go 0.00% 1 Missing ⚠️
pkg/datastore/target/gnmi/stream.go 0.00% 1 Missing ⚠️
pkg/tree/api/leaf_entry.go 0.00% 1 Missing ⚠️
pkg/tree/tree_context.go 95.45% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant