Conversation
Contributor
Reviewer's GuideRefactors complex property expression building in LambdaExtensions to be null-safe for navigation properties and support dynamic objects while simplifying the navigation traversal logic. Class diagram for updated LambdaExtensions complex property handlingclassDiagram
class LambdaExtensions {
+Expression~Func~TModel, TResult~~ GetComplexPropertyExpression~TModel, TResult~()
-static Expression BuildPropertyAccess(Expression instance, Type instanceType, string propertyName)
}
class Expression
class Type
class IDynamicMetaObjectProvider
class RuntimeBinder
class CSharpArgumentInfo
LambdaExtensions ..> Expression : uses
LambdaExtensions ..> Type : uses
LambdaExtensions ..> IDynamicMetaObjectProvider : checks assignable
LambdaExtensions ..> RuntimeBinder : dynamic member access
LambdaExtensions ..> CSharpArgumentInfo : dynamic binder args
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
BuildPropertyAccessnull-guard logic assumes reference types: comparinginstancetoExpression.Constant(null, instanceType)and returningExpression.Constant(null, p.PropertyType)will throw or be invalid for non-nullable value types; consider restricting this path to reference/nullable types or usingtypeof(object)for the null constant and handling value types explicitly. - In
GetComplexPropertyExpression, each iteration passesbody(which becomes a conditional expression) as theinstancetoBuildPropertyAccess, but the conditional result may be a value type orobjectrather than the actual declaring type of the next property, so property chaining will fail for deeper navigation; you may need to track the actual runtime type (e.g., via reflection) instead of relying onbody.Typeafter each step. - The dynamic-member branch in
BuildPropertyAccessusesinstanceType.IsAssignableTo(typeof(IDynamicMetaObjectProvider)), but after the first dynamic accessinstanceTypewill likely beobject, so subsequent navigation on dynamic values will not take the dynamic path; consider carrying the original dynamic type or checking the expression node for being dynamic rather than relying solely oninstanceType.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `BuildPropertyAccess` null-guard logic assumes reference types: comparing `instance` to `Expression.Constant(null, instanceType)` and returning `Expression.Constant(null, p.PropertyType)` will throw or be invalid for non-nullable value types; consider restricting this path to reference/nullable types or using `typeof(object)` for the null constant and handling value types explicitly.
- In `GetComplexPropertyExpression`, each iteration passes `body` (which becomes a conditional expression) as the `instance` to `BuildPropertyAccess`, but the conditional result may be a value type or `object` rather than the actual declaring type of the next property, so property chaining will fail for deeper navigation; you may need to track the actual runtime type (e.g., via reflection) instead of relying on `body.Type` after each step.
- The dynamic-member branch in `BuildPropertyAccess` uses `instanceType.IsAssignableTo(typeof(IDynamicMetaObjectProvider))`, but after the first dynamic access `instanceType` will likely be `object`, so subsequent navigation on dynamic values will not take the dynamic path; consider carrying the original dynamic type or checking the expression node for being dynamic rather than relying solely on `instanceType`.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Extensions/LambdaExtensions.cs:681-682` </location>
<code_context>
+ {
+ var propertyAccess = Expression.Property(instance, p);
+ return Expression.Condition(
+ test: Expression.Equal(instance, Expression.Constant(null, instanceType)),
+ ifTrue: Expression.Constant(null, p.PropertyType),
+ ifFalse: propertyAccess
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** Null checks and null constants will break when `instanceType` or `p.PropertyType` are non-nullable value types.
This logic only works when both `instance` and `p.PropertyType` are nullable/comparable to `null`. For a non-nullable struct `instanceType`, `Expression.Equal(instance, Constant(null, instanceType))` will throw, and `Expression.Constant(null, p.PropertyType)` is also invalid for non-nullable value types. Please either restrict this path to reference types or perform the comparison via `object`/nullable types and then convert back to the target type.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Extensions/LambdaExtensions.cs:686-693` </location>
<code_context>
+ ifFalse: propertyAccess
+ );
+ }
+ else if (instanceType.IsAssignableTo(typeof(IDynamicMetaObjectProvider)))
+ {
+ var binder = Microsoft.CSharp.RuntimeBinder.Binder.GetMember(
+ CSharpBinderFlags.None,
+ propertyName,
+ instanceType,
+ new[] { CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null) });
+ return Expression.Dynamic(binder, typeof(object), instance);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Dynamic member access support is limited to the first segment in a nested property path.
Because the dynamic branch returns an `Expression` of type `object`, the next iteration sees `instanceType == typeof(object)`, which no longer matches `IDynamicMetaObjectProvider`. Any subsequent segment after a dynamic one (e.g., `dynamicObj.Prop1.Prop2`) will therefore drop into the reflection path and fail. If chained dynamic access is needed, you’ll need to either carry forward the original dynamic type for `instanceType` or add a dedicated handling path for `instanceType == typeof(object)` that continues using dynamic binders.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Extensions/LambdaExtensions.cs:674` </location>
<code_context>
}
}
+ private static Expression BuildPropertyAccess(Expression instance, Type instanceType, string propertyName)
+ {
+ var p = instanceType.GetPropertyByName(propertyName);
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting property access, null-safety, and dynamic handling into separate helpers so the complex property expression is built through a clearer, linear chain.
You can keep the new features (null-safety and dynamic support) while reducing complexity by splitting responsibilities and making the chain construction more linear.
### 1. Split `BuildPropertyAccess` into focused helpers
Right now `BuildPropertyAccess` handles:
- static reflection
- null checks
- dynamic binder
You can isolate these concerns:
```csharp
private static Expression BuildPropertyAccessCore(Expression instance, Type instanceType, string propertyName)
{
var p = instanceType.GetPropertyByName(propertyName);
if (p != null)
{
// pure static property access
return Expression.Property(instance, p);
}
if (typeof(IDynamicMetaObjectProvider).IsAssignableFrom(instanceType))
{
return BuildDynamicPropertyAccess(instance, instanceType, propertyName);
}
throw new InvalidOperationException($"类型 {instanceType.Name} 未找到 {propertyName} 属性，无法获取其值");
}
private static Expression BuildDynamicPropertyAccess(Expression instance, Type instanceType, string propertyName)
{
var binder = Microsoft.CSharp.RuntimeBinder.Binder.GetMember(
CSharpBinderFlags.None,
propertyName,
instanceType,
new[] { CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null) });
return Expression.Dynamic(binder, typeof(object), instance);
}
```
### 2. Move null-safety into a small wrapper
Instead of mixing null checks into property resolution, add a single-purpose helper that wraps any access expression with a per-step null guard:
```csharp
private static Expression BuildNullSafeAccess(Expression instance, Func<Expression, Expression> accessFactory)
{
var access = accessFactory(instance);
return Expression.Condition(
test: Expression.Equal(instance, Expression.Constant(null, instance.Type)),
ifTrue: Expression.Constant(null, access.Type),
ifFalse: access);
}
```
### 3. Make `GetComplexPropertyExpression` linear and easy to follow
Now `GetComplexPropertyExpression` reads as “build a chain, applying null-safe access at each step” with the actual concerns separated:
```csharp
Expression<Func<TModel, TResult>> GetComplexPropertyExpression()
{
var propertyNames = propertyName.Split(".");
Expression body = Expression.Convert(parameter, type);
foreach (var name in propertyNames)
{
body = BuildNullSafeAccess(
body,
instance => BuildPropertyAccessCore(instance, instance.Type, name));
}
return Expression.Lambda<Func<TModel, TResult>>(
Expression.Convert(body, typeof(TResult)),
parameter);
}
```
This keeps:
- per-segment null-propagation
- dynamic member support
- existing functionality
while reducing cognitive load by:
- separating static vs dynamic access
- separating null-safety from member resolution
- keeping the property-chain construction readable and linear.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7296 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 745
Lines 32628 32629 +1
Branches 4522 4520 -2
=========================================
+ Hits 32628 32629 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds null navigation property support for Table components by refactoring the complex property expression building logic to handle null intermediate properties gracefully. The change addresses issue #7293 where accessing nested properties (e.g., Foo.Name) would throw NullReferenceException when intermediate properties were null.
Key Changes:
- Refactored
GetComplexPropertyExpression()to use a new helper method for property access - Introduced
BuildPropertyAccess()method that wraps property access with null-checking conditional expressions - Version bumped to 10.1.4-beta01
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/BootstrapBlazor/Extensions/LambdaExtensions.cs | Refactored complex property expression building to add null-safe navigation; extracted property access logic into BuildPropertyAccess helper method |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bumped to 10.1.4-beta01 for beta release |
Comments suppressed due to low confidence (2)
src/BootstrapBlazor/Extensions/LambdaExtensions.cs:697
- The new BuildPropertyAccess method lacks test coverage for null navigation properties. While existing tests in LambadaExtensionsTest.cs cover GetPropertyValueLambda with nested properties (e.g., "Foo.Name"), there are no tests that verify behavior when intermediate properties are null (e.g., when Foo is null in "Foo.Name"). Tests should be added to verify that null navigation properties return null instead of throwing NullReferenceException.
private static ConditionalExpression BuildPropertyAccess(Expression instance, Type instanceType, string propertyName)
{
var p = instanceType.GetPropertyByName(propertyName) ?? throw new InvalidOperationException($"类型 {instanceType.Name} 未找到 {propertyName} 属性，无法获取其值");
var propertyAccess = Expression.Property(instance, p);
return Expression.Condition(
test: Expression.Equal(instance, Expression.Constant(null, instanceType)),
ifTrue: Expression.Constant(null, p.PropertyType),
ifFalse: propertyAccess
);
}
/// <summary>
/// 给指定模型属性赋值 Lambda 表达式
/// </summary>
/// <typeparam name="TModel"></typeparam>
/// <typeparam name="TValue"></typeparam>
/// <param name="model"></param>
/// <param name="propertyName"></param>
/// <returns></returns>
public static Expression<Action<TModel, TValue>> SetPropertyValueLambda<TModel, TValue>(TModel model, string propertyName)
{
if (model == null)
{
src/BootstrapBlazor/Extensions/LambdaExtensions.cs:693
- Dynamic property access doesn't include null checking like the static property case. If the instance is null, the dynamic invocation will throw a NullReferenceException at runtime. Consider wrapping the dynamic expression with a similar null-check conditional to provide consistent behavior between static and dynamic property access.
/// <summary>
/// 给指定模型属性赋值 Lambda 表达式
/// </summary>
/// <typeparam name="TModel"></typeparam>
/// <typeparam name="TValue"></typeparam>
/// <param name="model"></param>
/// <param name="propertyName"></param>
/// <returns></returns>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+674
to
+683
| private static ConditionalExpression BuildPropertyAccess(Expression instance, Type instanceType, string propertyName) | ||
| { | ||
| var p = instanceType.GetPropertyByName(propertyName) ?? throw new InvalidOperationException($"类型 {instanceType.Name} 未找到 {propertyName} 属性，无法获取其值"); | ||
|
|
||
| var propertyAccess = Expression.Property(instance, p); | ||
| return Expression.Condition( | ||
| test: Expression.Equal(instance, Expression.Constant(null, instanceType)), | ||
| ifTrue: Expression.Constant(null, p.PropertyType), | ||
| ifFalse: propertyAccess | ||
| ); |
There was a problem hiding this comment.
The null-checking logic has several issues: (1) Expression.Equal cannot properly handle value types - if instanceType is a value type (struct), comparing it to null will cause compilation errors. (2) Expression.Constant(null, p.PropertyType) will throw ArgumentException if p.PropertyType is a non-nullable value type. The code should check if instanceType is a reference type or nullable value type before adding the null check, and ensure the return type is properly nullable.
Suggested change
| private static ConditionalExpression BuildPropertyAccess(Expression instance, Type instanceType, string propertyName) | |
| { | |
| var p = instanceType.GetPropertyByName(propertyName) ?? throw new InvalidOperationException($"类型 {instanceType.Name} 未找到 {propertyName} 属性，无法获取其值"); | |
| var propertyAccess = Expression.Property(instance, p); | |
| return Expression.Condition( | |
| test: Expression.Equal(instance, Expression.Constant(null, instanceType)), | |
| ifTrue: Expression.Constant(null, p.PropertyType), | |
| ifFalse: propertyAccess | |
| ); | |
| private static Expression BuildPropertyAccess(Expression instance, Type instanceType, string propertyName) | |
| { | |
| var p = instanceType.GetPropertyByName(propertyName) ?? throw new InvalidOperationException($"类型 {instanceType.Name} 未找到 {propertyName} 属性，无法获取其值"); | |
| var propertyAccess = Expression.Property(instance, p); | |
| // Only add null check if instanceType is a reference type or nullable value type | |
| if (!instanceType.IsValueType || Nullable.GetUnderlyingType(instanceType) != null) | |
| { | |
| // Only use null constant if property type is reference or nullable value type | |
| if (!p.PropertyType.IsValueType || Nullable.GetUnderlyingType(p.PropertyType) != null) | |
| { | |
| return Expression.Condition( | |
| test: Expression.Equal(instance, Expression.Constant(null, instanceType)), | |
| ifTrue: Expression.Constant(null, p.PropertyType), | |
| ifFalse: propertyAccess | |
| ); | |
| } | |
| else | |
| { | |
| return Expression.Condition( | |
| test: Expression.Equal(instance, Expression.Constant(null, instanceType)), | |
| ifTrue: Expression.Default(p.PropertyType), | |
| ifFalse: propertyAccess | |
| ); | |
| } | |
| } | |
| else | |
| { | |
| // For non-nullable value types, just return property access | |
| return propertyAccess; | |
| } |
13 tasks
This was referenced Dec 22, 2025
This was referenced Jan 7, 2026
This was referenced Jan 21, 2026
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.
Link issues
fixes #7293
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enhancements: