Repository navigation
Throw for change-tracking proxy properties whose setters cannot be overridden - #39592
Draft
ajcvickers wants to merge 2 commits into
Draft
ajcvickers wants to merge 2 commits into
ajcvickers wants to merge 2 commits into
Conversation
Castle proxies cannot override members that are not visible to
DynamicProxyGenAssembly2, such as internal virtual navigations, unless the
declaring assembly has [InternalsVisibleTo("DynamicProxyGenAssembly2")].
Such navigations passed model validation but were silently never
lazy-loaded or change-tracked.
Model validation now throws when the getter (lazy-loading proxies) or the
setter (change-tracking proxies) of a navigation cannot be overridden,
using Castle's own accessibility rules. IgnoreNonVirtualNavigations and
EnableLazyLoading(false) still suppress the check for lazy-loading.
Entity types that are themselves inaccessible are not reported here, since
they already fail when a proxy is created.
Fixes dotnet#32352
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Change-tracking proxies must override the setter of every mapped property
to receive change notifications. A property whose setter is not visible to
DynamicProxyGenAssembly2, such as `public virtual string Name { get; internal set; }`,
passed model validation but was silently never tracked, so changes to it
were not detected or saved.
Model validation now throws when the setter of a mapped scalar property or
of the mapped indexer property cannot be overridden by the proxy, using the
same Castle accessibility rules as for navigations. Key properties are
validated like any other property. Lazy-loading proxies are unaffected.
Fixes dotnet#39589
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #39589
Change-tracking proxies must override the setter of every mapped property to receive change notifications. A property whose setter is not visible to
DynamicProxyGenAssembly2, such aspublic virtual string Name { get; internal set; }, passed model validation but was silently never tracked, so changes to it were not detected or saved. #39591 fixes the same problem for navigations.Change
Model validation (
ProxyBindingRewriter) now throwsInvalidOperationExceptionwhenUseChangeTrackingProxiesis used and the setter ofcannot be overridden by the proxy. It uses the same Castle accessibility rules as #39591 (
ProxyUtil.IsAccessible), soInternalsVisibleTo("DynamicProxyGenAssembly2")is honored. The scalar message is the one added in #39591 and names the property; the indexer has a new message, since the CLR indexer has no useful name.virtualcheck.Tests
ProxyAccessibilityTests(emitted types): scalar properties and the indexer at every accessibility level, with and withoutInternalsVisibleTo; lazy-loading does not validate them; and an end-to-end test that a visibleinternalsetter is intercepted, so the entity becomesModifiedwithoutDetectChanges.ChangeDetectionProxyTests(plain C# classes):{ get; internal set; }on a property and on a key throws, and{ get; protected set; }is accepted.Verified with the Proxies tests (160), the InMemory, Sqlite and SQL Server proxy functional tests, and the AspNet identity and complex-type tracking tests. All pass.
Breaking change
Apps with
public virtual T Prop { get; internal set; }(including keys) under change-tracking proxies, withoutInternalsVisibleTo("DynamicProxyGenAssembly2"), will now get a model-validation error instead of changes being silently lost. The docs breaking-change note (dotnet/EntityFramework.Docs#5515) is being extended to cover this.Not in this PR
Entity types that are abstract are skipped by all of the rewriter's checks, so an abstract base with an invisible setter is still not validated. This is an existing gap that also applies to navigations and the
virtualcheck.🤖 Generated with Claude Code