-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow using property-paths with respect to declaring type from model. #2734
base: master
Are you sure you want to change the base?
Allow using property-paths with respect to declaring type from model. #2734
Conversation
Solves property path expressions like Stamp/CreatedByUser/Name, where CreatedByUser is interface (e.g. IMyInterface) which do not directly have a Name property, but the Name is in some base interface up in the hierarchy. Otherwise it internally throws System.ArgumentException: 'Instance property 'Name' is not defined for type 'IMyInterface'. It is possible to overcome the issue using a cast expression, but this forces to use hardcoded casts on client calling odata endpoints and have deeper knowledge about model behind it.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Add tests |
var propertyDeclaringClrType = EdmLibHelpers.GetClrType(property.DeclaringType, model); | ||
if (propertyDeclaringClrType != null) | ||
propertyValue = Expression.Property(propertyValue, propertyDeclaringClrType, propertyNameParts[i]); | ||
else | ||
propertyValue = Expression.Property(propertyValue, propertyNameParts[i]); | ||
} | ||
else | ||
propertyValue = Expression.Property(propertyValue, propertyNameParts[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions to meet library's coding conventions
var propertyDeclaringClrType = EdmLibHelpers.GetClrType(property.DeclaringType, model); | |
if (propertyDeclaringClrType != null) | |
propertyValue = Expression.Property(propertyValue, propertyDeclaringClrType, propertyNameParts[i]); | |
else | |
propertyValue = Expression.Property(propertyValue, propertyNameParts[i]); | |
} | |
else | |
propertyValue = Expression.Property(propertyValue, propertyNameParts[i]); | |
Type propertyDeclaringClrType = EdmLibHelpers.GetClrType(property.DeclaringType, model); | |
if (propertyDeclaringClrType != null) | |
{ | |
propertyValue = Expression.Property(propertyValue, propertyDeclaringClrType, propertyNameParts[i]); | |
} | |
else | |
{ | |
propertyValue = Expression.Property(propertyValue, propertyNameParts[i]); | |
} | |
} | |
else | |
{ | |
propertyValue = Expression.Property(propertyValue, propertyNameParts[i]); | |
} |
{ | ||
propertyValue = Expression.Property(propertyValue, propertyName); | ||
// property access over declaring type from model without extra casts | ||
// - solves property path expressions like Stamp/CreatedByUser/Name, where CreatedByUser is interface which do not directly have a Name property, but the Name is in some base interface up in the hierarchy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you talk about a "base interface up in the hierarchy", do you mean a type that inerhits from CreatedByUser
or a type that CreatedByUser
inherits from?
Why not use that type that has the property in the path expressions instead of using a type that is not guaranteed to have that property?
Could you share more information about concrete scenarios where this issue occurs and that this PR aims to fix?
@mirecg Add tests and also respond to some of the questions already asked. |
@mirecg Also link the Issue ID associated with this PR if available. |
Solves property path expressions like Stamp/CreatedByUser/Name, where CreatedByUser is interface (e.g. IMyInterface) which do not directly have a Name property, but the Name is in some base interface up in the hierarchy.
Otherwise it internally throws System.ArgumentException: 'Instance property 'Name' is not defined for type 'IMyInterface'.
It is possible to overcome the issue using a cast expression, but this forces to use hardcoded casts on client calling odata endpoints and have deeper knowledge about model behind it.
Description
Using IEdmProperty and IEdmModel in ExpressionBinderBase.GetPropertyExpression() we can create property access expression with regards to declaring type of the property