From 43cb6e17dc676c9f4b194ea05302b1ac7f601942 Mon Sep 17 00:00:00 2001 From: steiler Date: Mon, 4 May 2026 16:42:46 +0200 Subject: [PATCH] Split TreeContext into TreeConfig and OperationState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- pkg/datastore/deviations.go | 2 +- pkg/datastore/target/gnmi/stream.go | 2 +- pkg/datastore/transaction_rpc.go | 8 +- pkg/tree/api/leaf_entry.go | 2 +- pkg/tree/api/leaf_variants.go | 12 +- pkg/tree/api/leaf_variants_test.go | 50 +++++++ pkg/tree/api/operation_state.go | 39 +++++ pkg/tree/api/tree_config.go | 13 ++ pkg/tree/api/tree_context.go | 11 +- pkg/tree/entry_test.go | 24 +-- pkg/tree/issue05_regression_test.go | 111 ++++++++++++++ pkg/tree/ops/treeexport.go | 4 +- .../processor_explicit_delete_test.go | 2 +- pkg/tree/processors/processor_importer.go | 4 +- pkg/tree/root_entry.go | 6 +- pkg/tree/root_entry_test.go | 20 +-- pkg/tree/sharedEntryAttributes.go | 8 +- pkg/tree/sharedEntryAttributes_test.go | 2 +- pkg/tree/tree_context.go | 62 ++++---- pkg/tree/tree_context_test.go | 137 ++++++++++++++++++ 20 files changed, 433 insertions(+), 86 deletions(-) create mode 100644 pkg/tree/api/operation_state.go create mode 100644 pkg/tree/api/tree_config.go create mode 100644 pkg/tree/issue05_regression_test.go create mode 100644 pkg/tree/tree_context_test.go diff --git a/pkg/datastore/deviations.go b/pkg/datastore/deviations.go index 964817ec..8d1ec4c2 100644 --- a/pkg/datastore/deviations.go +++ b/pkg/datastore/deviations.go @@ -201,7 +201,7 @@ func (d *Datastore) calculateDeviations(ctx context.Context) (<-chan *treetypes. if log := log.V(logger.VTrace); log.Enabled() { log.Info("deviation tree", "content", deviationTree.String()) - log.Info("nonrevertive infos", "data", deviationTree.GetTreeContext().NonRevertiveInfo().String()) + log.Info("nonrevertive infos", "data", deviationTree.GetTreeContext().GetOperationState().NonRevertiveInfo().String()) } deviationChan := make(chan *treetypes.DeviationEntry, 10) diff --git a/pkg/datastore/target/gnmi/stream.go b/pkg/datastore/target/gnmi/stream.go index b8816924..0ee993e5 100644 --- a/pkg/datastore/target/gnmi/stream.go +++ b/pkg/datastore/target/gnmi/stream.go @@ -157,7 +157,7 @@ func (s *StreamSync) buildTreeSyncWithDatastore(cUS <-chan *NotificationData, sy if err != nil { log.Error(err, "failed adding update to synctree") } - syncTree.GetTreeContext().ExplicitDeletes().Add(consts.RunningIntentName, consts.RunningValuesPrio, noti.deletes) + syncTree.GetTreeContext().GetOperationState().ExplicitDeletes().Add(consts.RunningIntentName, consts.RunningValuesPrio, noti.deletes) case <-syncResponse: syncTree, err = s.syncToRunning(syncTree, syncTreeMutex, true) tickerActive = true diff --git a/pkg/datastore/transaction_rpc.go b/pkg/datastore/transaction_rpc.go index d89d55b1..34d736f2 100644 --- a/pkg/datastore/transaction_rpc.go +++ b/pkg/datastore/transaction_rpc.go @@ -241,7 +241,7 @@ func (d *Datastore) lowlevelTransactionSet(ctx context.Context, transaction *typ } // clear the owners existing explicit delete entries, retrieving the old entries for storing in the transaction for possible rollback - oldExplicitDeletes := root.GetTreeContext().ExplicitDeletes().Remove(intent.GetName()) + oldExplicitDeletes := root.GetTreeContext().GetOperationState().ExplicitDeletes().Remove(intent.GetName()) priority := int32(math.MaxInt32) if len(oldIntentContent) > 0 { @@ -268,14 +268,14 @@ func (d *Datastore) lowlevelTransactionSet(ctx context.Context, transaction *typ } // add the explicit delete entries - treeContext.ExplicitDeletes().Add(intent.GetName(), intent.GetPriority(), intent.GetDeletes()) + treeContext.GetOperationState().ExplicitDeletes().Add(intent.GetName(), intent.GetPriority(), intent.GetDeletes()) } // add non-revertive info to tree context - treeContext.NonRevertiveInfo().Add(intent.GetName(), intent.NonRevertive(), intent.GetRevertPaths()...) + treeContext.GetOperationState().NonRevertiveInfo().Add(intent.GetName(), intent.NonRevertive(), intent.GetRevertPaths()...) } - log.V(logger.VDebug).Info("nonrevertive infos", "data", treeContext.NonRevertiveInfo().String()) + log.V(logger.VDebug).Info("nonrevertive infos", "data", treeContext.GetOperationState().NonRevertiveInfo().String()) les := ops.LeafsOfOwner(root.Entry, consts.RunningIntentName) diff --git a/pkg/tree/api/leaf_entry.go b/pkg/tree/api/leaf_entry.go index 5c8d4a9f..78b3583d 100644 --- a/pkg/tree/api/leaf_entry.go +++ b/pkg/tree/api/leaf_entry.go @@ -139,7 +139,7 @@ func (l *LeafEntry) NonRevertive() bool { if l.parentEntry == nil { return false } - return l.parentEntry.GetTreeContext().NonRevertiveInfo().IsNonRevertive(l.Owner(), l) + return l.parentEntry.GetTreeContext().GetOperationState().NonRevertiveInfo().IsNonRevertive(l.Owner(), l) } // String returns a string representation of the LeafEntry diff --git a/pkg/tree/api/leaf_variants.go b/pkg/tree/api/leaf_variants.go index 0a3d84a5..1c6d6765 100644 --- a/pkg/tree/api/leaf_variants.go +++ b/pkg/tree/api/leaf_variants.go @@ -14,14 +14,14 @@ import ( type LeafVariants struct { les LeafVariantSlice lesMutex sync.RWMutex - tc TreeContext + os OperationState parentEntry Entry } -func NewLeafVariants(tc TreeContext, parentEnty Entry) *LeafVariants { +func NewLeafVariants(os OperationState, parentEnty Entry) *LeafVariants { return &LeafVariants{ les: make(LeafVariantSlice, 0, 2), - tc: tc, + os: os, parentEntry: parentEnty, } } @@ -259,10 +259,10 @@ func (lv *LeafVariants) GetHighestPrecedenceValue(filter HighestPrecedenceFilter return result } -func (lv *LeafVariants) DeepCopy(tc TreeContext, parent Entry) *LeafVariants { +func (lv *LeafVariants) DeepCopy(os OperationState, parent Entry) *LeafVariants { result := &LeafVariants{ lesMutex: sync.RWMutex{}, - tc: tc, + os: os, les: make([]*LeafEntry, 0, len(lv.les)), parentEntry: parent, } @@ -382,7 +382,7 @@ func (lv *LeafVariants) highestIsUnequalRunning(highest *LeafEntry) bool { } // if highest is not new or updated and highest is non-revertive - if !highest.IsNew && !highest.IsUpdated && lv.tc.NonRevertiveInfo().IsNonRevertive(highest.Update.Owner(), lv.parentEntry) { + if !highest.IsNew && !highest.IsUpdated && lv.os.NonRevertiveInfo().IsNonRevertive(highest.Update.Owner(), lv.parentEntry) { return false } diff --git a/pkg/tree/api/leaf_variants_test.go b/pkg/tree/api/leaf_variants_test.go index 4e8b865d..3926faa6 100644 --- a/pkg/tree/api/leaf_variants_test.go +++ b/pkg/tree/api/leaf_variants_test.go @@ -474,3 +474,53 @@ func TestLeafVariants_canDelete(t *testing.T) { }) } } + +// TestNewLeafVariants_AcceptsOperationState verifies that NewLeafVariants accepts +// an OperationState (not a full TreeContext) and returns a non-nil LeafVariants. +func TestNewLeafVariants_AcceptsOperationState(t *testing.T) { + os := NewOperationState() + lv := NewLeafVariants(os, nil) + if lv == nil { + t.Fatal("expected NewLeafVariants to return non-nil LeafVariants") + } +} + +// TestLeafVariants_NonRevertiveBehaviorPreservedAfterDeepCopy verifies that +// when an owner is marked non-revertive in OperationState, GetHighestPrecedence +// does not report the entry as changed — and that DeepCopy preserves this +// behaviour with the copied state. +func TestLeafVariants_NonRevertiveBehaviorPreservedAfterDeepCopy(t *testing.T) { + os := NewOperationState() + os.NonRevertiveInfo().Add("owner1", true) + + // parentEntry nil is safe: NonRevertiveInfo.IsNonRevertive does not + // dereference the path when no revert-paths are configured. + lv := NewLeafVariants(os, nil) + + // running entry: lower priority (higher number), value "v1" + leRun := NewLeafEntry( + types.NewUpdate(&mockUpdateParent{}, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "v1"}}, RunningValuesPrio, RunningIntentName, 0), + types.NewUpdateInsertFlags(), + nil, + ) + // owner1 entry: higher priority (lower number), different value "v2", not new/updated + leOwner := NewLeafEntry( + types.NewUpdate(&mockUpdateParent{}, &sdcpb.TypedValue{Value: &sdcpb.TypedValue_StringVal{StringVal: "v2"}}, 10, "owner1", 0), + types.NewUpdateInsertFlags(), + nil, + ) + lv.Add(leRun) + lv.Add(leOwner) + + // Non-revertive: even though owner1 has a different value, it is not "new or updated" + // so GetHighestPrecedence(onlyNewOrUpdated=true) should return nil. + if got := lv.GetHighestPrecedence(true, false, false); got != nil { + t.Errorf("expected nil from GetHighestPrecedence with non-revertive owner, got entry owned by %q", got.Owner()) + } + + // DeepCopy must carry the copied OperationState so the same behaviour holds. + lvCopy := lv.DeepCopy(os.DeepCopyState(), nil) + if got := lvCopy.GetHighestPrecedence(true, false, false); got != nil { + t.Errorf("after DeepCopy: expected nil from GetHighestPrecedence with non-revertive owner, got entry owned by %q", got.Owner()) + } +} diff --git a/pkg/tree/api/operation_state.go b/pkg/tree/api/operation_state.go new file mode 100644 index 00000000..c823f8e6 --- /dev/null +++ b/pkg/tree/api/operation_state.go @@ -0,0 +1,39 @@ +package api + +// OperationState holds mutable state accumulated during a tree operation +// (e.g. an import pass or transaction). It is created once per tree root and +// deep-copied when the tree is branched. +type OperationState interface { + ExplicitDeletes() *DeletePathSet + NonRevertiveInfo() NonRevertiveInfos + DeepCopyState() OperationState +} + +// operationState is the concrete, package-private implementation. +type operationState struct { + explicitDeletes *DeletePathSet + nonRevertiveInfo NonRevertiveInfos +} + +// NewOperationState creates a fresh OperationState with empty collections. +func NewOperationState() OperationState { + return &operationState{ + explicitDeletes: NewDeletePaths(), + nonRevertiveInfo: NewNonRevertiveInfos(), + } +} + +func (o *operationState) ExplicitDeletes() *DeletePathSet { + return o.explicitDeletes +} + +func (o *operationState) NonRevertiveInfo() NonRevertiveInfos { + return o.nonRevertiveInfo +} + +func (o *operationState) DeepCopyState() OperationState { + return &operationState{ + explicitDeletes: o.explicitDeletes.DeepCopy(), + nonRevertiveInfo: o.nonRevertiveInfo.DeepCopy(), + } +} diff --git a/pkg/tree/api/tree_config.go b/pkg/tree/api/tree_config.go new file mode 100644 index 00000000..a549a3b8 --- /dev/null +++ b/pkg/tree/api/tree_config.go @@ -0,0 +1,13 @@ +package api + +import ( + schemaClient "github.com/sdcio/data-server/pkg/datastore/clients/schema" + "github.com/sdcio/data-server/pkg/pool" +) + +// TreeConfig holds immutable tree-setup values. It is created once per tree +// root and shared (by identity) across all DeepCopy calls. +type TreeConfig interface { + SchemaClient() schemaClient.SchemaClientBound + PoolFactory() pool.VirtualPoolFactory +} diff --git a/pkg/tree/api/tree_context.go b/pkg/tree/api/tree_context.go index d14b59fc..3752eb53 100644 --- a/pkg/tree/api/tree_context.go +++ b/pkg/tree/api/tree_context.go @@ -1,14 +1,7 @@ package api -import ( - schemaClient "github.com/sdcio/data-server/pkg/datastore/clients/schema" - "github.com/sdcio/data-server/pkg/pool" -) - type TreeContext interface { - PoolFactory() pool.VirtualPoolFactory - SchemaClient() schemaClient.SchemaClientBound + GetTreeConfig() TreeConfig + GetOperationState() OperationState DeepCopy() TreeContext - ExplicitDeletes() *DeletePathSet - NonRevertiveInfo() NonRevertiveInfos } diff --git a/pkg/tree/entry_test.go b/pkg/tree/entry_test.go index 2bef7ab3..c7881c54 100644 --- a/pkg/tree/entry_test.go +++ b/pkg/tree/entry_test.go @@ -1294,7 +1294,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // because thats an update. t.Run("Delete Non New", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) le1 := api.NewLeafEntry(types.NewUpdate(nil, nil, 2, owner1, ts), testhelper.FlagsExisting, nil) lv.Add(le1) @@ -1314,7 +1314,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // should return nil t.Run("Single entry thats also marked for deletion", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) lv.Add(api.NewLeafEntry(types.NewUpdate(nil, nil, 1, owner1, ts), testhelper.FlagsDelete, nil)) le := lv.GetHighestPrecedence(true, false, false) @@ -1328,7 +1328,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // on onlyIfPrioChanged == true we do not expect output. t.Run("New Low Prio IsUpdate OnlyChanged True", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) lv.Add(api.NewLeafEntry(types.NewUpdate(nil, nil, 5, owner1, ts), testhelper.FlagsExisting, nil)) lv.Add(api.NewLeafEntry(types.NewUpdate(nil, nil, 6, owner2, ts), testhelper.FlagsNew, nil)) lv.Add(api.NewLeafEntry(types.NewUpdate(nil, nil, RunningValuesPrio, RunningIntentName, ts), testhelper.FlagsExisting, nil)) @@ -1345,7 +1345,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // on onlyIfPrioChanged == false we do not expect the highes prio update to be returned. t.Run("New Low Prio IsUpdate OnlyChanged False", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) le1 := api.NewLeafEntry(types.NewUpdate(nil, nil, 5, owner1, ts), testhelper.FlagsExisting, nil) lv.Add(le1) le2 := api.NewLeafEntry(types.NewUpdate(nil, nil, 6, owner2, ts), testhelper.FlagsNew, nil) @@ -1363,7 +1363,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // // on onlyIfPrioChanged == true we do not expect output. t.Run("New Low Prio IsNew OnlyChanged == True", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) lv.Add(api.NewLeafEntry(types.NewUpdate(nil, nil, 5, owner1, ts), testhelper.FlagsExisting, nil)) lv.Add(api.NewLeafEntry(types.NewUpdate(nil, nil, 6, owner2, ts), testhelper.FlagsNew, nil)) @@ -1381,7 +1381,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // // on onlyIfPrioChanged == true we do not expect output. t.Run("New Low Prio IsNew OnlyChanged == True, with running not existing", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) le1 := api.NewLeafEntry(types.NewUpdate(nil, nil, 5, owner1, ts), testhelper.FlagsExisting, nil) lv.Add(le1) le2 := api.NewLeafEntry(types.NewUpdate(nil, nil, 6, owner2, ts), testhelper.FlagsNew, nil) @@ -1397,7 +1397,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { t.Run("New Low Prio IsNew OnlyChanged == False", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) le1 := api.NewLeafEntry(types.NewUpdate(nil, nil, 5, owner1, ts), testhelper.FlagsExisting, nil) lv.Add(le1) le2 := api.NewLeafEntry(types.NewUpdate(nil, nil, 6, owner2, ts), testhelper.FlagsNew, nil) @@ -1414,7 +1414,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // If no entries exist in the list nil should be returned. t.Run("No Entries", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) le := lv.GetHighestPrecedence(true, false, false) @@ -1427,7 +1427,7 @@ func TestLeafVariants_GetHighesPrio(t *testing.T) { // make sure the secondhighes is also populated if the highes was the first entry t.Run("secondhighes populated if highes was first", func(t *testing.T) { - lv := api.NewLeafVariants(&TreeContext{}, nil) + lv := api.NewLeafVariants(api.NewOperationState(), nil) le1 := api.NewLeafEntry(types.NewUpdate(nil, nil, 1, owner1, ts), testhelper.FlagsDelete, nil) lv.Add(le1) le2 := api.NewLeafEntry(types.NewUpdate(nil, nil, 2, owner2, ts), testhelper.FlagsExisting, nil) @@ -2224,13 +2224,13 @@ func Test_RevertNonRevertive(t *testing.T) { t.Fatalf("failed to get existing config: %v", err) } - _, err = root.ImportConfig(ctx, nil, runningConfig, types.NewUpdateInsertFlags(), tc.poolFactory) + _, err = root.ImportConfig(ctx, nil, runningConfig, types.NewUpdateInsertFlags(), tc.GetTreeConfig().PoolFactory()) if err != nil { t.Fatalf("failed to import running config: %v", err) } for _, existing := range existingConfig { - _, err = root.ImportConfig(ctx, nil, existing, types.NewUpdateInsertFlags(), tc.poolFactory) + _, err = root.ImportConfig(ctx, nil, existing, types.NewUpdateInsertFlags(), tc.GetTreeConfig().PoolFactory()) if err != nil { t.Fatalf("failed to import existing config: %v", err) } @@ -2238,7 +2238,7 @@ func Test_RevertNonRevertive(t *testing.T) { // adding paths to the non revertive info, this should mark the paths as non revertive, and thus not be deleted in the end. for _, path := range tt.revertPaths { - tc.NonRevertiveInfo().Add(owner1, false, path) + tc.GetOperationState().NonRevertiveInfo().Add(owner1, false, path) } err = root.FinishInsertionPhase(ctx) diff --git a/pkg/tree/issue05_regression_test.go b/pkg/tree/issue05_regression_test.go new file mode 100644 index 00000000..bc74a5aa --- /dev/null +++ b/pkg/tree/issue05_regression_test.go @@ -0,0 +1,111 @@ +package tree + +import ( + "context" + "testing" +) + +// Regression gate for Issue 05: end-to-end split semantics through RootEntry.DeepCopy. +// These tests verify the behavioral invariants that the TreeConfig / OperationState +// split must uphold across the import, transaction, and branch-copy flows. + +// Behavior 1: Mutation of the copy's ExplicitDeletes does not affect the original. +func TestRootEntry_DeepCopy_CopyMutationDoesNotAffectOriginal(t *testing.T) { + tc := newTestTreeContext(t) + root, err := NewTreeRoot(context.Background(), tc) + if err != nil { + t.Fatal(err) + } + + copied, err := root.DeepCopy(context.Background()) + if err != nil { + t.Fatal(err) + } + + // Mutate the copy's ExplicitDeletes. + copied.GetTreeContext().GetOperationState().ExplicitDeletes().Add("copyOwner", 5, nil) + + // Original must still have zero entries under that intent. + paths := root.GetTreeContext().GetOperationState().ExplicitDeletes().GetByIntentName("copyOwner") + count := 0 + for range paths.Items() { + count++ + } + if count != 0 { + t.Fatalf("expected original ExplicitDeletes to be unaffected after copy mutation, got %d path(s)", count) + } +} + +// Behavior 2: Mutation of the original's ExplicitDeletes after the copy does not affect the copy. +func TestRootEntry_DeepCopy_OriginalMutationPostCopyDoesNotAffectCopy(t *testing.T) { + tc := newTestTreeContext(t) + root, err := NewTreeRoot(context.Background(), tc) + if err != nil { + t.Fatal(err) + } + + copied, err := root.DeepCopy(context.Background()) + if err != nil { + t.Fatal(err) + } + + // Mutate the original after the copy was taken. + root.GetTreeContext().GetOperationState().ExplicitDeletes().Add("origOwner", 5, nil) + + // Copy must still have zero entries under that intent. + paths := copied.GetTreeContext().GetOperationState().ExplicitDeletes().GetByIntentName("origOwner") + count := 0 + for range paths.Items() { + count++ + } + if count != 0 { + t.Fatalf("expected copy ExplicitDeletes to be unaffected by post-copy original mutation, got %d path(s)", count) + } +} + +// Behavior 3: Original and copy share the same TreeConfig pointer (immutable config identity). +func TestRootEntry_DeepCopy_SharesTreeConfigPointer(t *testing.T) { + tc := newTestTreeContext(t) + root, err := NewTreeRoot(context.Background(), tc) + if err != nil { + t.Fatal(err) + } + + copied, err := root.DeepCopy(context.Background()) + if err != nil { + t.Fatal(err) + } + + if root.GetTreeContext().GetTreeConfig() != copied.GetTreeContext().GetTreeConfig() { + t.Fatal("expected original and copy to share the same TreeConfig instance (pointer identity)") + } +} + +// Behavior 4: Pre-copy NonRevertiveInfo is preserved in the copy, but subsequent +// mutations to the copy do not bleed back to the original. +func TestRootEntry_DeepCopy_NonRevertiveInfoIsDeepCopied(t *testing.T) { + tc := newTestTreeContext(t) + root, err := NewTreeRoot(context.Background(), tc) + if err != nil { + t.Fatal(err) + } + + // Record a NonRevertiveInfo entry on the original before copying. + root.GetTreeContext().GetOperationState().NonRevertiveInfo().Add("ownerA", true) + + copied, err := root.DeepCopy(context.Background()) + if err != nil { + t.Fatal(err) + } + + // The copy should carry the pre-copy state. + if !copied.GetTreeContext().GetOperationState().NonRevertiveInfo().IsGenerallyNonRevertive("ownerA") { + t.Fatal("expected copy to preserve pre-copy NonRevertiveInfo entry") + } + + // Adding a new entry to the copy must not affect the original. + copied.GetTreeContext().GetOperationState().NonRevertiveInfo().Add("ownerCopyOnly", true) + if root.GetTreeContext().GetOperationState().NonRevertiveInfo().IsGenerallyNonRevertive("ownerCopyOnly") { + t.Fatal("expected original NonRevertiveInfo to be unaffected by copy-side mutation") + } +} diff --git a/pkg/tree/ops/treeexport.go b/pkg/tree/ops/treeexport.go index cee869bc..012cf262 100644 --- a/pkg/tree/ops/treeexport.go +++ b/pkg/tree/ops/treeexport.go @@ -17,7 +17,7 @@ func TreeExport(e api.Entry, owner string, priority int32, orphan bool) (*tree_p return nil, err } - explicitDeletes := e.GetTreeContext().ExplicitDeletes().GetByIntentName(owner).ToPathSlice() + explicitDeletes := e.GetTreeContext().GetOperationState().ExplicitDeletes().GetByIntentName(owner).ToPathSlice() var rootExportEntry *tree_persist.TreeElement if len(treeExport) != 0 { @@ -29,7 +29,7 @@ func TreeExport(e api.Entry, owner string, priority int32, orphan bool) (*tree_p IntentName: owner, Root: rootExportEntry, Priority: priority, - NonRevertive: e.GetTreeContext().NonRevertiveInfo().IsGenerallyNonRevertive(owner), + NonRevertive: e.GetTreeContext().GetOperationState().NonRevertiveInfo().IsGenerallyNonRevertive(owner), ExplicitDeletes: explicitDeletes, Orphan: orphan, }, nil diff --git a/pkg/tree/processors/processor_explicit_delete_test.go b/pkg/tree/processors/processor_explicit_delete_test.go index e71db4fd..3032a423 100644 --- a/pkg/tree/processors/processor_explicit_delete_test.go +++ b/pkg/tree/processors/processor_explicit_delete_test.go @@ -284,7 +284,7 @@ func TestExplicitDeleteVisitor_Visit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { root := tt.root() - root.GetTreeContext().ExplicitDeletes().Add(owner2, tt.priority, tt.explicitDeletes) + root.GetTreeContext().GetOperationState().ExplicitDeletes().Add(owner2, tt.priority, tt.explicitDeletes) err := root.FinishInsertionPhase(ctx) if err != nil { diff --git a/pkg/tree/processors/processor_importer.go b/pkg/tree/processors/processor_importer.go index f8c6daf8..95bfaed8 100644 --- a/pkg/tree/processors/processor_importer.go +++ b/pkg/tree/processors/processor_importer.go @@ -35,10 +35,10 @@ func (p *ImportConfigProcessor) GetStats() *types.ImportStats { func (p *ImportConfigProcessor) Run(ctx context.Context, e api.Entry, poolFactory pool.VirtualPoolFactory) error { // store non revertive info - e.GetTreeContext().NonRevertiveInfo().Add(p.importer.GetName(), p.importer.GetNonRevertive()) + e.GetTreeContext().GetOperationState().NonRevertiveInfo().Add(p.importer.GetName(), p.importer.GetNonRevertive()) // store explicit deletes - e.GetTreeContext().ExplicitDeletes().Add(p.importer.GetName(), p.importer.GetPriority(), p.importer.GetDeletes()) + e.GetTreeContext().GetOperationState().ExplicitDeletes().Add(p.importer.GetName(), p.importer.GetPriority(), p.importer.GetDeletes()) workerPool := poolFactory.NewVirtualPool(pool.VirtualFailFast) diff --git a/pkg/tree/root_entry.go b/pkg/tree/root_entry.go index 54b6f876..1fad8fae 100644 --- a/pkg/tree/root_entry.go +++ b/pkg/tree/root_entry.go @@ -84,7 +84,7 @@ func (r *RootEntry) ImportConfig(ctx context.Context, basePath *sdcpb.Path, impo } func (r *RootEntry) SetNonRevertiveIntent(intentName string, nonRevertive bool) { - r.GetTreeContext().NonRevertiveInfo().Add(intentName, nonRevertive) + r.GetTreeContext().GetOperationState().NonRevertiveInfo().Add(intentName, nonRevertive) } // String returns the string representation of the Tree. @@ -150,7 +150,7 @@ func (r *RootEntry) FinishInsertionPhase(ctx context.Context) error { edpsc := processors.ExplicitDeleteProcessorStatCollection{} // apply the explicit deletes - for deletePathPrio := range r.GetTreeContext().ExplicitDeletes().Items() { + for deletePathPrio := range r.GetTreeContext().GetOperationState().ExplicitDeletes().Items() { for path := range deletePathPrio.PathItems() { // set the priority // navigate to the stated path @@ -159,7 +159,7 @@ func (r *RootEntry) FinishInsertionPhase(ctx context.Context) error { log.Error(nil, "Applying explicit delete - path not found, skipping", "severity", "WARN", "path", path.ToXPath(false)) } edp := processors.NewExplicitDeleteProcessor(&processors.ExplicitDeleteTaskParams{Owner: deletePathPrio.GetOwner(), Priority: deletePathPrio.GetPrio()}) - err = edp.Run(ctx, entry, r.GetTreeContext().PoolFactory()) + err = edp.Run(ctx, entry, r.GetTreeContext().GetTreeConfig().PoolFactory()) if err != nil { return err } diff --git a/pkg/tree/root_entry_test.go b/pkg/tree/root_entry_test.go index de6563a1..e95b07b5 100644 --- a/pkg/tree/root_entry_test.go +++ b/pkg/tree/root_entry_test.go @@ -53,7 +53,7 @@ func TestRootEntry_TreeExport(t *testing.T) { cacheMutex: sync.Mutex{}, treeContext: tc, } - result.leafVariants = api.NewLeafVariants(tc, result) + result.leafVariants = api.NewLeafVariants(tc.GetOperationState(), result) result.leafVariants.Add( api.NewLeafEntry( @@ -101,7 +101,7 @@ func TestRootEntry_TreeExport(t *testing.T) { cacheMutex: sync.Mutex{}, treeContext: tc, } - result.leafVariants = api.NewLeafVariants(tc, result) + result.leafVariants = api.NewLeafVariants(tc.GetOperationState(), result) // create /interface sharedEntryAttributes interf := &sharedEntryAttributes{ @@ -111,7 +111,7 @@ func TestRootEntry_TreeExport(t *testing.T) { schemaMutex: sync.RWMutex{}, cacheMutex: sync.Mutex{}, } - interf.leafVariants = api.NewLeafVariants(tc, interf) + interf.leafVariants = api.NewLeafVariants(tc.GetOperationState(), interf) // add interf to result (root) _ = result.childs.AddOrGet(interf) @@ -166,7 +166,7 @@ func TestRootEntry_TreeExport(t *testing.T) { cacheMutex: sync.Mutex{}, treeContext: tc, } - result.leafVariants = api.NewLeafVariants(tc, result) + result.leafVariants = api.NewLeafVariants(tc.GetOperationState(), result) // create /interface sharedEntryAttributes interf := &sharedEntryAttributes{ @@ -176,7 +176,7 @@ func TestRootEntry_TreeExport(t *testing.T) { schemaMutex: sync.RWMutex{}, cacheMutex: sync.Mutex{}, } - interf.leafVariants = api.NewLeafVariants(tc, interf) + interf.leafVariants = api.NewLeafVariants(tc.GetOperationState(), interf) // add interf to result (root) _ = result.childs.AddOrGet(interf) @@ -209,7 +209,7 @@ func TestRootEntry_TreeExport(t *testing.T) { schemaMutex: sync.RWMutex{}, cacheMutex: sync.Mutex{}, } - system.leafVariants = api.NewLeafVariants(tc, system) + system.leafVariants = api.NewLeafVariants(tc.GetOperationState(), system) // add system to result (root) _ = result.childs.AddOrGet(system) @@ -264,7 +264,7 @@ func TestRootEntry_TreeExport(t *testing.T) { cacheMutex: sync.Mutex{}, treeContext: tc, } - result.leafVariants = api.NewLeafVariants(tc, result) + result.leafVariants = api.NewLeafVariants(tc.GetOperationState(), result) // create /interface sharedEntryAttributes interf := &sharedEntryAttributes{ @@ -274,7 +274,7 @@ func TestRootEntry_TreeExport(t *testing.T) { schemaMutex: sync.RWMutex{}, cacheMutex: sync.Mutex{}, } - interf.leafVariants = api.NewLeafVariants(tc, interf) + interf.leafVariants = api.NewLeafVariants(tc.GetOperationState(), interf) // add interf to result (root) _ = result.childs.AddOrGet(interf) @@ -453,12 +453,12 @@ func TestRootEntry_AddUpdatesRecursive(t *testing.T) { if err != nil { t.Fatal(err) } - schema, err := tc.schemaClient.GetSchemaSdcpbPath(ctx, nil) + schema, err := tc.GetTreeConfig().SchemaClient().GetSchemaSdcpbPath(ctx, nil) if err != nil { t.Fatal(err) } s.schema = schema.GetSchema() - s.leafVariants = api.NewLeafVariants(tc, s) + s.leafVariants = api.NewLeafVariants(tc.GetOperationState(), s) return s }, }, diff --git a/pkg/tree/sharedEntryAttributes.go b/pkg/tree/sharedEntryAttributes.go index 7142ee11..25b62915 100644 --- a/pkg/tree/sharedEntryAttributes.go +++ b/pkg/tree/sharedEntryAttributes.go @@ -79,7 +79,7 @@ func (s *sharedEntryAttributes) DeepCopy(tc api.TreeContext, parent api.Entry) ( } // copy leafvariants - result.leafVariants = s.leafVariants.DeepCopy(tc, result) + result.leafVariants = s.leafVariants.DeepCopy(tc.GetOperationState(), result) return result, nil } @@ -95,7 +95,7 @@ func NewSharedEntryAttributes(ctx context.Context, parent api.Entry, pathElemNam childs: api.NewChildMap(), treeContext: tc, } - s.leafVariants = api.NewLeafVariants(tc, s) + s.leafVariants = api.NewLeafVariants(tc.GetOperationState(), s) // populate the schema err := s.populateSchema(ctx) @@ -169,7 +169,7 @@ func (s *sharedEntryAttributes) loadDefaults(ctx context.Context) error { } func (s *sharedEntryAttributes) tryLoadingDefault(ctx context.Context, path *sdcpb.Path) (api.Entry, error) { - schema, err := s.treeContext.SchemaClient().GetSchemaSdcpbPath(ctx, path) + schema, err := s.treeContext.GetTreeConfig().SchemaClient().GetSchemaSdcpbPath(ctx, path) if err != nil { return nil, fmt.Errorf("error trying to load defaults for %s: %v", path.ToXPath(false), err) } @@ -218,7 +218,7 @@ func (s *sharedEntryAttributes) populateSchema(ctx context.Context) error { if getSchema { // trieve if the getSchema var is still true - schemaResp, err := s.treeContext.SchemaClient().GetSchemaSdcpbPath(ctx, path) + schemaResp, err := s.treeContext.GetTreeConfig().SchemaClient().GetSchemaSdcpbPath(ctx, path) if err != nil { return err } diff --git a/pkg/tree/sharedEntryAttributes_test.go b/pkg/tree/sharedEntryAttributes_test.go index f5c333a3..b8825722 100644 --- a/pkg/tree/sharedEntryAttributes_test.go +++ b/pkg/tree/sharedEntryAttributes_test.go @@ -106,7 +106,7 @@ func Test_sharedEntryAttributes_DeepCopy(t *testing.T) { choicesResolvers: api.ChoiceResolvers{}, parent: nil, treeContext: tc, - leafVariants: api.NewLeafVariants(tc, e), + leafVariants: api.NewLeafVariants(tc.GetOperationState(), e), } r := &RootEntry{ Entry: e, diff --git a/pkg/tree/tree_context.go b/pkg/tree/tree_context.go index d9c821b3..3e164128 100644 --- a/pkg/tree/tree_context.go +++ b/pkg/tree/tree_context.go @@ -6,46 +6,50 @@ import ( "github.com/sdcio/data-server/pkg/tree/api" ) -type TreeContext struct { - schemaClient schemaClient.SchemaClientBound - nonRevertiveInfo api.NonRevertiveInfos - explicitDeletes *api.DeletePathSet - poolFactory pool.VirtualPoolFactory +// treeConfig holds the immutable setup shared across all DeepCopy calls. +type treeConfig struct { + schemaClient schemaClient.SchemaClientBound + poolFactory pool.VirtualPoolFactory } -func NewTreeContext(sc schemaClient.SchemaClientBound, poolFactory pool.VirtualPoolFactory) *TreeContext { - return &TreeContext{ - schemaClient: sc, - nonRevertiveInfo: api.NewNonRevertiveInfos(), - explicitDeletes: api.NewDeletePaths(), - poolFactory: poolFactory, - } +func (c *treeConfig) SchemaClient() schemaClient.SchemaClientBound { + return c.schemaClient } -// deepCopy root is required to be set manually -func (t *TreeContext) DeepCopy() api.TreeContext { - tc := &TreeContext{ - schemaClient: t.schemaClient, - poolFactory: t.poolFactory, - } +func (c *treeConfig) PoolFactory() pool.VirtualPoolFactory { + return c.poolFactory +} - tc.nonRevertiveInfo = t.nonRevertiveInfo.DeepCopy() - tc.explicitDeletes = t.explicitDeletes.DeepCopy() - return tc +type TreeContext struct { + config *treeConfig + operationState api.OperationState } -func (t *TreeContext) PoolFactory() pool.VirtualPoolFactory { - return t.poolFactory +func NewTreeContext(sc schemaClient.SchemaClientBound, poolFactory pool.VirtualPoolFactory) *TreeContext { + return &TreeContext{ + config: &treeConfig{ + schemaClient: sc, + poolFactory: poolFactory, + }, + operationState: api.NewOperationState(), + } } -func (t *TreeContext) SchemaClient() schemaClient.SchemaClientBound { - return t.schemaClient +// GetTreeConfig returns the immutable tree setup. The same instance is shared +// across all DeepCopy calls. +func (t *TreeContext) GetTreeConfig() api.TreeConfig { + return t.config } -func (t *TreeContext) ExplicitDeletes() *api.DeletePathSet { - return t.explicitDeletes +// GetOperationState returns the mutable per-operation state. +func (t *TreeContext) GetOperationState() api.OperationState { + return t.operationState } -func (t *TreeContext) NonRevertiveInfo() api.NonRevertiveInfos { - return t.nonRevertiveInfo +// deepCopy root is required to be set manually +func (t *TreeContext) DeepCopy() api.TreeContext { + return &TreeContext{ + config: t.config, // shared by identity — immutable + operationState: t.operationState.DeepCopyState(), + } } diff --git a/pkg/tree/tree_context_test.go b/pkg/tree/tree_context_test.go new file mode 100644 index 00000000..decd32ae --- /dev/null +++ b/pkg/tree/tree_context_test.go @@ -0,0 +1,137 @@ +package tree + +import ( + "context" + "runtime" + "testing" + + "github.com/sdcio/data-server/pkg/pool" + "github.com/sdcio/data-server/pkg/utils/testhelper" + "go.uber.org/mock/gomock" +) + +func newTestTreeContext(t *testing.T) *TreeContext { + t.Helper() + mockCtrl := gomock.NewController(t) + scb, err := testhelper.GetSchemaClientBound(t, mockCtrl) + if err != nil { + t.Fatal(err) + } + return NewTreeContext(scb, pool.NewSharedTaskPool(context.Background(), runtime.GOMAXPROCS(0))) +} + +// Behavior 1: GetTreeConfig returns non-nil +func TestNewTreeContext_GetTreeConfig_NonNil(t *testing.T) { + tc := newTestTreeContext(t) + if tc.GetTreeConfig() == nil { + t.Fatal("expected GetTreeConfig() to return non-nil TreeConfig") + } +} + +// Behavior 2: GetOperationState returns non-nil +func TestNewTreeContext_GetOperationState_NonNil(t *testing.T) { + tc := newTestTreeContext(t) + if tc.GetOperationState() == nil { + t.Fatal("expected GetOperationState() to return non-nil OperationState") + } +} + +// Behavior 3: TreeConfig carries the schema client +func TestNewTreeContext_TreeConfig_SchemaClient(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + scb, err := testhelper.GetSchemaClientBound(t, mockCtrl) + if err != nil { + t.Fatal(err) + } + tc := NewTreeContext(scb, pool.NewSharedTaskPool(context.Background(), runtime.GOMAXPROCS(0))) + if tc.GetTreeConfig().SchemaClient() != scb { + t.Fatal("expected TreeConfig.SchemaClient() to be the same instance passed to NewTreeContext") + } +} + +// Behavior 4: TreeConfig carries the pool factory +func TestNewTreeContext_TreeConfig_PoolFactory(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + scb, err := testhelper.GetSchemaClientBound(t, mockCtrl) + if err != nil { + t.Fatal(err) + } + pf := pool.NewSharedTaskPool(context.Background(), runtime.GOMAXPROCS(0)) + tc := NewTreeContext(scb, pf) + if tc.GetTreeConfig().PoolFactory() != pf { + t.Fatal("expected TreeConfig.PoolFactory() to be the same instance passed to NewTreeContext") + } +} + +// Behavior 5: OperationState starts with non-nil, empty ExplicitDeletes +func TestNewTreeContext_OperationState_EmptyExplicitDeletes(t *testing.T) { + tc := newTestTreeContext(t) + ed := tc.GetOperationState().ExplicitDeletes() + if ed == nil { + t.Fatal("expected ExplicitDeletes() to be non-nil") + } + count := 0 + for range ed.Items() { + count++ + } + if count != 0 { + t.Fatalf("expected ExplicitDeletes to be empty, got %d entries", count) + } +} + +// Behavior 6: OperationState starts with non-nil, empty NonRevertiveInfo +func TestNewTreeContext_OperationState_EmptyNonRevertiveInfo(t *testing.T) { + tc := newTestTreeContext(t) + nri := tc.GetOperationState().NonRevertiveInfo() + if nri == nil { + t.Fatal("expected NonRevertiveInfo() to be non-nil") + } +} + +// Behavior 7: DeepCopy reuses the same TreeConfig instance (pointer identity) +func TestTreeContext_DeepCopy_ReusesTreeConfig(t *testing.T) { + tc := newTestTreeContext(t) + copied := tc.DeepCopy() + concreteCopy, ok := copied.(*TreeContext) + if !ok { + t.Fatal("DeepCopy did not return a *TreeContext") + } + if tc.GetTreeConfig() != concreteCopy.GetTreeConfig() { + t.Fatal("expected DeepCopy to reuse the same TreeConfig instance (pointer identity)") + } +} + +// Behavior 8: DeepCopy produces a distinct OperationState instance +func TestTreeContext_DeepCopy_DistinctOperationState(t *testing.T) { + tc := newTestTreeContext(t) + copied := tc.DeepCopy() + concreteCopy, ok := copied.(*TreeContext) + if !ok { + t.Fatal("DeepCopy did not return a *TreeContext") + } + if tc.GetOperationState() == concreteCopy.GetOperationState() { + t.Fatal("expected DeepCopy to produce a distinct OperationState instance") + } +} + +// Behavior 9: Mutations to original OperationState do not affect the copy +func TestTreeContext_DeepCopy_MutationIsolation(t *testing.T) { + tc := newTestTreeContext(t) + copied := tc.DeepCopy() + concreteCopy, ok := copied.(*TreeContext) + if !ok { + t.Fatal("DeepCopy did not return a *TreeContext") + } + // Add an explicit delete to the original; copy must stay empty + tc.GetOperationState().ExplicitDeletes().Add("owner1", 0, nil) + copyPaths := concreteCopy.GetOperationState().ExplicitDeletes().GetByIntentName("owner1") + copyCount := 0 + for range copyPaths.Items() { + copyCount++ + } + if copyCount != 0 { + t.Fatal("expected copy's ExplicitDeletes to remain empty after mutating original") + } +}