From 8307aaf0f6f048723e4d3c78a9cd3f34a7a7b648 Mon Sep 17 00:00:00 2001
From: Matthew Barnes <mbarnes@fedorapeople.org>
Date: Tue, 16 Apr 2024 13:07:57 -0400
Subject: [PATCH 1/3] api: Enforce visibility struct tags

This enforces property visibility when creating or updating ARO-HCP
resource types to comply with the OpenAPI spec.  The implementation
encodes property visibility as struct tags.

The autorest-generated API models have the useful characteristic of
all struct fields being pointers.  This allows us to identify which
properties were explicitly provided in a create or update request.

However the generated models do not have property metadata encoded
into struct tags, and we can't (or shouldn't) modify the generated
models to add such information.

To get around this limitation, struct tags for property visibiility
are encoded into the INTERNAL API models, which are not generated.
The internal API models are of different Go types than the generated
API models and are therefore not compatible, however the field names
in each set of models are identical where property visibility is
explicit (as opposed to implicitly inherited from parent types).

We can therefore build a lookup table of visibility struct tags by
field name to use when evaluating create and update requests using
the generated API models.  We call this table a StructTagMap:

type StructTagMap map[string]reflect.StructTag

where the map keys are dotted struct field names (e.g. "A.B.C.D").

Then, while recursing over a generated API model type, we keep
track of struct field names in order to build lookup keys for the
table.  This effectively allows struct tags from one Go type to be
grafted on to another similar but incompatible Go type.
---
 .../hcpopenshiftclusters_methods.go           |   5 +-
 internal/api/v20240610preview/register.go     |   5 +-
 internal/api/visibility.go                    | 276 +++++
 internal/api/visibility_test.go               | 983 ++++++++++++++++++
 internal/go.mod                               |   1 +
 5 files changed, 1268 insertions(+), 2 deletions(-)
 create mode 100644 internal/api/visibility.go
 create mode 100644 internal/api/visibility_test.go

diff --git a/internal/api/v20240610preview/hcpopenshiftclusters_methods.go b/internal/api/v20240610preview/hcpopenshiftclusters_methods.go
index fe1f1ade3..49f9ef658 100644
--- a/internal/api/v20240610preview/hcpopenshiftclusters_methods.go
+++ b/internal/api/v20240610preview/hcpopenshiftclusters_methods.go
@@ -304,7 +304,10 @@ func (c *HcpOpenShiftClusterResource) ValidateStatic(current api.VersionedHCPOpe
 		"Content validation failed on multiple fields")
 	cloudError.Details = make([]arm.CloudErrorBody, 0)
 
-	// FIXME Validate visibility tags by comparing the new cluster (c) to current.
+	errorDetails = api.ValidateVisibility(c, current, clusterStructTagMap, updating)
+	if errorDetails != nil {
+		cloudError.Details = append(cloudError.Details, errorDetails...)
+	}
 
 	c.Normalize(&normalized)
 
diff --git a/internal/api/v20240610preview/register.go b/internal/api/v20240610preview/register.go
index 0cf6427d7..50730abfe 100644
--- a/internal/api/v20240610preview/register.go
+++ b/internal/api/v20240610preview/register.go
@@ -17,7 +17,10 @@ func (v version) String() string {
 	return "2024-06-10-preview"
 }
 
