Fix $ref replaced with inline schema copy (Issue #198)

[avgalex]

Summary

• Fixes 7.1.2 replaces schema reference with copy of schema #198: SetValidator with nested object types (e.g. Address) caused $ref to be replaced with inline schema copy in the OpenAPI document
• Root cause: ResolveRefProperty (introduced in Add BigInteger support for min/max restrictions (#146) #196 for BigInteger isolation) replaced all $ref properties with copies, destroying reference structure
• Fix: snapshot $ref properties before rule application, restore them afterwards if no validation constraints (min/max, minLength, pattern, etc.) were added by rules
• BigInteger per-model constraints (Issue Minimum and maximum restrictions for BigInteger #146) continue to work because those rules DO modify the copy, so the $ref is not restored

Test plan

• Added SetValidator_Should_Preserve_Ref_For_Nested_Object test reproducing Issue 7.1.2 replaces schema reference with copy of schema #198
• All existing tests pass on net8.0 (81), net9.0 (81+5), net10.0 (67)
• BigInteger tests (SharedRef_Should_Not_Corrupt_Between_Models, BigInteger_InclusiveBetween_Should_Set_MinMax) pass on net10.0

🤖 Generated with Claude Code

[avgalex]

@claude review this PR

[claude]

Claude finished @avgalex's task in 2m 39s —— View job

---

PR Review

• Gather context and understand the PR
• Read changed files
• Analyze fix correctness and edge cases
• Check test coverage
• Post review feedback

---

Summary

This PR correctly identifies and fixes the root cause of Issue #198. The snapshot-restore approach is clean and minimally invasive. The fix works well for the primary use case.

---

Correctness Analysis

Root cause identification is accurate. ResolveRefProperty (from #196) unconditionally replaced all $ref entries with shallow copies — even for nested object-type properties like Address that had no per-instance constraints to apply.

The fix logic is sound:

1. SnapshotRefs captures all $ref properties before rules run
2. Rules run and may replace some $refs with inline copies (via ResolveRefProperty)
3. RestoreUnmodifiedRefs compares each copy against its component schema and restores the $ref if nothing meaningful changed

BigInteger regression safety: The SharedRef_Should_Not_Corrupt_Between_Models test verifies that models with actual numeric constraints (which DO appear in HasValidationConstraintChanges) still get isolated copies. ✓

---

Issues Found

1. HasValidationConstraintChanges is incomplete — missing Format and AllOf

The DefaultFluentValidationRuleProvider has rules that can modify properties beyond what's checked:

• EMail rule sets context.Property.Format = "email" — not checked in HasValidationConstraintChanges
• Pattern rule with UseAllOfForMultipleRules appends to AllOf — not checked

If a custom type is registered as a $ref and an EMail or multi-pattern rule is applied to one of its property refs, the restoration would silently discard the Format or AllOf change.

// DefaultFluentValidationRuleProvider.cs:124 — not detected by HasValidationConstraintChanges
yield return new FluentValidationRule("EMail")
    .WithApply(context => context.Property.Format = "email");

Fix this →

2. Nullable is not checked — but this is probably correct

The Required rule calls SetNotNullable(context.Property) which sets Nullable = false on the inline copy. This change is not detected by HasValidationConstraintChanges, so the $ref is restored even though Nullable was modified.

However, in OpenAPI 3.0, sibling keywords alongside $ref are ignored, so setting Nullable = false on a $ref property is semantically invalid and meaningless. Restoring the $ref (without the Nullable change) is arguably the more correct behavior here. The test case exercises this scenario (NotEmpty().SetValidator(...)) and the test passes because the parent's Required array correctly captures the "non-null" intent. This is acceptable.

3. Null vs empty Required edge case

// OpenApiSchemaCompatibility.cs:361
if (copyReq?.Count != origReq?.Count) return true;

If copyReq is null and origReq is an empty ISet<string>, this evaluates to null != 0 → true (detected as changed). In practice this shouldn't occur since the copy is a shallow clone of the original, but it's a theoretical false positive.

---

Test Coverage

✅ SetValidator_Should_Preserve_Ref_For_Nested_Object — covers the primary regression
✅ SharedRef_Should_Not_Corrupt_Between_Models — covers BigInteger isolation regression
✅ BigInteger_InclusiveBetween_Should_Set_MinMax — covers numeric constraint application

Missing test: a scenario where a $ref property does have a constraint applied (e.g., Email) to verify the $ref is not restored in that case. This would strengthen confidence in the restoration gate.

---

Minor Observations

• The #if OPENAPI_V2 guards are consistent — ResolveRefProperty only exists in that branch, so the snapshot/restore is correctly scoped.
• SnapshotRefs uses a lazy-initialized dictionary (??=) which is a nice optimization for the common case where there are no $ref properties.
• XML doc cross-references (<see cref="RestoreUnmodifiedRefs"/>) are helpful for navigating the paired API.

---

Verdict

The fix is correct and safe for the reported issue. The primary concern is the incomplete HasValidationConstraintChanges check for Format and AllOf, which could cause silent data loss in edge cases involving custom $ref types with email or multi-pattern validators. I'd recommend adding those checks before merging.

[avgalex]

@claude check this commit 6d26a4c Add Format and AllOf checks to HasValidationConstraintChanges

[claude]

Claude finished @avgalex's task in 51s —— View job

---

Review: commit 6d26a4c — Add Format and AllOf checks to HasValidationConstraintChanges

• Read the commit diff
• Analyze the Format and AllOf checks
• Check test coverage

---

Summary

This commit correctly addresses both issues raised in the previous review. The implementation is clean and well-targeted.

---

Format check (line 357)

if (copy.Format != original.Format) return true;

Correct. Placed alongside the other scalar property checks. Handles the EMail rule that sets Format = "email". A simple string equality check is appropriate here.

---

AllOf check (lines 365–368)

// Check AllOf collection changes (Pattern rule with UseAllOfForMultipleRules)
var copyAllOfCount = copy.AllOf?.Count ?? 0;
var origAllOfCount = original.AllOf?.Count ?? 0;
if (copyAllOfCount != origAllOfCount) return true;

Correct for the primary case. Since UseAllOfForMultipleRules only appends to AllOf, a count difference is sufficient to detect that change. One theoretical edge case: if a rule removed an AllOf entry and added another (net-zero count change with different contents), the check would miss it — but no built-in rule does this, so this is acceptable pragmatism.

---

New test: Ref_Property_With_Constraint_Should_Not_Be_Restored

The test verifies that EmailAddress() on a plain string property results in Format = "email" being preserved. This is a good positive-path test confirming the gate works correctly.

One observation: ContactEmail is a plain string, which typically does not generate a $ref in the first place — $ref entries arise for complex object types like Address. So this test may be verifying the EMail rule works in general rather than specifically testing the "do not restore when Format changed" gate. The gate for Format is still exercised when a property of a custom registered type (with its own schema entry) has an email validator applied to it, but that scenario is harder to construct.

That said, the test adds value as a regression guard and the logic path through HasValidationConstraintChanges is correct regardless.

---

Verdict

Both additions are correct. The Format check closes the EMail rule gap, and the AllOf count check closes the multi-pattern gap. The new test provides a useful regression guard. The implementation is ready to merge.