From c5c9cf101589f8926604aad872623e5caf1b16a0 Mon Sep 17 00:00:00 2001 From: Sam Xu Date: Wed, 15 Feb 2023 09:19:43 -0800 Subject: [PATCH] fixes #2598, Composite key entities PUT,PATCH URIs generated by OData 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 --- .../ALinq/QueryableResourceExpression.cs | 6 +- .../ALinq/ResourceBinder.cs | 67 +++++++++++++++- src/Microsoft.OData.Client/ClientEdmModel.cs | 15 +++- .../Metadata/ODataTypeInfo.cs | 78 ++++++++++++++++--- .../Tests/ClientEdmModelTests.cs | 24 ++++++ .../Tests/LinqIntegrationTests.cs | 31 +++++++- .../Tests/TemplatingIntegrationTests.cs | 28 +++---- 7 files changed, 218 insertions(+), 31 deletions(-) diff --git a/src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs b/src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs index 141ef45799..c2333e9d94 100644 --- a/src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs +++ b/src/Microsoft.OData.Client/ALinq/QueryableResourceExpression.cs @@ -507,9 +507,9 @@ internal void SetKeyPredicate(IEnumerable keyValues) /// Gets the key properties from KeyPredicateConjuncts /// /// The key properties. - internal Dictionary GetKeyProperties() + internal IList> GetKeyProperties() { - var keyValues = new Dictionary(EqualityComparer.Default); + var keyValues = new List>(); if (this.keyPredicateConjuncts.Count > 0) { foreach (Expression predicate in this.keyPredicateConjuncts) @@ -518,7 +518,7 @@ internal Dictionary GetKeyProperties() ConstantExpression constantValue; if (ResourceBinder.PatternRules.MatchKeyComparison(predicate, out property, out constantValue)) { - keyValues.Add(property, constantValue); + keyValues.Add(new KeyValuePair(property, constantValue)); } } } diff --git a/src/Microsoft.OData.Client/ALinq/ResourceBinder.cs b/src/Microsoft.OData.Client/ALinq/ResourceBinder.cs index 780459ca57..2393e989b9 100644 --- a/src/Microsoft.OData.Client/ALinq/ResourceBinder.cs +++ b/src/Microsoft.OData.Client/ALinq/ResourceBinder.cs @@ -333,6 +333,7 @@ private static List ExtractKeyPredicate( Debug.Assert(predicates != null, "predicates != null"); Dictionary keyValuesFromPredicates = null; + Dictionary expToPropertyNameMap = null; nonKeyPredicates = null; List keyPredicates = null; @@ -345,6 +346,7 @@ private static List ExtractKeyPredicate( if (keyValuesFromPredicates == null) { keyValuesFromPredicates = new Dictionary(EqualityComparer.Default); + expToPropertyNameMap = new Dictionary(); keyPredicates = new List(); } else if (keyValuesFromPredicates.ContainsKey(property)) @@ -356,6 +358,7 @@ private static List ExtractKeyPredicate( keyValuesFromPredicates.Add(property, constantValue); keyPredicates.Add(predicate); + expToPropertyNameMap.Add(predicate, ClientTypeUtil.GetServerDefinedName(property)); } else { @@ -384,22 +387,84 @@ private static List 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 AdjustKeyExpressionsBasedOnKeyOrders(IEdmStructuralProperty[] edmKeys, + List keyPredicates, Dictionary 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 orderedKeyPredicates = new List(); + 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; + } + /// Adds all AND'ed expressions to the specified list. /// Expression to recursively add conjuncts from. /// Target list of conjuncts. diff --git a/src/Microsoft.OData.Client/ClientEdmModel.cs b/src/Microsoft.OData.Client/ClientEdmModel.cs index ef2b5d431d..5f76c2e97c 100644 --- a/src/Microsoft.OData.Client/ClientEdmModel.cs +++ b/src/Microsoft.OData.Client/ClientEdmModel.cs @@ -473,7 +473,20 @@ private EdmTypeCacheValue GetOrCreateEdmTypeInternal(IEdmStructuredType edmBaseT entityType.AddProperty(property); } - entityType.AddKeys(loadedKeyProperties); + // add keys in the same order of "keyProperties" + List orderedloadedKeyProperties = new List(); + 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 diff --git a/src/Microsoft.OData.Client/Metadata/ODataTypeInfo.cs b/src/Microsoft.OData.Client/Metadata/ODataTypeInfo.cs index ea1393574c..d94e62c75e 100644 --- a/src/Microsoft.OData.Client/Metadata/ODataTypeInfo.cs +++ b/src/Microsoft.OData.Client/Metadata/ODataTypeInfo.cs @@ -271,7 +271,7 @@ private IEnumerable GetAllProperties() } private PropertyInfo[] GetKeyProperties() - { + { if (CommonUtil.IsUnsupportedType(type)) { throw new InvalidOperationException(c.Strings.ClientType_UnsupportedType(type)); @@ -281,27 +281,34 @@ private PropertyInfo[] GetKeyProperties() IEnumerable customAttributes = type.GetCustomAttributes(true); bool isEntity = customAttributes.OfType().Any(); KeyAttribute dataServiceKeyAttribute = customAttributes.OfType().FirstOrDefault(); - List keyProperties = new List(); - + + List> keyWithOrders = new List>(); + 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 keyProperties = new List(); + foreach (var item in keyWithOrders) + { + keyProperties.Add(item.Key); + } + Type keyPropertyDeclaringType = null; foreach (PropertyInfo key in keyProperties) { @@ -336,26 +343,56 @@ private PropertyInfo[] GetKeyProperties() return keyProperties.Count > 0 ? keyProperties.ToArray() : (isEntity ? ClientTypeUtil.EmptyPropertyInfoArray : null); } + private static void InserKeyBasedOnOrder(List> keys, PropertyInfo keyPi, int order) + { + var newKeyWithOrder = new KeyValuePair(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); + } + } + /// /// Returns the KeyKind if is declared as a key in or it follows the key naming convention. /// /// Property in question. /// DataServiceKeyAttribute instance. /// Returns the KeyKind if is declared as a key in or it follows the key naming convention. - 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().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)) { @@ -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().FirstOrDefault(); + if (columnAttribute != null) + { + order = columnAttribute.Order; + } + + kind = KeyKind.AttributedKey; + + return true; + } } } diff --git a/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/ClientEdmModelTests.cs b/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/ClientEdmModelTests.cs index 2749ab3969..34fcfa71eb 100644 --- a/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/ClientEdmModelTests.cs +++ b/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/ClientEdmModelTests.cs @@ -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 Objects { get; set; } diff --git a/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/LinqIntegrationTests.cs b/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/LinqIntegrationTests.cs index a6522adf49..bc5f0a471a 100644 --- a/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/LinqIntegrationTests.cs +++ b/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/LinqIntegrationTests.cs @@ -147,6 +147,13 @@ internal class EntityWithTwoKeyProperties public List 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 { @@ -164,6 +171,24 @@ internal class DerivedEntityWithTwoKeyProperties : EntityWithTwoKeyProperties [Fact] public void TwoWhereClausesForKeysShouldReturnUrlForSingleton() + { + // Simple case, only two key properties provided + string query1 = context.CreateQuery("Test") + .Where(p => p.ID1 == 1) + .Where(p => p.ID2 == 2) + .ToString(); + + string query2 = context.CreateQuery("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("Test") @@ -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("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] @@ -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("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] diff --git a/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/TemplatingIntegrationTests.cs b/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/TemplatingIntegrationTests.cs index 6e596d05a8..b414cbd3e1 100644 --- a/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/TemplatingIntegrationTests.cs +++ b/test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/TemplatingIntegrationTests.cs @@ -62,19 +62,19 @@ public void AttachShouldFailOnNullBuilder() public void AttachPinningTest() { const string expectedOutput = @"http://test.org/test/EntityS%C3%A9t1('foo%2B%2Fbar') -http://test.org/test/EntityS%C3%A9t2(Property1='fo%2Bo',Property2='b%2Far') +http://test.org/test/EntityS%C3%A9t2(Property2='b%2Far',Property1='fo%2Bo') http://test.org/test/EntitySet1('foo') -http://test.org/test/EntitySet2(Property1='foo',Property2='bar') +http://test.org/test/EntitySet2(Property2='bar',Property1='foo') http://test.org/test/EntitySet1('foo') -http://test.org/test/EntitySet2(Property1='foo',Property2='bar') +http://test.org/test/EntitySet2(Property2='bar',Property1='foo') http://test.org/test/EntitySet1/Fake(1)('foo') http://test.org/test/EntitySet1/Fake(1)('foo') http://test.org/test/EntitySet1/Fake(1)('foo') http://test.org/test/EntitySet1/Fake(1)('foo') -http://test.org/test/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') -http://test.org/test/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') -http://test.org/test/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') -http://test.org/test/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') +http://test.org/test/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') +http://test.org/test/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') +http://test.org/test/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') +http://test.org/test/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') "; var ctx = new DataServiceContext(new Uri("http://test.org/test")); RunAttachPinningTest(ctx, expectedOutput); @@ -87,19 +87,19 @@ public void AttachPinningTest() public void AttachPinningTestWithEntitySetResolver() { const string expectedOutput = @"http://resolved.org/EntityS%C3%A9t1('foo%2B%2Fbar') -http://resolved.org/EntityS%C3%A9t2(Property1='fo%2Bo',Property2='b%2Far') +http://resolved.org/EntityS%C3%A9t2(Property2='b%2Far',Property1='fo%2Bo') http://resolved.org/EntitySet1('foo') -http://resolved.org/EntitySet2(Property1='foo',Property2='bar') +http://resolved.org/EntitySet2(Property2='bar',Property1='foo') http://resolved.org/EntitySet1('foo') -http://resolved.org/EntitySet2(Property1='foo',Property2='bar') +http://resolved.org/EntitySet2(Property2='bar',Property1='foo') http://resolved.org/EntitySet1/Fake(1)('foo') http://resolved.org/EntitySet1/Fake(1)('foo') http://resolved.org/EntitySet1/Fake(1)('foo') http://resolved.org/EntitySet1/Fake(1)('foo') -http://resolved.org/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') -http://resolved.org/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') -http://resolved.org/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') -http://resolved.org/EntitySet2/Fake(1)/Navigation(Property1='foo',Property2='bar') +http://resolved.org/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') +http://resolved.org/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') +http://resolved.org/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') +http://resolved.org/EntitySet2/Fake(1)/Navigation(Property2='bar',Property1='foo') "; var ctx = new TestClientContext { ResolveEntitySet = s => new Uri("http://resolved.org/" + s) }; RunAttachPinningTest(ctx, expectedOutput);