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") + } +}