fix: normalize identityref values in JSON importer and leafref validation

pkg/datastore/sync.go

@@ -134,11 +134,21 @@ func (d *Datastore) NewEmptyTree(ctx context.Context) (*tree.RootEntry, error) {
 
 func (d *Datastore) performRevert(ctx context.Context, t *tree.RootEntry) error {
 	log := logger.FromContext(ctx)
+
+	// Critical section 1: snapshot the intent store.
+	// Hold dmutex so that any concurrent delete transaction (device write +
+	// cache delete) fully completes before we read the cache. Without this,
+	// LoadAllButRunningIntents can race with IntentDelete and push stale intent
+	// config back to the device, undoing the deletion.
+	d.dmutex.Lock()
 	_, err := d.LoadAllButRunningIntents(ctx, t)
+	d.dmutex.Unlock()
 	if err != nil {
 		return err
 	}
 
+	// Tree processing operates only on the local copy t — no shared state is
+	// accessed, so no lock is required here.
 	err = t.FinishInsertionPhase(ctx)
 	if err != nil {
 		return err
@@ -164,6 +174,12 @@ func (d *Datastore) performRevert(ctx context.Context, t *tree.RootEntry) error
 	}
 
 	if performApply {
+		// Critical section 2: device write.
+		// Re-acquire dmutex to serialize the gNMI SET with in-flight
+		// transactions; a concurrent delete must not interleave its device write
+		// with ours.
+		d.dmutex.Lock()
+		defer d.dmutex.Unlock()
 		log.Info("reverting after sync")
 		resp, err := d.applyIntent(ctx, adapter.NewEntryOutputAdapter(t.Entry))
 		if err != nil {

pkg/datastore/sync_test.go

@@ -374,6 +374,7 @@ func TestApplyToRunning(t *testing.T) {
 
 			datastore := &Datastore{
 				syncTreeMutex: &sync.RWMutex{},
+				dmutex:        &sync.Mutex{},
 				syncTree:      syncTree,
 				taskPool:      pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)),
 				cacheClient:   tt.cacheClientFunc(ctrl),

pkg/server/transaction.go

@@ -156,5 +156,8 @@ func translateInternalToGrpcError(err error) error {
 	if errors.Is(err, datastore.ErrDatastoreLocked) {
 		return status.Error(codes.Aborted, err.Error())
 	}
+	if errors.Is(err, types.ErrTransactionOngoing) {
+		return status.Error(codes.Aborted, err.Error())
+	}
 	return err
 }

pkg/tree/importer/import_config_adapter.go

@@ -29,6 +29,9 @@ type ImportConfigAdapterElement interface {
 	// GetKeyValue can be called on Leafs or LeafList elements to retrieve the underlaying value
 	// When and were to expect a Leafs or LeafList is defined by the yang schema.
 	// The String value is typically used for the keys.
+	// Contract: for identityref leaf types, GetKeyValue must return the bare identity name
+	// (e.g. "BGP"), stripping any module prefix (e.g. "openconfig-policy-types:BGP").
+	// This ensures JSON_IETF and plain JSON inputs produce identical tree keys.
 	GetKeyValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (string, error)
 	// GetTVValue returns the TypedValue based value defined via the SchemaLeafType. Can also only be called on Leafs or LeafLists
 	GetTVValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (*sdcpb.TypedValue, error)

pkg/tree/importer/json/json_tree_importer.go

@@ -107,7 +107,14 @@ func (j *JsonTreeImporterElement) GetElements() []importer.ImportConfigAdapterEl
 }
 
 func (j *JsonTreeImporterElement) GetKeyValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (string, error) {
-	return fmt.Sprintf("%v", j.data), nil
+	if slt == nil {
+		return fmt.Sprintf("%v", j.data), nil
+	}
+	tv, err := j.GetTVValue(ctx, slt)
+	if err != nil {
+		return "", err
+	}
+	return tv.ToString(), nil
 }
 
 func (j *JsonTreeImporterElement) GetTVValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (*sdcpb.TypedValue, error) {

pkg/tree/importer/json/json_tree_importer_test.go

@@ -332,3 +332,34 @@ func TestJsonTreeImporter_UnknownContainer(t *testing.T) {
 		t.Error("GetElement(\"non-existent-container\") should return nil")
 	}
 }
+
+// TestJsonTreeImporter_GetKeyValue_Identityref verifies that GetKeyValue strips the module
+// prefix from a JSON_IETF identityref value and returns the bare identity name, and that
+// plain JSON (no prefix) and JSON_IETF (module-prefixed) inputs produce identical keys.
+func TestJsonTreeImporter_GetKeyValue_Identityref(t *testing.T) {
+	slt := &sdcpb.SchemaLeafType{
+		Type:                "identityref",
+		IdentityPrefixesMap: map[string]string{"BGP": "oc-policy-types"},
+		ModulePrefixMap:     map[string]string{"BGP": "openconfig-policy-types"},
+	}
+
+	tests := []struct {
+		name string
+		data any
+	}{
+		{"plain_json", "BGP"},
+		{"json_ietf_prefixed", "openconfig-policy-types:BGP"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			elem := newJsonTreeImporterElement("afi-safi-type", tt.data)
+			got, err := elem.GetKeyValue(context.Background(), slt)
+			if err != nil {
+				t.Fatalf("GetKeyValue() error: %v", err)
+			}
+			if got != "BGP" {
+				t.Errorf("GetKeyValue() = %q, want %q", got, "BGP")
+			}
+		})
+	}
+}

pkg/tree/ops/validation/validation_entry_leafref.go

@@ -195,7 +195,7 @@ func resolveLeafrefKeyPath(ctx context.Context, e api.Entry, keys map[string]*ty
 			return fmt.Errorf("no leafentry found")
 		}
 		tv := lvs[0].Value()
-		keys[k].Value = tv.GetStringVal()
+		keys[k].Value = tv.ToString()
 		keys[k].DoNotResolve = true
 	}
 	return nil

pkg/tree/ops/validation/validation_entry_leafref_test.go

@@ -23,6 +23,147 @@ import (
 	"go.uber.org/mock/gomock"
 )
 
+// buildTree creates a tree populated with config supplied as a plain-JSON map.
+func buildTree(t *testing.T, jsonConf map[string]any) *tree.RootEntry {
+	t.Helper()
+	ctx := context.Background()
+
+	sc, schema, err := testhelper.InitSDCIOSchema()
+	if err != nil {
+		t.Fatal(err)
+	}
+	scb := schemaClient.NewSchemaClientBound(schema, sc)
+	tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)))
+
+	root, err := tree.NewTreeRoot(ctx, tc)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	vpf := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))
+	_, err = root.ImportConfig(ctx, &sdcpb.Path{}, jsonImporter.NewJsonTreeImporter(jsonConf, "owner1", 500, false), types.NewUpdateInsertFlags(), vpf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := root.FinishInsertionPhase(ctx); err != nil {
+		t.Fatal(err)
+	}
+	return root
+}
+
+// TestLeafref_IdentityrefKey verifies that leafref validation resolves
+// current()-relative key predicates whose values are identityref leaves.
+// Without the tv.ToString() fix in resolveLeafrefKeyPath the key is "" and
+// the reference cannot be found.
+func TestLeafref_IdentityrefKey(t *testing.T) {
+	tests := []struct {
+		name        string
+		conf        map[string]any
+		wantErrLen  int
+	}{
+		{
+			name: "identityref key resolves - pass",
+			conf: map[string]any{
+				"identityref":        map[string]any{"cryptoA": "otherAlgo"},
+				"intentityrefkey":    []any{map[string]any{"crypto": "otherAlgo", "description": "my-desc"}},
+				"intentityrefkey-ref": "my-desc",
+			},
+			wantErrLen: 0,
+		},
+		{
+			name: "identityref key mismatch - fail",
+			conf: map[string]any{
+				"identityref":        map[string]any{"cryptoA": "rsa"},
+				"intentityrefkey":    []any{map[string]any{"crypto": "otherAlgo", "description": "my-desc"}},
+				"intentityrefkey-ref": "my-desc",
+			},
+			wantErrLen: 1,
+		},
+	}
+
+	lrefPath := &sdcpb.Path{
+		Elem: []*sdcpb.PathElem{sdcpb.NewPathElem("intentityrefkey-ref", nil)},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			root := buildTree(t, tt.conf)
+
+			e, err := ops.NavigateSdcpbPath(ctx, root.Entry, lrefPath)
+			if err != nil {
+				t.Fatalf("NavigateSdcpbPath: %v", err)
+			}
+
+			valConf := config.NewValidationConfig()
+			valConf.DisabledValidators.DisableAll()
+			valConf.DisabledValidators.Leafref = false
+			valConf.DisableConcurrency = true
+
+			result, _ := validation.Validate(ctx, e, valConf, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)))
+			if len(result) != tt.wantErrLen {
+				t.Errorf("Validate() returned %d errors, want %d: %v", len(result), tt.wantErrLen, result)
+			}
+		})
+	}
+}
+
+// TestMust_IdentityrefKey verifies that must-statement validation navigates
+// correctly when the predicate key value originates from an identityref leaf.
+// Without the p.StripPathElemPrefixPath() fix in Navigate the YangString()
+// prefix ("sdcio_identity:otherAlgo") prevents the list entry from being found.
+func TestMust_IdentityrefKey(t *testing.T) {
+	mustPath := &sdcpb.Path{
+		Elem: []*sdcpb.PathElem{sdcpb.NewPathElem("identityref-must-test", nil)},
+	}
+
+	tests := []struct {
+		name       string
+		conf       map[string]any
+		wantErrLen int
+	}{
+		{
+			name: "must satisfied - pass",
+			conf: map[string]any{
+				"identityref-must-test": map[string]any{"crypto-ref": "otherAlgo"},
+				"intentityrefkey":       []any{map[string]any{"crypto": "otherAlgo", "description": "present"}},
+			},
+			wantErrLen: 0,
+		},
+		{
+			name: "must not satisfied - fail",
+			conf: map[string]any{
+				"identityref-must-test": map[string]any{"crypto-ref": "otherAlgo"},
+				// no intentityrefkey entry → description path resolves to "" → must false
+			},
+			wantErrLen: 1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			root := buildTree(t, tt.conf)
+
+			e, err := ops.NavigateSdcpbPath(ctx, root.Entry, mustPath)
+			if err != nil {
+				t.Fatalf("NavigateSdcpbPath: %v", err)
+			}
+
+			valConf := config.NewValidationConfig()
+			valConf.DisabledValidators.DisableAll()
+			valConf.DisabledValidators.MustStatement = false
+			valConf.DisableConcurrency = true
+
+			result, _ := validation.Validate(ctx, e, valConf, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)))
+			if len(result) != tt.wantErrLen {
+				t.Errorf("Validate() returned %d errors, want %d: %v", len(result), tt.wantErrLen, result)
+			}
+		})
+	}
+}
+
 func Test_sharedEntryAttributes_validateLeafRefs(t *testing.T) {
 	owner1 := "owner1"
 

pkg/tree/ops/validation/yang-parser-adapter.go

@@ -120,6 +120,8 @@ func (y *yangParserEntryAdapter) Navigate(p *sdcpb.Path) (xpath.Entry, error) {
 		return y, nil
 	}
 
+	p.StripPathElemPrefixPath()
+
 	entry, err := ops.NavigateSdcpbPath(y.ctx, y.e, p)
 	if err != nil {
 		return newYangParserValueEntry(xpath.NewNodesetDatum([]xutils.XpathNode{}), err), nil

tests/schema/sdcio_model_identity.yang

@@ -38,5 +38,31 @@ module sdcio_model_identity {
         type string;
       }
     }
+
+    // must exercises Navigate prefix stripping: the key predicate value is
+    // derived from an identityref leaf via current(), which YangString()
+    // returns as "prefix:value".  Navigate must strip that prefix before
+    // calling NavigateSdcpbPath, otherwise the list entry cannot be found.
+    // The sdcio_identity: self-prefix on path steps is a non-standard alias
+    // (the importing module uses a different alias), exercising the strip.
+    container identityref-must-test {
+      leaf crypto-ref {
+        type identityref {
+          base identity_base:crypto-alg;
+        }
+      }
+      must "../sdcio_identity:intentityrefkey[sdcio_identity:crypto=current()/sdcio_identity:crypto-ref]/sdcio_identity:description != ''" {
+        error-message "intentityrefkey entry for crypto-ref must exist with non-empty description";
+      }
+    }
+
+    // leafref exercises resolveLeafrefKeyPath identityref key normalisation:
+    // the key predicate current()/../identityref/cryptoA resolves to an
+    // IdentityrefVal; tv.ToString() must be used (not tv.GetStringVal()).
+    leaf intentityrefkey-ref {
+      type leafref {
+        path "../intentityrefkey[crypto=current()/../identityref/cryptoA]/description";
+      }
+    }
   }
 }