Skip to content

Commit

Permalink
fixes #2598, Composite key entities PUT,PATCH URIs generated by OData…
Browse files Browse the repository at this point in the history 
… Client do not follow key order (#2603)

* fixes #2598, Composite key entities PUT,PATCH URIs generated by OData Client do not follow key order

* Add more tests and fix the failing test cases

* Fix the failing test cases.

* Add the comments
  • Loading branch information
@xuzhg
xuzhg authored Feb 15, 2023
1 parent aa6a0af commit c5c9cf1
Show file tree
Hide file tree
Showing 7 changed files with 218 additions and 31 deletions.
6 changes: 3 additions & 3 deletions src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,9 +507,9 @@ internal void SetKeyPredicate(IEnumerable<Expression> keyValues)
/// Gets the key properties from KeyPredicateConjuncts
/// </summary>
/// <returns>The key properties.</returns>
internal Dictionary<PropertyInfo, ConstantExpression> GetKeyProperties()
internal IList<KeyValuePair<PropertyInfo, ConstantExpression>> GetKeyProperties()
{
var keyValues = new Dictionary<PropertyInfo, ConstantExpression>(EqualityComparer<PropertyInfo>.Default);
var keyValues = new List<KeyValuePair<PropertyInfo, ConstantExpression>>();
if (this.keyPredicateConjuncts.Count > 0)
{
foreach (Expression predicate in this.keyPredicateConjuncts)
Expand All @@ -518,7 +518,7 @@ internal Dictionary<PropertyInfo, ConstantExpression> GetKeyProperties()
ConstantExpression constantValue;
if (ResourceBinder.PatternRules.MatchKeyComparison(predicate, out property, out constantValue))
{
keyValues.Add(property, constantValue);
keyValues.Add(new KeyValuePair<PropertyInfo, ConstantExpression>(property, constantValue));
}
}
}
Expand Down
67 changes: 66 additions & 1 deletion src/Microsoft.OData.Client/ALinq/ResourceBinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ private static List<Expression> ExtractKeyPredicate(
Debug.Assert(predicates != null, "predicates != null");

Dictionary<PropertyInfo, ConstantExpression> keyValuesFromPredicates = null;
Dictionary<Expression, string> expToPropertyNameMap = null;
nonKeyPredicates = null;
List<Expression> keyPredicates = null;

Expand All @@ -345,6 +346,7 @@ private static List<Expression> ExtractKeyPredicate(
if (keyValuesFromPredicates == null)
{
keyValuesFromPredicates = new Dictionary<PropertyInfo, ConstantExpression>(EqualityComparer<PropertyInfo>.Default);
expToPropertyNameMap = new Dictionary<Expression, string>();
keyPredicates = new List<Expression>();
}
else if (keyValuesFromPredicates.ContainsKey(property))
Expand All @@ -356,6 +358,7 @@ private static List<Expression> ExtractKeyPredicate(

keyValuesFromPredicates.Add(property, constantValue);
keyPredicates.Add(predicate);
expToPropertyNameMap.Add(predicate, ClientTypeUtil.GetServerDefinedName(property));
}
else
{
Expand Down Expand Up @@ -384,22 +387,84 @@ private static List<Expression> ExtractKeyPredicate(

Debug.Assert(schemaType != null, "SchemaType can not be null.");

IEdmStructuralProperty[] keys = schemaType.Key().ToArray();
// Obtain the key properties from EdmEntityType, and compare them with keys obtained from predicates
// By this time we are sure that keyValuesFromPredicates has unique set (i.e. no duplicates), and has all the keys in it.
// So just comparing count with EDMModel's keys is enough for validation
bool allKeysPresent = schemaType.Key().Count() == keyValuesFromPredicates.Keys.Count;
bool allKeysPresent = keys.Length == keyValuesFromPredicates.Keys.Count;

if (!allKeysPresent)
{
keyValuesFromPredicates = null;
keyPredicates = null;
}

if (allKeysPresent && keys.Length > 1)
{
// OData Client generates a request that should be the canonical URL, based on the order of keys in the CSDL.
keyPredicates = AdjustKeyExpressionsBasedOnKeyOrders(keys, keyPredicates, expToPropertyNameMap);
}
}
}

return keyPredicates;
}

private static List<Expression> AdjustKeyExpressionsBasedOnKeyOrders(IEdmStructuralProperty[] edmKeys,
List<Expression> keyPredicates, Dictionary<Expression, string> expToPropertyNameMap)
{
bool needAdjust = false;

for (int i = 0; i < edmKeys.Length; ++i)
{
IEdmStructuralProperty edmKey = edmKeys[i];
Expression keyExpr = keyPredicates[i];

if (edmKey.Name != expToPropertyNameMap[keyExpr])
{
needAdjust = true;
break;
}
}

if (needAdjust)
{
List<Expression> orderedKeyPredicates = new List<Expression>();
foreach (var key in edmKeys)
{
// find
Expression foundExp = null;
foreach (var exp in keyPredicates)
{
string propertyName = expToPropertyNameMap[exp];

if (key.Name == propertyName)
{
foundExp = exp;
break;
}
}

// if found
if (foundExp != null)
{
keyPredicates.Remove(foundExp);
orderedKeyPredicates.Add(foundExp);
}
}

// append the remainings
foreach (var exp in keyPredicates)
{
orderedKeyPredicates.Add(exp);
}

keyPredicates = orderedKeyPredicates;
}

return keyPredicates;
}

/// <summary>Adds all AND'ed expressions to the specified <paramref name="conjuncts"/> list.</summary>
/// <param name="e">Expression to recursively add conjuncts from.</param>
/// <param name="conjuncts">Target list of conjuncts.</param>
Expand Down
15 changes: 14 additions & 1 deletion src/Microsoft.OData.Client/ClientEdmModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,20 @@ private EdmTypeCacheValue GetOrCreateEdmTypeInternal(IEdmStructuredType edmBaseT
entityType.AddProperty(property);
}

entityType.AddKeys(loadedKeyProperties);
// add keys in the same order of "keyProperties"
List<IEdmStructuralProperty> orderedloadedKeyProperties = new List<IEdmStructuralProperty>();
foreach (var orderedKey in keyProperties)
{
string serverDefinedName = ClientTypeUtil.GetServerDefinedName(orderedKey);
IEdmStructuralProperty keyProperty = loadedKeyProperties.FirstOrDefault(k => k.Name == serverDefinedName);

if (keyProperty != null)
{
orderedloadedKeyProperties.Add(keyProperty);
}
}

entityType.AddKeys(orderedloadedKeyProperties);
};

// Creating an entity type
Expand Down
78 changes: 68 additions & 10 deletions src/Microsoft.OData.Client/Metadata/ODataTypeInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ private IEnumerable<PropertyInfo> GetAllProperties()
}

