Addressed #725, node_builder now performs some more detailed checks using reflection when handling empty nodes that need to remain empty.

Author: daveshanley

bundler/bundler_composer_test.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/pb33f/libopenapi"
 	"github.com/pb33f/libopenapi/datamodel"
+	v3 "github.com/pb33f/libopenapi/datamodel/high/v3"
 	"github.com/pb33f/libopenapi/index"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -754,3 +755,104 @@ oneOf:
 
 	runtime.GC()
 }
+
+const emptyDefaultServerSpec = `openapi: 3.0.0
+info:
+  title: defaults
+  version: 1.0.0
+servers:
+  - url: https://{env}.example.com
+    variables:
+      env:
+        default: ""
+        description: environment host
+  - url: https://{shard}.example.com
+    variables:
+      shard:
+        description: shard id
+        default: ""
+  - url: https://{slot}.example.com
+    variables:
+      slot:
+        default: ""
+paths: {}`
+
+func TestBundleBytesComposed_PreservesEmptyServerVariableDefaults(t *testing.T) {
+	spec := emptyDefaultServerSpec
+
+	tmp := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(tmp, "main.yaml"), []byte(spec), 0o644))
+
+	data, err := os.ReadFile(filepath.Join(tmp, "main.yaml"))
+	require.NoError(t, err)
+
+	bundled, err := BundleBytesComposed(data, &datamodel.DocumentConfiguration{
+		BasePath: tmp,
+	}, nil)
+	require.NoError(t, err)
+
+	doc, err := libopenapi.NewDocument(bundled)
+	require.NoError(t, err)
+
+	model, err := doc.BuildV3Model()
+	require.NoError(t, err)
+
+	var envVar, shardVar *v3.ServerVariable
+	for _, srv := range model.Model.Servers {
+		if srv == nil || srv.Variables == nil {
+			continue
+		}
+		if candidate := srv.Variables.GetOrZero("env"); candidate != nil {
+			envVar = candidate
+		}
+		if candidate := srv.Variables.GetOrZero("shard"); candidate != nil {
+			shardVar = candidate
+		}
+	}
+
+	require.NotNil(t, envVar, "env variable must exist")
+	assert.Equal(t, "", envVar.Default)
+	assert.False(t, envVar.GoLow().Default.IsEmpty())
+	assert.Equal(t, "environment host", envVar.Description)
+
+	require.NotNil(t, shardVar, "shard variable must exist")
+	assert.Equal(t, "", shardVar.Default)
+	assert.False(t, shardVar.GoLow().Default.IsEmpty())
+	assert.Equal(t, "shard id", shardVar.Description)
+
+	slotVar := model.Model.Servers[2].Variables.GetOrZero("slot")
+	require.NotNil(t, slotVar, "slot variable must exist")
+	assert.Equal(t, "", slotVar.Default)
+	assert.False(t, slotVar.GoLow().Default.IsEmpty())
+	assert.Equal(t, "", slotVar.Description)
+}
+
+func TestBundleBytes_PreservesEmptyServerVariableDefaults(t *testing.T) {
+	spec := []byte(emptyDefaultServerSpec)
+
+	bundled, err := BundleBytes(spec, &datamodel.DocumentConfiguration{})
+	require.NoError(t, err)
+
+	doc, err := libopenapi.NewDocument(bundled)
+	require.NoError(t, err)
+
+	model, err := doc.BuildV3Model()
+	require.NoError(t, err)
+
+	require.Len(t, model.Model.Servers, 3)
+
+	envVar := model.Model.Servers[0].Variables.GetOrZero("env")
+	require.NotNil(t, envVar)
+	assert.Equal(t, "", envVar.Default)
+	assert.False(t, envVar.GoLow().Default.IsEmpty())
+
+	shardVar := model.Model.Servers[1].Variables.GetOrZero("shard")
+	require.NotNil(t, shardVar)
+	assert.Equal(t, "", shardVar.Default)
+	assert.False(t, shardVar.GoLow().Default.IsEmpty())
+
+	slotVar := model.Model.Servers[2].Variables.GetOrZero("slot")
+	require.NotNil(t, slotVar)
+	assert.Equal(t, "", slotVar.Default)
+	assert.False(t, slotVar.GoLow().Default.IsEmpty())
+}

