refactor: apply codegen field policies after reflection

Author: dmacvicar

internal/codegen/README.md

@@ -16,6 +16,7 @@ The generator follows the global XML <-> HCL mapping rules in `docs/schema-mappi
 
 - Inputs: libvirtxml reflection, docs registry (`internal/codegen/docs/*.yaml` from docindex/docgen), small config hooks
 - IR builder: normalizes struct metadata, tracks optionality, and carries doc strings
+- Field policy layer: applies Terraform-specific semantics after reflection (for example top-level identities, immutability, and exact path overrides)
 - Generators: templates render models/schemas/converters into `internal/generated/*.gen.go`
 - Orchestration: `main.go` wires the pieces and runs gofmt
 
@@ -53,6 +54,48 @@ The generator follows the global XML <-> HCL mapping rules in `docs/schema-mappi
 
 - Resources embed the generated models/schemas and call the conversions; add/override resource-specific fields (IDs, create helpers) manually
 
+## Field Policy Design
+
+The generator has two separate responsibilities:
+
+1. Structural analysis: reflect libvirtxml structs into IR using facts from Go types and, over time, RNG metadata. This layer should answer questions like "is this a pointer?", "is this an XML attribute?", and "is this nested or repeated?".
+2. Terraform policy: decide how those reflected fields behave in Terraform schemas and conversions. This layer owns decisions like `Computed`, `Required`, `RequiresReplace`, and "preserve user intent" behavior.
+
+Keep those layers separate. The parser should not accumulate ad hoc Terraform exceptions based on struct names. If a field needs special Terraform behavior, prefer a policy rule after reflection rather than embedding one-off conditionals into the reflector.
+
+### Policy rules
+
+Policy should be applied in an ordered pass over `StructIR` / `FieldIR`:
+
+- Generic scope-aware rules first
+- Exact path overrides second
+- Resource-specific fallbacks last
+
+Examples:
+
+- Top-level resource identity fields such as `id`, `uuid`, and `key` are provider-managed and should usually be `Computed`
+- Top-level `name` and some top-level `type` fields are immutable inputs and should usually be `Required` plus `RequiresReplace`
+- Nested `id` fields are not automatically provider-managed; many are part of libvirt configuration and should keep their reflected semantics unless an explicit override says otherwise
+- Reported-only fields such as storage pool `capacity` / `allocation` / `available` should be handled by explicit policy rules, not inferred from field names alone
+
+### Override strategy
+
+When the default rules are not enough, use explicit overrides keyed by full Terraform or XML path rather than helper functions like `isUserManagedFooStruct`.
+
+Good override targets:
+
+- `storage_pool.capacity`
+- `storage_pool.allocation`
+- `storage_volume.physical`
+
+Avoid:
+
+- Struct-name allowlists for one field
+- Field-name heuristics that ignore nesting scope
+- Mixing reflection logic with provider semantics in the same function
+
+This keeps the generator predictable, makes exceptions easy to audit, and prevents the parser from turning into a collection of special cases.
+
 ## Documentation tools
 
 - `cmd/docindex`: scrape `/usr/share/doc/libvirt/html` into an index used for prompting. Run `go run ./internal/codegen/cmd/docindex --input /usr/share/doc/libvirt/html --output internal/codegen/docs/.index.json`.

internal/codegen/main.go

@@ -9,6 +9,7 @@ import (
 	"github.com/dmacvicar/terraform-provider-libvirt/v2/internal/codegen/docregistry"
 	"github.com/dmacvicar/terraform-provider-libvirt/v2/internal/codegen/generator"
 	"github.com/dmacvicar/terraform-provider-libvirt/v2/internal/codegen/parser"
+	"github.com/dmacvicar/terraform-provider-libvirt/v2/internal/codegen/policy"
 	"github.com/dmacvicar/terraform-provider-libvirt/v2/internal/util/stringutil"
 	"libvirt.org/go/libvirtxml"
 )
@@ -97,6 +98,8 @@ func run() error {
 		}
 	}
 
+	policy.ApplyFieldPolicies(structs)
+
 	// Silently generate code
 
 	// Generate one file per struct

internal/codegen/parser/libvirtxml.go

@@ -106,9 +106,6 @@ func (r *LibvirtXMLReflector) ReflectStruct(structType reflect.Type) (*generator
 		}
 	}
 
-	// Apply universal and resource-specific patterns
-	r.applyFieldPatterns(structType.Name(), ir.Fields)
-
 	// Post-process: expand chardata+attribute fields into separate flattened fields
 	// Example: <memory unit='KiB'>524288</memory> → memory (value), memory_unit (attr)
 	//          <vcpu placement='static'>2</vcpu> → vcpu (value), vcpu_placement (attr)
@@ -182,109 +179,9 @@ func (r *LibvirtXMLReflector) ReflectStruct(structType reflect.Type) (*generator
 	}
 	ir.Fields = expandedFields
 
-	// Apply patterns again after field expansion
-	r.applyFieldPatterns(structType.Name(), ir.Fields)
-
 	return ir, nil
 }
 
-// applyFieldPatterns applies universal and resource-specific field patterns
-func (r *LibvirtXMLReflector) applyFieldPatterns(structName string, fields []*generator.FieldIR) {
-	for _, field := range fields {
-		// Universal patterns (apply to ALL resources)
-		switch field.TFName {
-		case "uuid", "id", "key":
-			if field.TFName == "id" && isUserManagedVLANTagIDStruct(structName) {
-				field.IsComputed = false
-				field.IsOptional = false
-				field.IsRequired = true
-				field.PlanModifier = "RequiresReplace"
-				continue
-			}
-
-			field.IsComputed = true
-			field.IsOptional = false
-			field.IsRequired = false
-			field.PlanModifier = "UseStateForUnknown"
-
-		case "name":
-			// At resource root level, name is required and immutable
-			if structName == "StoragePool" || structName == "Domain" || structName == "Network" || structName == "StorageVolume" {
-				field.IsRequired = true
-				field.IsOptional = false
-				field.PlanModifier = "RequiresReplace"
-			}
-
-		case "type":
-			// At resource root level, type is required and immutable
-			if structName == "StoragePool" || structName == "Domain" || structName == "Network" {
-				field.IsRequired = true
-				field.IsOptional = false
-				field.PlanModifier = "RequiresReplace"
-			}
-		}
-
-		// Resource-specific patterns
-		if structName == "StoragePool" {
-			// Skip unit fields - they stay optional
-			if field.IsFlattenedUnit {
-				continue
-			}
-
-			switch field.TFName {
-			case "capacity":
-				field.IsComputed = true
-				field.IsOptional = false
-				field.IsRequired = false
-				field.PlanModifier = "UseStateForUnknown"
-				// Pool capacity is purely reported by libvirt; always read from XML.
-				field.PreserveUserIntent = false
-
-			case "allocation", "available":
-				field.IsComputed = true
-				field.IsOptional = false
-				field.IsRequired = false
-				// Purely informational; always read from XML.
-				field.PreserveUserIntent = false
-			}
-		}
-
-		if structName == "StorageVolume" {
-			// Skip unit fields - they stay optional
-			if field.IsFlattenedUnit {
-				continue
-			}
-
-			switch field.TFName {
-			case "capacity":
-				field.IsComputed = true
-				field.IsOptional = false
-				field.IsRequired = false
-				// Keep PreserveUserIntent = true (set by analyzeField for pointer fields)
-				// so that when the user specifies capacity with a capacity_unit, the
-				// plan value is preserved on readback instead of the bytes-normalised
-				// value that libvirt returns (fixes issue #1253).
-
-			case "allocation", "physical":
-				field.IsComputed = true
-				field.IsOptional = false
-				field.IsRequired = false
-				// Purely informational; always read from XML.
-				field.PreserveUserIntent = false
-			}
-		}
-	}
-}
-
-func isUserManagedVLANTagIDStruct(structName string) bool {
-	switch structName {
-	case "NetworkVLANTag", "DomainInterfaceVLanTag", "NetworkPortVLANTag":
-		return true
-	default:
-		return false
-	}
-}
-
 func (r *LibvirtXMLReflector) analyzeField(structName string, field reflect.StructField) (*generator.FieldIR, error) {
 	// Skip XMLName fields (used by encoding/xml)
 	if field.Name == "XMLName" {

internal/codegen/policy/field_policy.go

@@ -0,0 +1,93 @@
+package policy
+
+import "github.com/dmacvicar/terraform-provider-libvirt/v2/internal/codegen/generator"
+
+// ApplyFieldPolicies mutates the IR with Terraform-specific schema/conversion
+// semantics after structural reflection is complete.
+func ApplyFieldPolicies(structs []*generator.StructIR) {
+	for _, s := range structs {
+		applyStructPolicies(s)
+	}
+}
+
+func applyStructPolicies(s *generator.StructIR) {
+	for _, field := range s.Fields {
+		if field.IsExcluded || field.IsCycle {
+			continue
+		}
+
+		applyTopLevelIdentityPolicy(s, field)
+		applyTopLevelImmutabilityPolicy(s, field)
+		applyReportedFieldPolicy(s, field)
+	}
+}
+
+func applyTopLevelIdentityPolicy(s *generator.StructIR, field *generator.FieldIR) {
+	if !s.IsTopLevel {
+		return
+	}
+
+	switch field.TFName {
+	case "uuid", "id", "key":
+		field.IsComputed = true
+		field.IsOptional = false
+		field.IsRequired = false
+		field.PlanModifier = "UseStateForUnknown"
+	}
+}
+
+func applyTopLevelImmutabilityPolicy(s *generator.StructIR, field *generator.FieldIR) {
+	if !s.IsTopLevel {
+		return
+	}
+
+	switch field.TFName {
+	case "name":
+		field.IsRequired = true
+		field.IsOptional = false
+		field.IsComputed = false
+		field.PlanModifier = "RequiresReplace"
+	case "type":
+		if s.Name != "StorageVolume" {
+			field.IsRequired = true
+			field.IsOptional = false
+			field.IsComputed = false
+			field.PlanModifier = "RequiresReplace"
+		}
+	}
+}
+
+func applyReportedFieldPolicy(s *generator.StructIR, field *generator.FieldIR) {
+	if field.IsFlattenedUnit {
+		return
+	}
+
+	switch s.Name {
+	case "StoragePool":
+		switch field.TFName {
+		case "capacity":
+			field.IsComputed = true
+			field.IsOptional = false
+			field.IsRequired = false
+			field.PlanModifier = "UseStateForUnknown"
+			field.PreserveUserIntent = false
+		case "allocation", "available":
+			field.IsComputed = true
+			field.IsOptional = false
+			field.IsRequired = false
+			field.PreserveUserIntent = false
+		}
+	case "StorageVolume":
+		switch field.TFName {
+		case "capacity":
+			field.IsComputed = true
+			field.IsOptional = false
+			field.IsRequired = false
+		case "allocation", "physical":
+			field.IsComputed = true
+			field.IsOptional = false
+			field.IsRequired = false
+			field.PreserveUserIntent = false
+		}
+	}
+}

internal/codegen/policy/field_policy_test.go

@@ -0,0 +1,52 @@
+package policy
+
+import (
+	"testing"
+
+	"github.com/dmacvicar/terraform-provider-libvirt/v2/internal/codegen/generator"
+)
+
+func TestApplyFieldPoliciesMarksTopLevelIdentityComputed(t *testing.T) {
+	root := &generator.StructIR{
+		Name:       "Network",
+		IsTopLevel: true,
+		Fields: []*generator.FieldIR{
+			{TFName: "id", IsRequired: true},
+		},
+	}
+
+	ApplyFieldPolicies([]*generator.StructIR{root})
+
+	field := root.Fields[0]
+	if !field.IsComputed {
+		t.Fatal("expected top-level id to be computed")
+	}
+	if field.IsRequired {
+		t.Fatal("expected top-level id to not be required")
+	}
+	if field.PlanModifier != "UseStateForUnknown" {
+		t.Fatalf("expected top-level id plan modifier UseStateForUnknown, got %q", field.PlanModifier)
+	}
+}
+
+func TestApplyFieldPoliciesLeavesNestedIdentityUserManaged(t *testing.T) {
+	nested := &generator.StructIR{
+		Name: "DomainAudio",
+		Fields: []*generator.FieldIR{
+			{TFName: "id", IsRequired: true},
+		},
+	}
+
+	ApplyFieldPolicies([]*generator.StructIR{nested})
+
+	field := nested.Fields[0]
+	if field.IsComputed {
+		t.Fatal("expected nested id to remain user-managed")
+	}
+	if !field.IsRequired {
+		t.Fatal("expected nested id to remain required")
+	}
+	if field.PlanModifier != "" {
+		t.Fatalf("expected nested id to have no plan modifier, got %q", field.PlanModifier)
+	}
+}

internal/regression/vlan_tag_schema_test.go

@@ -46,3 +46,43 @@ func TestDomainInterfaceVLanTagIDIsRequired(t *testing.T) {
 		t.Fatal("expected domain interface VLAN tag id to not be computed")
 	}
 }
+
+func TestDomainAudioIDIsRequired(t *testing.T) {
+	attr := generated.DomainAudioSchemaAttribute()
+	nested, ok := attr.(schema.SingleNestedAttribute)
+	if !ok {
+		t.Fatalf("expected SingleNestedAttribute, got %T", attr)
+	}
+
+	idAttr, ok := nested.Attributes["id"].(schema.Int64Attribute)
+	if !ok {
+		t.Fatalf("expected Int64Attribute for domain audio id, got %T", nested.Attributes["id"])
+	}
+
+	if !idAttr.Required {
+		t.Fatal("expected domain audio id to be required")
+	}
+	if idAttr.Computed {
+		t.Fatal("expected domain audio id to not be computed")
+	}
+}
+
+func TestDomainCPUMemoryTuneNodeIDIsRequired(t *testing.T) {
+	attr := generated.DomainCPUMemoryTuneNodeSchemaAttribute()
+	nested, ok := attr.(schema.SingleNestedAttribute)
+	if !ok {
+		t.Fatalf("expected SingleNestedAttribute, got %T", attr)
+	}
+
+	idAttr, ok := nested.Attributes["id"].(schema.Int64Attribute)
+	if !ok {
+		t.Fatalf("expected Int64Attribute for domain CPU memory tune node id, got %T", nested.Attributes["id"])
+	}
+
+	if !idAttr.Required {
+		t.Fatal("expected domain CPU memory tune node id to be required")
+	}
+	if idAttr.Computed {
+		t.Fatal("expected domain CPU memory tune node id to not be computed")
+	}
+}