-var validate = api.NewValidator()
+var (
+	validate            = api.NewValidator()
+	clusterStructTagMap = api.NewStructTagMap[api.HCPOpenShiftCluster]()
+)
 
 func EnumValidateTag[S ~string](values ...S) string {
 	s := make([]string, len(values))
diff --git a/internal/api/visibility.go b/internal/api/visibility.go
new file mode 100644
index 000000000..b93f33307
--- /dev/null
+++ b/internal/api/visibility.go
@@ -0,0 +1,276 @@
+package api
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+
+	"github.com/Azure/ARO-HCP/internal/api/arm"
+)
+
+// Property visibility meanings:
+// https://azure.github.io/typespec-azure/docs/howtos/ARM/resource-type#property-visibility-and-other-constraints
+//
+// Field mutability guidelines:
+// https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#resource-schema--field-mutability
+
+const VisibilityStructTagKey = "visibility"
+
+// VisibilityFlags holds a visibility struct tag value as bit flags.
+type VisibilityFlags uint8
+
+const (
+	VisibilityRead VisibilityFlags = 1 << iota
+	VisibilityCreate
+	VisibilityUpdate
+
+	// option flags
+	VisibilityCaseInsensitive
+
+	VisibilityDefault = VisibilityRead | VisibilityCreate | VisibilityUpdate
+)
+
+func (f VisibilityFlags) ReadOnly() bool {
+	return f&(VisibilityRead|VisibilityCreate|VisibilityUpdate) == VisibilityRead
+}
+
+func (f VisibilityFlags) CanUpdate() bool {
+	return f&VisibilityUpdate != 0
+}
+
+func (f VisibilityFlags) CaseInsensitive() bool {
+	return f&VisibilityCaseInsensitive != 0
+}
+
+func (f VisibilityFlags) String() string {
+	s := []string{}
+	if f&VisibilityRead != 0 {
+		s = append(s, "read")
+	}
+	if f&VisibilityCreate != 0 {
+		s = append(s, "create")
+	}
+	if f&VisibilityUpdate != 0 {
+		s = append(s, "update")
+	}
+	if f&VisibilityCaseInsensitive != 0 {
+		s = append(s, "nocase")
+	}
+	return strings.Join(s, " ")
+}
+
+func GetVisibilityFlags(tag reflect.StructTag) (VisibilityFlags, bool) {
+	var flags VisibilityFlags
+
+	tagValue, ok := tag.Lookup(VisibilityStructTagKey)
+	if ok {
+		for _, v := range strings.Fields(tagValue) {
+			switch strings.ToLower(v) {
+			case "read":
+				flags |= VisibilityRead
+			case "create":
+				flags |= VisibilityCreate
+			case "update":
+				flags |= VisibilityUpdate
+			case "nocase":
+				flags |= VisibilityCaseInsensitive
+			default:
+				panic(fmt.Sprintf("Unknown visibility tag value '%s'", v))
+			}
+		}
+	}
+
+	return flags, ok
+}
+
+func join(ns, name string) string {
+	res := ns
+	if res != "" {
+		res += "."
+	}
+	res += name
+	return res
+}
+
+type StructTagMap map[string]reflect.StructTag
+
+func buildStructTagMap(m StructTagMap, t reflect.Type, path string) {
+	switch t.Kind() {
+	case reflect.Pointer, reflect.Slice:
+		buildStructTagMap(m, t.Elem(), path)
+
+	case reflect.Struct:
+		for i := 0; i < t.NumField(); i++ {
+			field := t.Field(i)
+			subpath := join(path, field.Name)
+
+			if len(field.Tag) > 0 {
+				m[subpath] = field.Tag
+			}
+
+			buildStructTagMap(m, field.Type, subpath)
+		}
+	}
+}
+
+// NewStructTagMap returns a mapping of dot-separated struct field names
+// to struct tags for the given type.  Each versioned API should create
+// its own visibiilty map for tracked resource types.
+//
+// Note: This assumes field names for internal and versioned structs are
+// identical where visibility is explicitly specified. If some divergence
+// emerges, one workaround could be to pass a field name override map.
+func NewStructTagMap[T any]() StructTagMap {
+	m := StructTagMap{}
+	buildStructTagMap(m, reflect.TypeFor[T](), "")
+	return m
+}
+
+type validateVisibility struct {
+	m        StructTagMap
+	updating bool
+	errs     []arm.CloudErrorBody
+}
+
+func ValidateVisibility(v, w interface{}, m StructTagMap, updating bool) []arm.CloudErrorBody {
+	vv := validateVisibility{
+		m:        m,
+		updating: updating,
+	}
+	vv.recurse(reflect.ValueOf(v), reflect.ValueOf(w), "", "", "", VisibilityDefault)
+	return vv.errs
+}
+
+func (vv *validateVisibility) recurse(v, w reflect.Value, mKey, namespace, fieldname string, implicitVisibility VisibilityFlags) {
+	flags, ok := GetVisibilityFlags(vv.m[mKey])
+	if !ok {
+		flags = implicitVisibility
+	}
+
+	if v.Type() != w.Type() {
+		panic(fmt.Sprintf("%s: value types differ (%s vs %s)", join(namespace, fieldname), v.Type().Name(), w.Type().Name()))
+	}
+
+	// Generated API structs are all pointer fields. A nil value in
+	// the incoming request (v) means the value is absent, which is
+	// always acceptable for visibility validation.
+	switch v.Kind() {
+	case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
+		if v.IsNil() {
+			return
+		}
+	}
+
+	switch v.Kind() {
+	case reflect.Bool:
+		if v.Bool() != w.Bool() {
+			vv.checkFlags(flags, namespace, fieldname)
+		}
+
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		if v.Int() != w.Int() {
+			vv.checkFlags(flags, namespace, fieldname)
+		}
+
+	case reflect.Uint, reflect.Uintptr, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
+		if v.Uint() != w.Uint() {
+			vv.checkFlags(flags, namespace, fieldname)
+		}
+
+	case reflect.Float32, reflect.Float64:
+		if v.Float() != w.Float() {
+			vv.checkFlags(flags, namespace, fieldname)
+		}
+
+	case reflect.Complex64, reflect.Complex128:
+		if v.Complex() != w.Complex() {
+			vv.checkFlags(flags, namespace, fieldname)
+		}
+
+	case reflect.String:
+		if flags.CaseInsensitive() {
+			if !strings.EqualFold(v.String(), w.String()) {
+				vv.checkFlags(flags, namespace, fieldname)
+			}
+		} else {
+			if v.String() != w.String() {
+				vv.checkFlags(flags, namespace, fieldname)
+			}
+		}
+
+	case reflect.Slice:
+		if w.IsNil() {
+			vv.checkFlags(flags, namespace, fieldname)
+			return
+		}
+
+		fallthrough
+
+	case reflect.Array:
+		if v.Len() != w.Len() {
+			vv.checkFlags(flags, namespace, fieldname)
+		} else {
+			for i := 0; i < min(v.Len(), w.Len()); i++ {
+				subscript := fmt.Sprintf("[%d]", i)
+				vv.recurse(v.Index(i), w.Index(i), mKey, namespace, fieldname+subscript, flags)
+			}
+		}
+
+	case reflect.Interface, reflect.Pointer:
+		if w.IsNil() {
+			vv.checkFlags(flags, namespace, fieldname)
+		} else {
+			vv.recurse(v.Elem(), w.Elem(), mKey, namespace, fieldname, flags)
+		}
+
+	case reflect.Map:
+		if w.IsNil() || v.Len() != w.Len() {
+			vv.checkFlags(flags, namespace, fieldname)
+		} else {
+			iter := v.MapRange()
+			for iter.Next() {
+				k := iter.Key()
+
+				subscript := fmt.Sprintf("[%q]", k.Interface())
+				if w.MapIndex(k).IsValid() {
+					vv.recurse(v.MapIndex(k), w.MapIndex(k), mKey, namespace, fieldname+subscript, flags)
+				} else {
+					vv.checkFlags(flags, namespace, fieldname+subscript)
+				}
+			}
+		}
+
+	case reflect.Struct:
+		for i := 0; i < v.NumField(); i++ {
+			structField := v.Type().Field(i)
+			mKeyNext := join(mKey, structField.Name)
+			namespaceNext := join(namespace, fieldname)
+			fieldnameNext := GetJSONTagName(vv.m[mKeyNext])
+			if fieldnameNext == "" {
+				fieldnameNext = structField.Name
+			}
+			vv.recurse(v.Field(i), w.Field(i), mKeyNext, namespaceNext, fieldnameNext, flags)
+		}
+	}
+}
+
+func (vv *validateVisibility) checkFlags(flags VisibilityFlags, namespace, fieldname string) {
+	if flags.ReadOnly() {
+		vv.errs = append(vv.errs,
+			arm.CloudErrorBody{
+				Code:    arm.CloudErrorCodeInvalidRequestContent,
+				Message: fmt.Sprintf("Field '%s' is read-only", fieldname),
+				Target:  join(namespace, fieldname),
+			})
+	} else if vv.updating && !flags.CanUpdate() {
+		vv.errs = append(vv.errs,
+			arm.CloudErrorBody{
+				Code:    arm.CloudErrorCodeInvalidRequestContent,
+				Message: fmt.Sprintf("Field '%s' cannot be updated", fieldname),
+				Target:  join(namespace, fieldname),
+			})
+	}
+}
diff --git a/internal/api/visibility_test.go b/internal/api/visibility_test.go
new file mode 100644
index 000000000..645f354b5
--- /dev/null
+++ b/internal/api/visibility_test.go
@@ -0,0 +1,983 @@
+package api
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestVisibilityFlags(t *testing.T) {
+	tests := []struct {
+		name                  string
+		tag                   reflect.StructTag
+		expectString          string
+		expectReadOnly        bool
+		expectCanUpdate       bool
+		expectCaseInsensitive bool
+	}{
+		{
+			name:                  "Visibility: (none)",
+			tag:                   reflect.StructTag("visibility:\"\""),
+			expectString:          "",
+			expectReadOnly:        false,
+			expectCanUpdate:       false,
+			expectCaseInsensitive: false,
+		},
+		{
+			name:                  "Visibility: read",
+			tag:                   reflect.StructTag("visibility:\"read\""),
+			expectString:          "read",
+			expectReadOnly:        true,
+			expectCanUpdate:       false,
+			expectCaseInsensitive: false,
+		},
+		{
+			name:                  "Visibility: create",
+			tag:                   reflect.StructTag("visibility:\"create\""),
+			expectString:          "create",
+			expectReadOnly:        false,
+			expectCanUpdate:       false,
+			expectCaseInsensitive: false,
+		},
+		{
+			name:                  "Visibility: update",
+			tag:                   reflect.StructTag("visibility:\"update\""),
+			expectString:          "update",
+			expectReadOnly:        false,
+			expectCanUpdate:       true,
+			expectCaseInsensitive: false,
+		},
+		{
+			name:                  "Visibility: nocase",
+			tag:                   reflect.StructTag("visibility:\"nocase\""),
+			expectString:          "nocase",
+			expectReadOnly:        false,
+			expectCanUpdate:       false,
+			expectCaseInsensitive: true,
+		},
+		{
+			name:                  "Visibility: (all)",
+			tag:                   reflect.StructTag("visibility:\"read create update nocase\""),
+			expectString:          "read create update nocase",
+			expectReadOnly:        false,
+			expectCanUpdate:       true,
+			expectCaseInsensitive: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			flags, _ := GetVisibilityFlags(tt.tag)
+			if flags.String() != tt.expectString {
+				t.Errorf("Expected flags.String() to be %q, got %q", tt.expectString, flags.String())
+			}
+			if flags.ReadOnly() != tt.expectReadOnly {
+				t.Errorf("Expected flags.ReadOnly() to be %v, got %v", tt.expectReadOnly, flags.ReadOnly())
+			}
+			if flags.CanUpdate() != tt.expectCanUpdate {
+				t.Errorf("Expected flags.CanUpdate() to be %v, got %v", tt.expectCanUpdate, flags.CanUpdate())
+			}
+			if flags.CaseInsensitive() != tt.expectCaseInsensitive {
+				t.Errorf("Expected flags.CaseInsensitive() to be %v, got %v", tt.expectCaseInsensitive, flags.CaseInsensitive())
+			}
+		})
+	}
+}
+
+type TestModelType struct {
+	// Subtype inherits default visibility.
+	A *TestModelSubtype
+
+	// Subtype inherits explicit visibility.
+	B *TestModelSubtype `visibility:"read"`
+
+	// Slice of simple type inherits visibility.
+	C []*string `visibility:"read"`
+
+	// Slice of struct type inherits visibility but can override.
+	D []*TestModelSubtype `visibility:"update nocase"`
+
+	// For equality checks of various reflect types.
+	E any `visibility:"read"`
+}
+
+type TestModelSubtype struct {
+	Implicit         *string
+	Read             *string `visibility:"read"`
+	ReadNoCase       *string `visibility:"read nocase"`
+	ReadCreate       *string `visibility:"read create"`
+	ReadCreateUpdate *string `visibility:"read create update"`
+}
+
+var (
+	TestModelTypeStructTagMap    = NewStructTagMap[TestModelType]()
+	TestModelSubtypeStructTagMap = NewStructTagMap[TestModelSubtype]()
+)
+
+func TestStructTagMap(t *testing.T) {
+	expectedStructTagMap := StructTagMap{
+		"A.Read":             reflect.StructTag("visibility:\"read\""),
+		"A.ReadNoCase":       reflect.StructTag("visibility:\"read nocase\""),
+		"A.ReadCreate":       reflect.StructTag("visibility:\"read create\""),
+		"A.ReadCreateUpdate": reflect.StructTag("visibility:\"read create update\""),
+		"B":                  reflect.StructTag("visibility:\"read\""),
+		"B.Read":             reflect.StructTag("visibility:\"read\""),
+		"B.ReadNoCase":       reflect.StructTag("visibility:\"read nocase\""),
+		"B.ReadCreate":       reflect.StructTag("visibility:\"read create\""),
+		"B.ReadCreateUpdate": reflect.StructTag("visibility:\"read create update\""),
+		"C":                  reflect.StructTag("visibility:\"read\""),
+		"D":                  reflect.StructTag("visibility:\"update nocase\""),
+		"D.Read":             reflect.StructTag("visibility:\"read\""),
+		"D.ReadNoCase":       reflect.StructTag("visibility:\"read nocase\""),
+		"D.ReadCreate":       reflect.StructTag("visibility:\"read create\""),
+		"D.ReadCreateUpdate": reflect.StructTag("visibility:\"read create update\""),
+		"E":                  reflect.StructTag("visibility:\"read\""),
+	}
+
+	// The test cases are encoded into the type itself.
+	if !cmp.Equal(TestModelTypeStructTagMap, expectedStructTagMap, nil) {
+		t.Errorf(
+			"StructTagMap had unexpected differences:\n%s",
+			cmp.Diff(expectedStructTagMap, TestModelTypeStructTagMap, nil))
+	}
+}
+
+func TestValidateVisibility(t *testing.T) {
+	testValues := TestModelSubtype{
+		Implicit:         Ptr("cherry"),
+		Read:             Ptr("strawberry"),
+		ReadNoCase:       Ptr("peach"),
+		ReadCreate:       Ptr("apple"),
+		ReadCreateUpdate: Ptr("melon"),
+	}
+
+	testImplicitVisibility := TestModelType{
+		A: &testValues,
+		B: &testValues,
+	}
+
+	tests := []struct {
+		name           string
+		v              any
+		w              any
+		m              StructTagMap
+		updating       bool
+		errorsExpected int
+	}{
+		{
+			// Required fields are out of scope for visibility tests.
+			name:           "Create: Empty structure is accepted",
+			v:              TestModelSubtype{},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set read-only field to same value is accepted",
+			v: TestModelSubtype{
+				Read: Ptr("strawberry"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set read-only field to same but differently cased value is rejected",
+			v: TestModelSubtype{
+				Read: Ptr("STRAWBERRY"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Create: Set read-only field to different value is rejected",
+			v: TestModelSubtype{
+				Read: Ptr("pretzel"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Create: Set case-insensitive read-only field to same value is accepted",
+			v: TestModelSubtype{
+				ReadNoCase: Ptr("peach"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set case-insensitive read-only field to same but differently cased value is accepted",
+			v: TestModelSubtype{
+				ReadNoCase: Ptr("PEACH"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set case-insensitive read-only field to different value is rejected",
+			v: TestModelSubtype{
+				ReadNoCase: Ptr("pretzel"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Create: Set read/create field to same value is accepted",
+			v: TestModelSubtype{
+				ReadCreate: Ptr("apple"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set read/create field to different value is accepted",
+			v: TestModelSubtype{
+				ReadCreate: Ptr("pear"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set read/create/update field to same value is accepted",
+			v: TestModelSubtype{
+				ReadCreateUpdate: Ptr("melon"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Create: Set read/create/update field to different value is accepted",
+			v: TestModelSubtype{
+				ReadCreateUpdate: Ptr("banana"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			// Required fields are out of scope for visibility tests.
+			name:           "Update: Empty structure is accepted",
+			v:              TestModelSubtype{},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Update: Set read-only field to same value is accepted",
+			v: TestModelSubtype{
+				Read: Ptr("strawberry"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Update: Set read-only field to same but differently cased value is rejected",
+			v: TestModelSubtype{
+				Read: Ptr("STRAWBERRY"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 1,
+		},
+		{
+			name: "Update: Set read-only field to different value is rejected",
+			v: TestModelSubtype{
+				Read: Ptr("pretzel"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 1,
+		},
+		{
+			name: "Update: Set case-insensitive read-only field to same value is accepted",
+			v: TestModelSubtype{
+				ReadNoCase: Ptr("peach"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Update: Set case-insensitive read-only field to same but differently cased value is accepted",
+			v: TestModelSubtype{
+				ReadNoCase: Ptr("PEACH"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Update: Set case-insensitive read-only field to different value is rejected",
+			v: TestModelSubtype{
+				ReadNoCase: Ptr("pretzel"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 1,
+		},
+		{
+			name: "Update: Set read/create field to same value is accepted",
+			v: TestModelSubtype{
+				ReadCreate: Ptr("apple"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Update: Set read/create field to different value is rejected",
+			v: TestModelSubtype{
+				ReadCreate: Ptr("pear"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 1,
+		},
+		{
+			name: "Update: Set read/create/update field to same value is accepted",
+			v: TestModelSubtype{
+				ReadCreateUpdate: Ptr("melon"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Update: Set read/create/update field to different value is accepted",
+			v: TestModelSubtype{
+				ReadCreateUpdate: Ptr("banana"),
+			},
+			w:              testValues,
+			m:              TestModelSubtypeStructTagMap,
+			updating:       true,
+			errorsExpected: 0,
+		},
+		{
+			name: "Set implicit read/create/update field to same value is accepted",
+			v: TestModelType{
+				A: &TestModelSubtype{
+					Implicit: Ptr("cherry"),
+				},
+			},
+			w:              testImplicitVisibility,
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Set implicit read/create/update field to different value is accepted",
+			v: TestModelType{
+				A: &TestModelSubtype{
+					Implicit: Ptr("bell"),
+				},
+			},
+			w:              testImplicitVisibility,
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Set implicit read-only field to same value is accepted",
+			v: TestModelType{
+				B: &TestModelSubtype{
+					Implicit: Ptr("cherry"),
+				},
+			},
+			w:              testImplicitVisibility,
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Set implicit read-only field to different value is rejected",
+			v: TestModelType{
+				B: &TestModelSubtype{
+					Implicit: Ptr("bell"),
+				},
+			},
+			w:              testImplicitVisibility,
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for bool type fields",
+			v: TestModelType{
+				E: Ptr(bool(true)),
+			},
+			w: TestModelType{
+				E: Ptr(bool(true)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for bool type fields",
+			v: TestModelType{
+				E: Ptr(bool(true)),
+			},
+			w: TestModelType{
+				E: Ptr(bool(false)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for int type fields",
+			v: TestModelType{
+				E: Ptr(int(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for int type fields",
+			v: TestModelType{
+				E: Ptr(int(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for int8 type fields",
+			v: TestModelType{
+				E: Ptr(int8(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int8(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for int8 type fields",
+			v: TestModelType{
+				E: Ptr(int8(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int8(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for int16 type fields",
+			v: TestModelType{
+				E: Ptr(int16(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int16(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for int16 type fields",
+			v: TestModelType{
+				E: Ptr(int16(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int16(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for int32 type fields",
+			v: TestModelType{
+				E: Ptr(int32(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int32(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for int32 type fields",
+			v: TestModelType{
+				E: Ptr(int32(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int32(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for int64 type fields",
+			v: TestModelType{
+				E: Ptr(int64(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int64(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for int64 type fields",
+			v: TestModelType{
+				E: Ptr(int64(1)),
+			},
+			w: TestModelType{
+				E: Ptr(int64(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for uint type fields",
+			v: TestModelType{
+				E: Ptr(uint(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for uint type fields",
+			v: TestModelType{
+				E: Ptr(uint(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for uintptr type fields",
+			v: TestModelType{
+				E: Ptr(uintptr(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uintptr(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for uintptr type fields",
+			v: TestModelType{
+				E: Ptr(uintptr(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uintptr(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for uint8 type fields",
+			v: TestModelType{
+				E: Ptr(uint8(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint8(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for uint8 type fields",
+			v: TestModelType{
+				E: Ptr(uint8(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint8(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for uint16 type fields",
+			v: TestModelType{
+				E: Ptr(uint16(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint16(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for uint16 type fields",
+			v: TestModelType{
+				E: Ptr(uint16(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint16(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for uint32 type fields",
+			v: TestModelType{
+				E: Ptr(uint32(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint32(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for uint32 type fields",
+			v: TestModelType{
+				E: Ptr(uint32(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint32(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for uint64 type fields",
+			v: TestModelType{
+				E: Ptr(uint64(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint64(1)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for uint64 type fields",
+			v: TestModelType{
+				E: Ptr(uint64(1)),
+			},
+			w: TestModelType{
+				E: Ptr(uint64(0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for float32 type fields",
+			v: TestModelType{
+				E: Ptr(float32(1.0)),
+			},
+			w: TestModelType{
+				E: Ptr(float32(1.0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for float32 type fields",
+			v: TestModelType{
+				E: Ptr(float32(1.0)),
+			},
+			w: TestModelType{
+				E: Ptr(float32(0.0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for float64 type fields",
+			v: TestModelType{
+				E: Ptr(float64(1.0)),
+			},
+			w: TestModelType{
+				E: Ptr(float64(1.0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for float64 type fields",
+			v: TestModelType{
+				E: Ptr(float64(1.0)),
+			},
+			w: TestModelType{
+				E: Ptr(float64(0.0)),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for complex64 type fields",
+			v: TestModelType{
+				E: Ptr(complex(float32(1.0), float32(-1.0))),
+			},
+			w: TestModelType{
+				E: Ptr(complex(float32(1.0), float32(-1.0))),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for complex64 type fields",
+			v: TestModelType{
+				E: Ptr(complex(float32(1.0), float32(-1.0))),
+			},
+			w: TestModelType{
+				E: Ptr(complex(float32(0.0), float32(1.0))),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for complex128 type fields",
+			v: TestModelType{
+				E: Ptr(complex(float64(1.0), float64(-1.0))),
+			},
+			w: TestModelType{
+				E: Ptr(complex(float64(1.0), float64(-1.0))),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality for complex128 type fields",
+			v: TestModelType{
+				E: Ptr(complex(float64(1.0), float64(-1.0))),
+			},
+			w: TestModelType{
+				E: Ptr(complex(float64(0.0), float64(1.0))),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test equality for slice fields",
+			v: TestModelType{
+				E: []int{1, 2, 3},
+			},
+			w: TestModelType{
+				E: []int{1, 2, 3},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality due to nil pointer for slice fields",
+			v: TestModelType{
+				E: []int{1, 2, 3},
+			},
+			w: TestModelType{
+				E: []int(nil),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test inequality due to length for slice fields",
+			v: TestModelType{
+				E: []int{1, 2, 3},
+			},
+			w: TestModelType{
+				E: []int{1},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test inequality due to content for slice fields",
+			v: TestModelType{
+				E: []int{3, 2, 1},
+			},
+			w: TestModelType{
+				E: []int{1, 2, 3},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 2, // error for each changed element
+		},
+		{
+			name: "Test equality for array fields",
+			v: TestModelType{
+				E: [3]int{1, 2, 3},
+			},
+			w: TestModelType{
+				E: [3]int{1, 2, 3},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality due for array fields",
+			v: TestModelType{
+				E: [3]int{3, 2, 1},
+			},
+			w: TestModelType{
+				E: [3]int{1, 2, 3},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 2, // error for each changed element
+		},
+		{
+			name: "Test equality for map fields",
+			v: TestModelType{
+				E: map[string]string{
+					"Blinky": "Shadow",
+					"Pinky":  "Speedy",
+					"Inky":   "Bashful",
+					"Clyde":  "Pokey",
+				},
+			},
+			w: TestModelType{
+				E: map[string]string{
+					"Blinky": "Shadow",
+					"Pinky":  "Speedy",
+					"Inky":   "Bashful",
+					"Clyde":  "Pokey",
+				},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 0,
+		},
+		{
+			name: "Test inequality due to nil pointer for map fields",
+			v: TestModelType{
+				E: map[string]string{
+					"Blinky": "Shadow",
+					"Pinky":  "Speedy",
+					"Inky":   "Bashful",
+					"Clyde":  "Pokey",
+				},
+			},
+			w: TestModelType{
+				E: map[string]string(nil),
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test inequality due to length for map fields",
+			v: TestModelType{
+				E: map[string]string{
+					"Blinky": "Shadow",
+					"Pinky":  "Speedy",
+					"Inky":   "Bashful",
+					"Clyde":  "Pokey",
+				},
+			},
+			w: TestModelType{
+				E: map[string]string{
+					"Blinky": "Shadow",
+				},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 1,
+		},
+		{
+			name: "Test inequality due to content for map fields",
+			v: TestModelType{
+				E: map[string]string{
+					"Akabei": "Oikake",
+					"Pinky":  "Machibuse",
+					"Aosuke": "Kimagure",
+					"Guzuta": "Otoboke",
+				},
+			},
+			w: TestModelType{
+				E: map[string]string{
+					"Blinky": "Shadow",
+					"Pinky":  "Speedy",
+					"Inky":   "Bashful",
+					"Clyde":  "Pokey",
+				},
+			},
+			m:              TestModelTypeStructTagMap,
+			updating:       false,
+			errorsExpected: 4, // error for each changed element
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cloudErrors := ValidateVisibility(tt.v, tt.w, tt.m, tt.updating)
+			if len(cloudErrors) != tt.errorsExpected {
+				t.Errorf("Expected %d errors, got %d: %v", tt.errorsExpected, len(cloudErrors), cloudErrors)
+			}
+		})
+	}
+}
diff --git a/internal/go.mod b/internal/go.mod
index b9577d5cb..ecca7b2f8 100644
--- a/internal/go.mod
+++ b/internal/go.mod
@@ -7,6 +7,7 @@ toolchain go1.22.2
 require (
 	github.com/Azure/azure-sdk-for-go/sdk/azcore v1.10.0
 	github.com/go-playground/validator/v10 v10.19.0
+	github.com/google/go-cmp v0.6.0
 	github.com/google/uuid v1.6.0
 	github.com/openshift/api v0.0.0-20240429104249-ac9356ba1784
 )

From 05e706016cca3c65ba4aefe6386b243a23e26836 Mon Sep 17 00:00:00 2001
From: Matthew Barnes <mbarnes@fedorapeople.org>
Date: Fri, 3 May 2024 08:51:12 -0400
Subject: [PATCH 2/3] api: Explain how to adapt to visibility changes in future
 APIs

---
 internal/api/v20240610preview/register.go | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/internal/api/v20240610preview/register.go b/internal/api/v20240610preview/register.go
index 50730abfe..6fe20cf03 100644
--- a/internal/api/v20240610preview/register.go
+++ b/internal/api/v20240610preview/register.go
@@ -31,6 +31,17 @@ func EnumValidateTag[S ~string](values ...S) string {
 }
 
 func init() {
+	// NOTE: If future versions of the API expand field visibility, such as
+	//       a field with @visibility("read","create") becoming updatable,
+	//       then earlier versions of the API will need to override their
+	//       StructTagMap to maintain the original visibility flags. This
+	//       is where such overrides should happen, along with a comment
+	//       about what changed and when. For example:
+	//
+	//       // This field became updatable in version YYYY-MM-DD.
+	//       clusterStructTagMap["Properties.Spec.FieldName"] = reflect.StructTag("visibility:\"read create\"")
+	//
+
 	api.Register(version{})
 
 	// Register enum type validations

From cf19516a48516f178d221d2ea5849ac58b6e9a04 Mon Sep 17 00:00:00 2001
From: Matthew Barnes <mbarnes@fedorapeople.org>
Date: Tue, 7 May 2024 12:18:08 -0400
Subject: [PATCH 3/3] api: Readability improvments to visibility.go

Too many single-letter variables make for poor readability, turns out.
---
 internal/api/visibility.go | 108 +++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 46 deletions(-)

diff --git a/internal/api/visibility.go b/internal/api/visibility.go
index b93f33307..0e381d59d 100644
--- a/internal/api/visibility.go
+++ b/internal/api/visibility.go
@@ -97,10 +97,10 @@ func join(ns, name string) string {
 
 type StructTagMap map[string]reflect.StructTag
 
-func buildStructTagMap(m StructTagMap, t reflect.Type, path string) {
+func buildStructTagMap(structTagMap StructTagMap, t reflect.Type, path string) {
 	switch t.Kind() {
 	case reflect.Pointer, reflect.Slice:
-		buildStructTagMap(m, t.Elem(), path)
+		buildStructTagMap(structTagMap, t.Elem(), path)
 
 	case reflect.Struct:
 		for i := 0; i < t.NumField(); i++ {
@@ -108,10 +108,10 @@ func buildStructTagMap(m StructTagMap, t reflect.Type, path string) {
 			subpath := join(path, field.Name)
 
 			if len(field.Tag) > 0 {
-				m[subpath] = field.Tag
+				structTagMap[subpath] = field.Tag
 			}
 
-			buildStructTagMap(m, field.Type, subpath)
+			buildStructTagMap(structTagMap, field.Type, subpath)
 		}
 	}
 }
@@ -124,85 +124,99 @@ func buildStructTagMap(m StructTagMap, t reflect.Type, path string) {
 // identical where visibility is explicitly specified. If some divergence
 // emerges, one workaround could be to pass a field name override map.
 func NewStructTagMap[T any]() StructTagMap {
-	m := StructTagMap{}
-	buildStructTagMap(m, reflect.TypeFor[T](), "")
-	return m
+	structTagMap := StructTagMap{}
+	buildStructTagMap(structTagMap, reflect.TypeFor[T](), "")
+	return structTagMap
 }
 
 type validateVisibility struct {
-	m        StructTagMap
-	updating bool
-	errs     []arm.CloudErrorBody
+	structTagMap StructTagMap
+	updating     bool
+	errs         []arm.CloudErrorBody
 }
 
-func ValidateVisibility(v, w interface{}, m StructTagMap, updating bool) []arm.CloudErrorBody {
+// ValidateVisibility compares the new value (newVal) to the current value
+// (curVal) and returns any violations of visibility restrictions as defined
+// by structTagMap.
+func ValidateVisibility(newVal, curVal interface{}, structTagMap StructTagMap, updating bool) []arm.CloudErrorBody {
 	vv := validateVisibility{
-		m:        m,
-		updating: updating,
+		structTagMap: structTagMap,
+		updating:     updating,
 	}
-	vv.recurse(reflect.ValueOf(v), reflect.ValueOf(w), "", "", "", VisibilityDefault)
+	vv.recurse(reflect.ValueOf(newVal), reflect.ValueOf(curVal), "", "", "", VisibilityDefault)
 	return vv.errs
 }
 
-func (vv *validateVisibility) recurse(v, w reflect.Value, mKey, namespace, fieldname string, implicitVisibility VisibilityFlags) {
-	flags, ok := GetVisibilityFlags(vv.m[mKey])
+// mapKey is a lookup key for the StructTagMap.  It DOES NOT include subscripts
+// for arrays, maps or slices since all elements are the same type.
+//
+// namespace is the struct field path up to but not including the field being
+// evaluated, analogous to path.Dir.  It DOES include subscripts for arrays,
+// maps and slices since its purpose is for error reporting.
+//
+// fieldname is the current field being evaluated, analgous to path.Base.  It
+// also includes subscripts for arrays, maps and slices when evaluating their
+// immediate elements.
+func (vv *validateVisibility) recurse(newVal, curVal reflect.Value, mapKey, namespace, fieldname string, implicitVisibility VisibilityFlags) {
+	flags, ok := GetVisibilityFlags(vv.structTagMap[mapKey])
 	if !ok {
 		flags = implicitVisibility
 	}
 
-	if v.Type() != w.Type() {
-		panic(fmt.Sprintf("%s: value types differ (%s vs %s)", join(namespace, fieldname), v.Type().Name(), w.Type().Name()))
+	if newVal.Type() != curVal.Type() {
+		panic(fmt.Sprintf("%s: value types differ (%s vs %s)", join(namespace, fieldname), newVal.Type().Name(), curVal.Type().Name()))
 	}
 
-	// Generated API structs are all pointer fields. A nil value in
-	// the incoming request (v) means the value is absent, which is
-	// always acceptable for visibility validation.
-	switch v.Kind() {
+	// Generated API structs are all pointer fields. A nil pointer in
+	// the incoming request (newVal) means the value is absent, which
+	// is always acceptable for visibility validation.
+	switch newVal.Kind() {
 	case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
-		if v.IsNil() {
+		if newVal.IsNil() {
 			return
 		}
 	}
 
-	switch v.Kind() {
+	switch newVal.Kind() {
 	case reflect.Bool:
-		if v.Bool() != w.Bool() {
+		if newVal.Bool() != curVal.Bool() {
 			vv.checkFlags(flags, namespace, fieldname)
 		}
 
 	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-		if v.Int() != w.Int() {
+		if newVal.Int() != curVal.Int() {
 			vv.checkFlags(flags, namespace, fieldname)
 		}
 
 	case reflect.Uint, reflect.Uintptr, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
-		if v.Uint() != w.Uint() {
+		if newVal.Uint() != curVal.Uint() {
 			vv.checkFlags(flags, namespace, fieldname)
 		}
 
 	case reflect.Float32, reflect.Float64:
-		if v.Float() != w.Float() {
+		if newVal.Float() != curVal.Float() {
 			vv.checkFlags(flags, namespace, fieldname)
 		}
 
 	case reflect.Complex64, reflect.Complex128:
-		if v.Complex() != w.Complex() {
+		if newVal.Complex() != curVal.Complex() {
 			vv.checkFlags(flags, namespace, fieldname)
 		}
 
 	case reflect.String:
 		if flags.CaseInsensitive() {
-			if !strings.EqualFold(v.String(), w.String()) {
+			if !strings.EqualFold(newVal.String(), curVal.String()) {
 				vv.checkFlags(flags, namespace, fieldname)
 			}
 		} else {
-			if v.String() != w.String() {
+			if newVal.String() != curVal.String() {
 				vv.checkFlags(flags, namespace, fieldname)
 			}
 		}
 
 	case reflect.Slice:
-		if w.IsNil() {
+		// We already know that newVal is not nil.
+		if curVal.IsNil() {
 			vv.checkFlags(flags, namespace, fieldname)
 			return
 		}
@@ -210,33 +224,35 @@ func (vv *validateVisibility) recurse(v, w reflect.Value, mKey, namespace, field
 		fallthrough
 
 	case reflect.Array:
-		if v.Len() != w.Len() {
+		if newVal.Len() != curVal.Len() {
 			vv.checkFlags(flags, namespace, fieldname)
 		} else {
-			for i := 0; i < min(v.Len(), w.Len()); i++ {
+			for i := 0; i < min(newVal.Len(), curVal.Len()); i++ {
 				subscript := fmt.Sprintf("[%d]", i)
-				vv.recurse(v.Index(i), w.Index(i), mKey, namespace, fieldname+subscript, flags)
+				vv.recurse(newVal.Index(i), curVal.Index(i), mapKey, namespace, fieldname+subscript, flags)
 			}
 		}
 
 	case reflect.Interface, reflect.Pointer:
-		if w.IsNil() {
+		// We already know that newVal is not nil.
+		if curVal.IsNil() {
 			vv.checkFlags(flags, namespace, fieldname)
 		} else {
-			vv.recurse(v.Elem(), w.Elem(), mKey, namespace, fieldname, flags)
+			vv.recurse(newVal.Elem(), curVal.Elem(), mapKey, namespace, fieldname, flags)
 		}
 
 	case reflect.Map:
-		if w.IsNil() || v.Len() != w.Len() {
+		// We already know that newVal is not nil.
+		if curVal.IsNil() || newVal.Len() != curVal.Len() {
 			vv.checkFlags(flags, namespace, fieldname)
 		} else {
-			iter := v.MapRange()
+			iter := newVal.MapRange()
 			for iter.Next() {
 				k := iter.Key()
 
 				subscript := fmt.Sprintf("[%q]", k.Interface())
-				if w.MapIndex(k).IsValid() {
-					vv.recurse(v.MapIndex(k), w.MapIndex(k), mKey, namespace, fieldname+subscript, flags)
+				if curVal.MapIndex(k).IsValid() {
+					vv.recurse(newVal.MapIndex(k), curVal.MapIndex(k), mapKey, namespace, fieldname+subscript, flags)
 				} else {
 					vv.checkFlags(flags, namespace, fieldname+subscript)
 				}
@@ -244,15 +260,15 @@ func (vv *validateVisibility) recurse(v, w reflect.Value, mKey, namespace, field
 		}
 
 	case reflect.Struct:
-		for i := 0; i < v.NumField(); i++ {
-			structField := v.Type().Field(i)
-			mKeyNext := join(mKey, structField.Name)
+		for i := 0; i < newVal.NumField(); i++ {
+			structField := newVal.Type().Field(i)
+			mapKeyNext := join(mapKey, structField.Name)
 			namespaceNext := join(namespace, fieldname)
-			fieldnameNext := GetJSONTagName(vv.m[mKeyNext])
+			fieldnameNext := GetJSONTagName(vv.structTagMap[mapKeyNext])
 			if fieldnameNext == "" {
 				fieldnameNext = structField.Name
 			}
-			vv.recurse(v.Field(i), w.Field(i), mKeyNext, namespaceNext, fieldnameNext, flags)
+			vv.recurse(newVal.Field(i), curVal.Field(i), mapKeyNext, namespaceNext, fieldnameNext, flags)
 		}
 	}
 }