Skip to content
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.

Sign up for GitHub

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

Fixes #1332: Deep Comparison for Array-based Concurrency tokens. #1334

Open
wants to merge 5 commits into
base: release-8.x
Choose a base branch
from
Open

Fixes #1332: Deep Comparison for Array-based Concurrency tokens. #1334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3641674
Added RowVersion field to ETagsModel.cs
anasik Oct 28, 2024
14e171b
Init and Update RowVersion in ETagsController.cs
anasik Oct 28, 2024
7ae9da7
It's a special tool that will help us later
anasik Oct 28, 2024
ed64ded
The magic happens
anasik Oct 28, 2024
31eae6f
Ignoring the new field in other tests.
anasik Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Microsoft.AspNetCore.OData/Query/ETag.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,11 @@ public virtual IQueryable ApplyTo(IQueryable query)
Expression value = itemValue != null
? LinqParameterContainer.Parameterize(itemValue.GetType(), itemValue)
: Expression.Constant(value: null);
BinaryExpression equal = Expression.Equal(name, value);
Expression equal;
if (itemValue != null && itemValue.GetType().IsArray)
equal = ExpressionHelpers.SequenceEquals(name, value);
else
equal = Expression.Equal(name, value);
where = where == null ? equal : Expression.AndAlso(where, equal);
}

Expand Down
9 changes: 9 additions & 0 deletions src/Microsoft.AspNetCore.OData/Query/ExpressionHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,5 +284,14 @@ public static LambdaExpression GetPropertyAccessLambda(Type type, string propert
MemberExpression propertyAccess = Expression.Property(odataItParameter, propertyName);
return Expression.Lambda(propertyAccess, odataItParameter);
}

public static Expression SequenceEquals(Expression left, Expression right)
{
MethodInfo sequenceEqualMethod = typeof(Enumerable)
.GetMethods(BindingFlags.Static | BindingFlags.Public)
.First(m => m.Name == "SequenceEqual" && m.GetParameters().Length == 2)
.MakeGenericMethod(right.Type.GetElementType());
return Expression.Call(sequenceEqualMethod, left, right);
}
}
}
3 changes: 3 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ETags/DerivedETagTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ private static IEdmModel GetEdmModel()
SingletonConfiguration<ETagsCustomer> eTagsCustomerSingleton = builder.Singleton<ETagsCustomer>("ETagsDerivedCustomersSingleton");
eTagsCustomerSingleton.HasRequiredBinding(c => c.RelatedCustomer, eTagsCustomersSet);
eTagsCustomerSingleton.HasRequiredBinding(c => c.ContainedCustomer, eTagsCustomersSet);
eTagsCustomersSet.EntityType.Ignore(d=>d.RowVersion);
return builder.GetEdmModel();
}

Expand All @@ -60,6 +61,8 @@ private static IEdmModel GetDerivedEdmModel()
EntitySetConfiguration<ETagsDerivedCustomer> eTagsDerivedCustomersSet = builder.EntitySet<ETagsDerivedCustomer>("ETagsDerivedCustomers");
eTagsDerivedCustomersSet.HasRequiredBinding(c => c.RelatedCustomer, eTagsCustomersSet);
eTagsDerivedCustomersSet.HasRequiredBinding(c => c.ContainedCustomer, eTagsCustomersSet);
eTagsDerivedCustomersSet.EntityType.Ignore(d=>d.RowVersion);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of excluding this property from EdmModel

Copy link
Author

@anasik anasik Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WanjohiSammy I added a new field. Most of these tests were written with the assumption that this field does not exist and therefore do not take its existence into account.

Me ignoring this field from these tests basically ensures that they continue to pass as they did previously since their test cases have nothing to do with the new field I created or the problem at hand.

I'm not overly fond of purposefully ignoring new code that breaks an existing test. Especially considering that the rest of the PR literally revolves around re-passing another existing test.

However, I only resolved to do this after I noticed that other test bases were ignoring one of the fields too for a similar reason. If you go to ETagsOtherTypesTest, you'd see that both the EDM Models there are ignoring the StringWithConcurrencyCheckAttributeProperty field.

eTagsCustomersSet.EntityType.Ignore(d=>d.RowVersion);
return builder.GetEdmModel();
}

Expand Down
5 changes: 3 additions & 2 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ETags/ETagsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public IActionResult Put(int key, [FromBody]ETagsCustomer eTagsCustomer, ODataQu
customer.UlongProperty = eTagsCustomer.UlongProperty;
customer.GuidProperty = eTagsCustomer.GuidProperty;
customer.DateTimeOffsetProperty = eTagsCustomer.DateTimeOffsetProperty;

customer.RowVersion[7]++;
return Ok(customer);
}

Expand Down Expand Up @@ -172,7 +172,7 @@ public IActionResult Patch(int key, [FromBody]Delta<ETagsCustomer> patch, ODataQ

ETagsCustomer customer = appliedCustomers.Single();
patch.Patch(customer);

customer.RowVersion[7]++;
return Ok(customer);
}
}
Expand All @@ -188,6 +188,7 @@ internal static ETagsCustomer CreateCustomer(int i)
ShortProperty = (short)(Int16.MaxValue - i),
DoubleProperty = 2.0 * (i + 1),
Notes = Enumerable.Range(0, i + 1).Select(j => "This is note " + (i * 10 + j)).ToList(),
RowVersion = new byte[] { 0, 0, 0, 0, 0, 0, 16, 28 },
RelatedCustomer = new ETagsCustomer
{
Id = i + 1,
Expand Down
2 changes: 2 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ETags/ETagsModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,7 @@ public class ETagsCustomer
public ETagsCustomer RelatedCustomer { get; set; }
[Contained]
public ETagsCustomer ContainedCustomer { get; set; }
[Timestamp]
public byte[] RowVersion { get; set; }
}
}
2 changes: 2 additions & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ETags/ETagsOtherTypesTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ private static IEdmModel GetDoubleETagEdmModel()
var customer = builder.EntitySet<ETagsCustomer>("ETagsCustomers").EntityType;
customer.Property(c => c.DoubleProperty).IsConcurrencyToken();
customer.Ignore(c => c.StringWithConcurrencyCheckAttributeProperty);
customer.Ignore(c => c.RowVersion);
return builder.GetEdmModel();
}

Expand All @@ -53,6 +54,7 @@ private static IEdmModel GetShortETagEdmModel()
var builder = new ODataConventionModelBuilder();
var customer = builder.EntitySet<ETagsCustomer>("ETagsCustomers").EntityType;
customer.Ignore(c => c.StringWithConcurrencyCheckAttributeProperty);
customer.Ignore(c => c.RowVersion);
customer.Property(c => c.ShortProperty).IsConcurrencyToken();
return builder.GetEdmModel();
}
Expand Down
1 change: 1 addition & 0 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ETags/JsonETagsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ private static IEdmModel GetEdmModel()
eTagsCustomers.Property(c => c.UlongProperty).IsConcurrencyToken();
eTagsCustomers.Property(c => c.GuidProperty).IsConcurrencyToken();
eTagsCustomers.Property(c => c.DateTimeOffsetProperty).IsConcurrencyToken();
eTagsCustomers.Ignore(d=>d.RowVersion);
return builder.GetEdmModel();
}

Expand Down