private PropertyInfo[] GetKeyProperties()
{
{
if (CommonUtil.IsUnsupportedType(type))
{
throw new InvalidOperationException(c.Strings.ClientType_UnsupportedType(type));
Expand All @@ -281,27 +281,34 @@ private PropertyInfo[] GetKeyProperties()
IEnumerable<object> customAttributes = type.GetCustomAttributes(true);
bool isEntity = customAttributes.OfType<EntityTypeAttribute>().Any();
KeyAttribute dataServiceKeyAttribute = customAttributes.OfType<KeyAttribute>().FirstOrDefault();
List<PropertyInfo> keyProperties = new List<PropertyInfo>();


List<KeyValuePair<PropertyInfo, int>> keyWithOrders = new List<KeyValuePair<PropertyInfo, int>>();

KeyKind currentKeyKind = KeyKind.NotKey;
KeyKind newKeyKind = KeyKind.NotKey;
foreach (PropertyInfo propertyInfo in Properties)
{
if ((newKeyKind = IsKeyProperty(propertyInfo, dataServiceKeyAttribute)) != KeyKind.NotKey)
if ((newKeyKind = IsKeyProperty(propertyInfo, dataServiceKeyAttribute, out int order)) != KeyKind.NotKey)
{
if (newKeyKind > currentKeyKind)
{
keyProperties.Clear();
keyWithOrders.Clear();
currentKeyKind = newKeyKind;
keyProperties.Add(propertyInfo);
InserKeyBasedOnOrder(keyWithOrders, propertyInfo, order);
}
else if (newKeyKind == currentKeyKind)
{
keyProperties.Add(propertyInfo);
InserKeyBasedOnOrder(keyWithOrders, propertyInfo, order);
}
}
}

List<PropertyInfo> keyProperties = new List<PropertyInfo>();
foreach (var item in keyWithOrders)
{
keyProperties.Add(item.Key);
}

Type keyPropertyDeclaringType = null;
foreach (PropertyInfo key in keyProperties)
{
Expand Down Expand Up @@ -336,26 +343,56 @@ private PropertyInfo[] GetKeyProperties()
return keyProperties.Count > 0 ? keyProperties.ToArray() : (isEntity ? ClientTypeUtil.EmptyPropertyInfoArray : null);
}

private static void InserKeyBasedOnOrder(List<KeyValuePair<PropertyInfo, int>> keys, PropertyInfo keyPi, int order)
{
var newKeyWithOrder = new KeyValuePair<PropertyInfo, int>(keyPi, order);
if (order < 0)
{
// order < 0 means there's no order value setting, append this key at the end of list.
keys.Add(newKeyWithOrder);
}
else
{
int index = 0;
foreach (var key in keys)
{
if (key.Value < 0 || key.Value > order)
{
// Insert the new key before the first negative order or first order bigger than new order.
// If the new order value is same as one item in the list , move to next.
break;
}

++index;
}

keys.Insert(index, newKeyWithOrder);
}
}

/// <summary>
/// Returns the KeyKind if <paramref name="propertyInfo"/> is declared as a key in <paramref name="dataServiceKeyAttribute"/> or it follows the key naming convention.
/// </summary>
/// <param name="propertyInfo">Property in question.</param>
/// <param name="dataServiceKeyAttribute">DataServiceKeyAttribute instance.</param>
/// <returns>Returns the KeyKind if <paramref name="propertyInfo"/> is declared as a key in <paramref name="dataServiceKeyAttribute"/> or it follows the key naming convention.</returns>
private static KeyKind IsKeyProperty(PropertyInfo propertyInfo, KeyAttribute dataServiceKeyAttribute)
private static KeyKind IsKeyProperty(PropertyInfo propertyInfo, KeyAttribute dataServiceKeyAttribute, out int order)
{
Debug.Assert(propertyInfo != null, "propertyInfo != null");

string propertyName = ClientTypeUtil.GetServerDefinedName(propertyInfo);

order = -1;
KeyKind keyKind = KeyKind.NotKey;
if (dataServiceKeyAttribute != null && dataServiceKeyAttribute.KeyNames.Contains(propertyName))
{
order = dataServiceKeyAttribute.KeyNames.IndexOf(propertyName);
keyKind = KeyKind.AttributedKey;
}
else if (propertyInfo.GetCustomAttributes().OfType<System.ComponentModel.DataAnnotations.KeyAttribute>().Any())
else if (IsDataAnnotationsKeyProperty(propertyInfo, out KeyKind newKind, out int newOrder))
{
keyKind = KeyKind.AttributedKey;
order = newOrder;
keyKind = newKind;
}
else if (propertyName.EndsWith("ID", StringComparison.Ordinal))
{
Expand All @@ -374,5 +411,26 @@ private static KeyKind IsKeyProperty(PropertyInfo propertyInfo, KeyAttribute dat

return keyKind;
}

private static bool IsDataAnnotationsKeyProperty(PropertyInfo propertyInfo, out KeyKind kind, out int order)
{
order = -1;
kind = KeyKind.NotKey;
var attributes = propertyInfo.GetCustomAttributes();
if(!attributes.Any(a => a is System.ComponentModel.DataAnnotations.KeyAttribute))
{
return false;
}

var columnAttribute = attributes.OfType<System.ComponentModel.DataAnnotations.Schema.ColumnAttribute>().FirstOrDefault();
if (columnAttribute != null)
{
order = columnAttribute.Order;
}

kind = KeyKind.AttributedKey;

return true;
}
}
}
24 changes: 24 additions & 0 deletions ...unctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/ClientEdmModelTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,30 @@ public void GetOrCreateEdmTypeShouldWorkForEnumTypes()
Assert.NotNull(enumType.FullName());
}

[Fact]
public void GetOrCreateEdmTypeShouldWorkForTypesWithMultipleKeys()
{
var clientModel = new ClientEdmModel(ODataProtocolVersion.V4);
var edmType = clientModel.GetOrCreateEdmType(typeof(EntityWithThreeKeyProperties));
Assert.NotNull(edmType);
Assert.Equal(EdmTypeKind.Entity, edmType.TypeKind);
IEdmEntityType entityType = edmType as IEdmEntityType;
var keys = entityType.Key();
Assert.Collection(keys,
e => Assert.Equal("ID1", e.Name),
e => Assert.Equal("ID3", e.Name),
e => Assert.Equal("ID2", e.Name)
);
}

[Key("ID1", "ID3", "ID2")]
internal class EntityWithThreeKeyProperties
{
public int ID1 { get; set; }
public string ID2 { get; set; }
public DateTimeOffset ID3 { get; set; }
}

private class TypeWithCollectionOfObjectProperty
{
public ICollection<object> Objects { get; set; }
Expand Down
31 changes: 29 additions & 2 deletions ...nctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/LinqIntegrationTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,13 @@ internal class EntityWithTwoKeyProperties
public List<AnotherEntityWithTwoKeyProperties> NavPropertyCollection { get; set; }
}

[Key("ID2", "ID1")]
internal class EntityWithTwoDiffOrderKeyProperties
{
public int ID1 { get; set; }
public int ID2 { get; set; }
}

[Key("Key1", "Key2")]
internal class AnotherEntityWithTwoKeyProperties
{
Expand All @@ -164,6 +171,24 @@ internal class DerivedEntityWithTwoKeyProperties : EntityWithTwoKeyProperties

[Fact]
public void TwoWhereClausesForKeysShouldReturnUrlForSingleton()
{
// Simple case, only two key properties provided
string query1 = context.CreateQuery<EntityWithTwoDiffOrderKeyProperties>("Test")
.Where(p => p.ID1 == 1)
.Where(p => p.ID2 == 2)
.ToString();

string query2 = context.CreateQuery<EntityWithTwoDiffOrderKeyProperties>("Test")
.Where(p => p.ID2 == 2)
.Where(p => p.ID1 == 1)
.ToString();

Assert.Equal(query1, query2);
Assert.Equal(rootUrl + "Test(ID2=2,ID1=1)", query1);
}

[Fact]
public void TwoWhereClausesForDiffOrderKeysShouldReturnUrlForSingleton()
{
// Simple case, only two key properties provided
string query = context.CreateQuery<EntityWithTwoKeyProperties>("Test")
Expand All @@ -177,10 +202,11 @@ public void TwoWhereClausesForKeysShouldReturnUrlForSingleton()
public void TwoWhereClausesForKeysInDifferentOrderShouldReturnUrlForSingltonInCorrectOrder()
{
// Key properties provided in reverse order
// The request should be canonical URL (using key order in schema)
query = context.CreateQuery<EntityWithTwoKeyProperties>("Test")
.Where(p => p.ID2 == 1).Where(p => p.ID1 == 2)
.ToString();
Assert.Equal(rootUrl + "Test(ID2=1,ID1=2)", query);
Assert.Equal(rootUrl + "Test(ID1=2,ID2=1)", query);
}

[Fact]
Expand Down Expand Up @@ -419,12 +445,13 @@ public void ThreeWhereClausesForKeysShouldReturnUrlForSingleton()
public void ThreeWhereClausesForKeysInDifferentOrderShouldReturnUrlForSingltonInCorrectOrder()
{
// Key properties provided in reverse order
// The request should be canonical URL (using key order in schema)
query = context.CreateQuery<EntityWithThreeKeyProperties>("Test")
.Where(p => p.ID2 == "bar")
.Where(p => p.ID1 == 2)
.Where(p => p.ID3 == this.sampleDateTimeOffset)
.ToString();
Assert.Equal(rootUrl + "Test(ID2='bar',ID1=2,ID3=" + this.sampleDate + ")", query);
Assert.Equal(rootUrl + "Test(ID1=2,ID2='bar',ID3=" + this.sampleDate + ")", query);
}

[Fact]
Expand Down
Loading

0 comments on commit c5c9cf1

Please sign in to comment.