-
Notifications
You must be signed in to change notification settings - Fork 31
issue #71 #104
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
base: master
Are you sure you want to change the base?
issue #71 #104
Changes from 9 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,16 @@ namespace EntityFrameworkCore.Projectables | |
| [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = true, AllowMultiple = false)] | ||
| public sealed class ProjectableAttribute : Attribute | ||
| { | ||
| public ProjectableAttribute() { } | ||
|
|
||
| public ProjectableAttribute(string useMemberBody, object memberBodyParameterValue) | ||
| { | ||
| UseMemberBody = useMemberBody; | ||
| MemberBodyParameterValues = new[] { memberBodyParameterValue }; | ||
|
|
||
| } | ||
| public ProjectableAttribute(string useMemberBody, string memberBodyParameterValue) : this(useMemberBody, (object)memberBodyParameterValue) { } | ||
|
|
||
| /// <summary> | ||
| /// Get or set how null-conditional operators are handeled | ||
| /// </summary> | ||
|
|
@@ -23,5 +33,10 @@ public sealed class ProjectableAttribute : Attribute | |
| /// or null to get it from the current member. | ||
| /// </summary> | ||
| public string? UseMemberBody { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Parameters values for UseMemberBody. | ||
| /// </summary> | ||
| public object[]? MemberBodyParameterValues { get; set; } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer the name
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment accepted |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ bool TryGetReflectedExpression(MemberInfo memberInfo, [NotNullWhen(true)] out La | |
|
|
||
| reflectedExpression = projectableAttribute is not null | ||
| ? _resolver.FindGeneratedExpression(memberInfo) | ||
| : null; | ||
| : (LambdaExpression?)null; | ||
|
|
||
| _projectableMemberCache.Add(memberInfo, reflectedExpression); | ||
| } | ||
|
|
@@ -209,19 +209,30 @@ PropertyInfo property when nodeExpression is not null | |
| if (nodeExpression is not null) | ||
| { | ||
| _expressionArgumentReplacer.ParameterArgumentMapping.Add(reflectedExpression.Parameters[0], nodeExpression); | ||
| if (reflectedExpression.Parameters.Count > 1) | ||
| { | ||
| var projectableAttribute = nodeMember.GetCustomAttribute<ProjectableAttribute>(false)!; | ||
| foreach (var prm in reflectedExpression.Parameters.Skip(1).Select((Parameter, Index) => new { Parameter, Index })) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer calling this
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment accepted |
||
| { | ||
| var value = projectableAttribute!.MemberBodyParameterValues![prm.Index]; | ||
| _expressionArgumentReplacer.ParameterArgumentMapping.Add(prm.Parameter, Expression.Constant(value)); | ||
| } | ||
| } | ||
|
|
||
| var updatedBody = _expressionArgumentReplacer.Visit(reflectedExpression.Body); | ||
| _expressionArgumentReplacer.ParameterArgumentMapping.Clear(); | ||
|
|
||
| return base.Visit( | ||
| return Visit( | ||
| updatedBody | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return base.Visit( | ||
| return Visit( | ||
| reflectedExpression.Body | ||
| ); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| return base.VisitMember(node); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,11 +15,14 @@ public LambdaExpression FindGeneratedExpression(MemberInfo projectableMemberInfo | |
|
|
||
| var expression = GetExpressionFromGeneratedType(projectableMemberInfo); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be no need to call this if
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment accepted |
||
|
|
||
| if (expression is null && projectableAttribute.UseMemberBody is not null && projectableAttribute.MemberBodyParameterValues is null) | ||
| { | ||
| expression = GetExpressionFromGeneratedType(projectableMemberInfo, true, projectableAttribute.UseMemberBody); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The result will be overwritten by the call on line 24
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think differently that when calling line 20 the expression will not be empty |
||
| } | ||
| if (expression is null && projectableAttribute.UseMemberBody is not null) | ||
| { | ||
| expression = GetExpressionFromMemberBody(projectableMemberInfo, projectableAttribute.UseMemberBody); | ||
| expression = GetExpressionFromMemberBody(projectableMemberInfo, projectableAttribute.UseMemberBody, projectableAttribute.MemberBodyParameterValues); | ||
| } | ||
|
|
||
| if (expression is null) | ||
| { | ||
| var declaringType = projectableMemberInfo.DeclaringType ?? throw new InvalidOperationException("Expected a valid type here"); | ||
|
|
@@ -33,17 +36,32 @@ public LambdaExpression FindGeneratedExpression(MemberInfo projectableMemberInfo | |
|
|
||
| return expression; | ||
|
|
||
| static LambdaExpression? GetExpressionFromMemberBody(MemberInfo projectableMemberInfo, string memberName) | ||
| static LambdaExpression? GetExpressionFromMemberBody(MemberInfo projectableMemberInfo, string memberName, object[]? memberParameters) | ||
| { | ||
| var declaringType = projectableMemberInfo.DeclaringType ?? throw new InvalidOperationException("Expected a valid type here"); | ||
| var exprProperty = declaringType.GetProperty(memberName, BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); | ||
|
|
||
| var exprProperty = declaringType.GetProperty(memberName, BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) | ||
| ?? declaringType.BaseType?.GetProperty(memberName, BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); | ||
| var lambda = exprProperty?.GetValue(null) as LambdaExpression; | ||
|
|
||
| if (lambda is not null) | ||
| { | ||
| if (projectableMemberInfo is PropertyInfo property && | ||
| lambda.Parameters.Count == 1 && | ||
| lambda.Parameters[0].Type == declaringType && lambda.ReturnType == property.PropertyType) | ||
|
|
||
| if (projectableMemberInfo is PropertyInfo property && (lambda.Parameters.Count == | ||
| (1 + (memberParameters?.Length ?? 0))) | ||
| && (lambda.Parameters[0].Type == declaringType || | ||
| lambda.Parameters[0].Type == | ||
| declaringType.BaseType) | ||
| && lambda.ReturnType == property.PropertyType | ||
| && (memberParameters?.Any() != true | ||
| || lambda.Parameters.Skip(1) | ||
| .Select((Parameter, Index) => | ||
| new { Parameter, Index }) | ||
| .All(p => p.Parameter.Type == | ||
| memberParameters[p.Index] | ||
| .GetType()))) | ||
|
|
||
|
|
||
| { | ||
| return lambda; | ||
| } | ||
|
|
@@ -55,16 +73,16 @@ public LambdaExpression FindGeneratedExpression(MemberInfo projectableMemberInfo | |
| return lambda; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| static LambdaExpression? GetExpressionFromGeneratedType(MemberInfo projectableMemberInfo) | ||
| static LambdaExpression? GetExpressionFromGeneratedType(MemberInfo projectableMemberInfo, bool useLocalType = false, string methodName = "Expression") | ||
| { | ||
| var declaringType = projectableMemberInfo.DeclaringType ?? throw new InvalidOperationException("Expected a valid type here"); | ||
| var generatedContainingTypeName = ProjectionExpressionClassNameGenerator.GenerateFullName(declaringType.Namespace, declaringType.GetNestedTypePath().Select(x => x.Name), projectableMemberInfo.Name); | ||
|
|
||
| var expressionFactoryType = declaringType.Assembly.GetType(generatedContainingTypeName); | ||
| var expressionFactoryType = !useLocalType | ||
| ? declaringType.Assembly.GetType(generatedContainingTypeName) | ||
| : declaringType; | ||
|
|
||
| if (expressionFactoryType is not null) | ||
| { | ||
|
|
@@ -73,7 +91,7 @@ public LambdaExpression FindGeneratedExpression(MemberInfo projectableMemberInfo | |
| expressionFactoryType = expressionFactoryType.MakeGenericType(declaringType.GenericTypeArguments); | ||
| } | ||
|
|
||
| var expressionFactoryMethod = expressionFactoryType.GetMethod("Expression", BindingFlags.Static | BindingFlags.NonPublic); | ||
| var expressionFactoryMethod = expressionFactoryType.GetMethod(methodName, BindingFlags.Static | BindingFlags.NonPublic); | ||
|
|
||
| var methodGenericArguments = projectableMemberInfo switch { | ||
| MethodInfo methodInfo => methodInfo.GetGenericArguments(), | ||
|
|
@@ -90,7 +108,6 @@ public LambdaExpression FindGeneratedExpression(MemberInfo projectableMemberInfo | |
| return expressionFactoryMethod.Invoke(null, null) as LambdaExpression ?? throw new InvalidOperationException("Expected lambda"); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| using System.Collections.Generic; | ||
| using EntityFrameworkCore.Projectables.Infrastructure; | ||
| using Microsoft.EntityFrameworkCore; | ||
| using Order = EntityFrameworkCore.Projectables.FunctionalTests.MemberBodyParameterValueTests.Order; | ||
| using OrderItem = EntityFrameworkCore.Projectables.FunctionalTests.MemberBodyParameterValueTests.OrderItem; | ||
|
|
||
| namespace EntityFrameworkCore.Projectables.FunctionalTests.Helpers | ||
| { | ||
| public class SampleBodyParamDbContext : DbContext | ||
| { | ||
| readonly CompatibilityMode _compatibilityMode; | ||
|
|
||
| public SampleBodyParamDbContext(CompatibilityMode compatibilityMode = CompatibilityMode.Full) | ||
| { | ||
| _compatibilityMode = compatibilityMode; | ||
|
|
||
| var _orders = new List<Order>() { | ||
| new() { | ||
| Id = 1, | ||
|
|
||
| }, | ||
| new() { | ||
| Id = 2, | ||
|
|
||
| } | ||
| }; | ||
|
|
||
| var _orders_items = new List<OrderItem>() { | ||
| new() { | ||
| Id = 1, | ||
| OrderId = 1, | ||
| Name = "Order_1" | ||
| }, | ||
| new() { | ||
| Id = 2, | ||
| OrderId = 1, | ||
| Name = "Order_2" | ||
| }, | ||
| new() { | ||
| Id = 3, | ||
| OrderId = 2, | ||
| Name = "Order_3" | ||
| }, | ||
| new() { | ||
| Id = 4, | ||
| OrderId = 2, | ||
| Name = "Order_4" | ||
| }, | ||
| }; | ||
|
|
||
| Order!.AddRange(_orders); | ||
| OrderItem!.AddRange(_orders_items); | ||
| SaveChanges(); | ||
| } | ||
|
|
||
| protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) | ||
| { | ||
| optionsBuilder.UseInMemoryDatabase("TestDb"); | ||
| optionsBuilder.UseProjectables(options => { | ||
| options.CompatibilityMode(_compatibilityMode); // Needed by our ComplexModelTests | ||
| }); | ||
| } | ||
|
|
||
| protected override void OnModelCreating(ModelBuilder modelBuilder) | ||
| { | ||
| modelBuilder.Entity<Order>() | ||
| .HasKey(x => x.Id); | ||
| modelBuilder.Entity<OrderItem>().HasKey(x => x.Id); | ||
|
|
||
| } | ||
|
|
||
| public DbSet<Order>? Order { get; set; } | ||
| public DbSet<OrderItem>? OrderItem { get; set; } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using System.ComponentModel.DataAnnotations.Schema; | ||
| using System.Linq; | ||
| using System.Linq.Expressions; | ||
| using EntityFrameworkCore.Projectables.FunctionalTests.Helpers; | ||
| using Microsoft.EntityFrameworkCore; | ||
| using Xunit; | ||
|
|
||
| namespace EntityFrameworkCore.Projectables.FunctionalTests | ||
| { | ||
| public class MemberBodyParameterValueTests | ||
| { | ||
| public class Order | ||
| { | ||
| [Key, DatabaseGenerated(DatabaseGeneratedOption.None)] | ||
| public int Id { get; set; } | ||
| public List<OrderItem> OrderItem { get; set; } = new List<OrderItem>(); | ||
|
|
||
| [Projectable(nameof(GetOrderItemNameExpression), 1)] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This example confuses me, could you not just have done: [Projectable(UseMemberBody = nameof(GetOrderItemNameInnerExpressionWithArgument))]
public string FirstOrderPropName => GetOrderItemName(1);
private static Expression<Func<Order, string>> GetOrderItemNameInnerExpressionWithArgument()
=> new MyParameterExpressionVisitor(parameter: "id", value: 1).Visit(GetOrderItemNameInnerExpression());
private static Expression<Func<Order, int, string>> GetOrderItemNameInnerExpression()
=> (@this, id) => @this.OrderItem
.Where(av => av.Id == id)
.Select(av => av.Name)
.FirstOrDefault() ?? string.Empty;Where
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also confused, but we generate this call in runtime and therefore it is convenient for us to pass the value of the parameters through the attribute. |
||
| public string FirstOrderPropName => GetOrderItemName(1); | ||
|
|
||
|
|
||
| [Projectable(UseMemberBody = nameof(GetOrderItemNameInnerExpression))] | ||
| public string GetOrderItemName(int id) | ||
| => OrderItem.Where(av => av.Id == id) | ||
| .Select(av => av.Name) | ||
| .FirstOrDefault() ?? throw new ArgumentException("Argument matching identifier not found"); | ||
|
|
||
| private static Expression<Func<Order, int, string>> GetOrderItemNameInnerExpression() | ||
| => (@this, id) => @this.OrderItem | ||
| .Where(av => av.Id == id) | ||
| .Select(av => av.Name) | ||
| .FirstOrDefault() ?? string.Empty; | ||
|
|
||
| public static Expression<Func<Order, int, string>> GetOrderItemNameExpression | ||
| => (order, id) => order.GetOrderItemName(id); | ||
| } | ||
|
|
||
| public class OrderItem | ||
| { | ||
| [Key, DatabaseGenerated(DatabaseGeneratedOption.None)] | ||
| public int Id { get; set; } | ||
| public int OrderId { get; set; } | ||
| public string Name { get; set; } = string.Empty; | ||
| } | ||
|
|
||
| [Fact] | ||
| public void UseBodyParameterValue() | ||
| { | ||
| //Arrange | ||
| using var dbContext = new SampleBodyParamDbContext(); | ||
|
|
||
| // Act | ||
| var query = dbContext | ||
| .Set<Order>() | ||
| .Include(a => a.OrderItem) | ||
| .FirstOrDefault(d => d.FirstOrderPropName == "Order_1"); | ||
|
|
||
| // Assert | ||
| Assert.NotNull(query); | ||
| Assert.True(query!.FirstOrderPropName == "Order_1"); | ||
| } | ||
| } | ||
| } | ||
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.
A constructor would be an aggressive change as this would limit our ability in the future to add additional constructor arguments. Just having a property would be sufficient. Can this constructor be dropped?
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.
Comment accepted