datamodel/high/node_builder.go

@@ -59,6 +59,31 @@ func (n *NodeBuilder) add(key string, i int) {
 		return
 	}
 
+	var (
+		lowFieldValue reflect.Value
+		lowFieldValid bool
+	)
+
+	if n.Low != nil && !reflect.ValueOf(n.Low).IsZero() {
+		low := reflect.ValueOf(n.Low)
+		if low.Kind() == reflect.Ptr && !low.IsNil() {
+			elem := low.Elem()
+			if elem.IsValid() {
+				field := elem.FieldByName(key)
+				if field.IsValid() {
+					lowFieldValue = field
+					lowFieldValid = true
+				}
+			}
+		} else if low.IsValid() {
+			field := low.FieldByName(key)
+			if field.IsValid() {
+				lowFieldValue = field
+				lowFieldValid = true
+			}
+		}
+	}
+
 	// if the key is 'Extensions' then we need to extract the keys from the map
 	// and add them to the node builder.
 	if key == "Extensions" {
@@ -137,6 +162,27 @@ func (n *NodeBuilder) add(key string, i int) {
 			}
 		}
 	}
+
+	if isZero && lowFieldValid {
+		var lowInterface any
+		if lowFieldValue.Kind() == reflect.Ptr {
+			if !lowFieldValue.IsNil() {
+				lowInterface = lowFieldValue.Elem().Interface()
+			}
+		} else {
+			lowInterface = lowFieldValue.Interface()
+		}
+		if lowInterface != nil {
+			if emptier, ok := lowInterface.(interface{ IsEmpty() bool }); ok && !emptier.IsEmpty() {
+				isZero = false
+			} else if nodeGetter, ok := lowInterface.(interface{ GetValueNode() *yaml.Node }); ok {
+				if node := nodeGetter.GetValueNode(); node != nil {
+					isZero = false
+				}
+			}
+		}
+	}
+
 	if !renderZeroFlag && isZero || omitEmptyFlag && isZero {
 		return
 	}
@@ -180,8 +226,7 @@ func (n *NodeBuilder) add(key string, i int) {
 	// so skip and default to 0, which means a new entry to the spec.
 	// this will place new content and the top of the rendered object.
 	if n.Low != nil && !reflect.ValueOf(n.Low).IsZero() {
-		lowFieldValue := reflect.ValueOf(n.Low).Elem().FieldByName(key)
-		if lowFieldValue.IsValid() {
+		if lowFieldValid {
 			fLow := lowFieldValue.Interface()
 			value = reflect.ValueOf(fLow)
 

document_examples_test.go

@@ -639,5 +639,5 @@ func ExampleNewDocument_modifyAndReRender() {
 	fmt.Printf("There were %d original paths. There are now %d paths in the document\n", originalPaths, newPaths)
 	fmt.Printf("The new spec has %d bytes\n", len(rawBytes))
 	// Output: There were 13 original paths. There are now 14 paths in the document
-	// The new spec has 31213 bytes
+	// The new spec has 31406 bytes
 }

document_test.go

@@ -247,7 +247,7 @@ func TestDocument_RenderAndReload_ChangeCheck_Stripe(t *testing.T) {
 		// get flat list of changes.
 		flatChanges := compReport.GetAllChanges()
 
-		// remove everything that is a description change (stripe has a lot of those from having 519 empty descriptions)
+		// remove everything that is a description change
 		var filtered []*model.Change
 		for i := range flatChanges {
 			if flatChanges[i].Property != "description" {
@@ -259,9 +259,9 @@ func TestDocument_RenderAndReload_ChangeCheck_Stripe(t *testing.T) {
 		tc := compReport.TotalChanges()
 		bc := compReport.TotalBreakingChanges()
 		assert.Equal(t, 0, bc)
-		assert.Equal(t, 819, tc)
+		assert.Equal(t, 9, tc)
 
-		// there should be no other changes than the 519 descriptions.
+		// there should be no other changes besides descriptions.
 		assert.Equal(t, 0, len(filtered))
 		done <- struct{}{}
 	}()