Port: Enforce EDM model boundary for open-type property binding in $apply groupby - #2903
Open
WanjohiSammy wants to merge 3 commits into
Open
Port: Enforce EDM model boundary for open-type property binding in $apply groupby#2903WanjohiSammy wants to merge 3 commits into
$apply groupby#2903WanjohiSammy wants to merge 3 commits into
Conversation
WanjohiSammy
force-pushed
the
fix/apply-open-type-model-binding
branch
from
July 20, 2026 13:26
087a13a to
31e1f42
Compare
xuzhg
previously approved these changes
Jul 20, 2026
There was a problem hiding this comment.
Pull request overview
This PR ports a fix to ensure $apply=groupby(...) open-type property binding respects the EDM model boundary, preventing $apply from reflecting and binding to public CLR properties that were intentionally excluded from the model (e.g., via [NotMapped]), which could otherwise leak into grouping results.
Changes:
- Updated transformation binding to map EDM property names to CLR properties only when the property is declared in the EDM model; otherwise bind via the dynamic property container.
- Added unit + E2E coverage validating that unmodeled CLR members on open types do not participate in
$apply groupbybinding/results. - Added a guard in dynamic-property-container resolution to avoid null dereferences for nested open paths and surface a cleaner query error.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/DataModel.cs | Adds an open-type test model with an EDM-excluded CLR property via [NotMapped]. |
| test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/AggregationBinderTests.cs | Adds a unit test ensuring groupby binds excluded members via the dynamic container rather than CLR reflection. |
| test/E2ETest/Microsoft.Test.E2E.AspNet.OData/Aggregation/AggregationTests.cs | Adds an E2E test validating excluded CLR values don’t show up in $apply=groupby results on open types. |
| test/E2ETest/Microsoft.Test.E2E.AspNet.OData/Aggregation/AggregationDataModel.cs | Introduces an open-type entity with a [NotMapped] CLR member used by the E2E scenario. |
| test/E2ETest/Microsoft.Test.E2E.AspNet.OData/Aggregation/AggregationController.cs | Adds a controller endpoint serving the open-type data for the new E2E test. |
| src/Microsoft.AspNet.OData.Shared/Query/Expressions/TransformationBinderBase.cs | Enforces EDM-boundary property resolution for open-property access during transformation binding. |
| src/Microsoft.AspNet.OData.Shared/Query/Expressions/ExpressionBinderBase.cs | Adds a null guard for open-property sources lacking type references, throwing a cleaner query error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Issues
This pull request fixes #xxx.
Description
Ported from: OData/AspNetCoreOData@
5e3adee4When an
$apply groupbypath crosses an open/dynamic segment, the transformation binder resolved the trailing open-property segment with raw reflection (Type.GetProperty(name)) using the EDM/URI name, binding to any matching public CLR property without consulting the model. A CLR property excluded from the model ([NotMapped]/Ignore()/ simply unmodeled) was still resolvable, so its value leaked into groupby results.Fix (
TransformationBinderBase.CreateOpenPropertyAccessExpression). Resolve the open property from the runtime type's dynamic-property container when the name is not an EDM-declared structural property, instead of falling back to CLR reflection. Declared properties bind as before. The dynamic-container access is null-guarded only whenQuerySettings.HandleNullPropagationis enabled (the in-memory LINQ-to-Objects path usesHandleNullPropagation.Default → True, so it is guarded).Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.