Fix logic to validate URL segment by type (#1891)

* Fix logic to validate by type * Added Tests
OData · Sep 22, 2020 · 74946ca · 74946ca
1 parent 6e942d1
commit 74946ca
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 37 deletions.
diff --git a/src/Microsoft.OData.Core/UrlValidation/Rules/DeprecationRules.cs b/src/Microsoft.OData.Core/UrlValidation/Rules/DeprecationRules.cs
@@ -118,10 +118,6 @@ private static void Validate(IEdmNamedElement element, IEdmType elementType, ODa
             {
                 context.Messages.Add(CreateUrlValidationMessage(element.Name, message, version, date, removalDate));
             }
-            else if (IsDeprecated(context.Model, elementType, out message, out version, out date, out removalDate))
-            {
-                context.Messages.Add(CreateUrlValidationMessage(element.Name, message, version, date, removalDate));
-            }
         }
 
         /// <summary>

diff --git a/src/Microsoft.OData.Core/UrlValidation/ValidationEngine/ODataUrlValidator.cs b/src/Microsoft.OData.Core/UrlValidation/ValidationEngine/ODataUrlValidator.cs
@@ -128,47 +128,44 @@ private bool ValidateTypeHierarchy(object item, Type itemType, ODataUrlValidatio
         {
             bool foundRule = false;
 
-            if (!this.unusedTypes.ContainsKey(itemType))
+            // if we are at the base type or already know there are no rules in the remaining hierarchy, return
+            if (itemType == typeof(object) || unusedTypes.ContainsKey(itemType))
             {
-                // if we are at the base type or already know there are no rules in the remaining hierarchy, return
-                if (itemType == typeof(object) || unusedTypes.ContainsKey(itemType))
-                {
-                    return false;
-                }
+                return false;
+            }
 
-                // find any rules defined on the type of the item
-                if (ValidateByType(item, itemType, validationContext, impliedProperty))
-                {
-                    foundRule = true;
-                }
+            // find any rules defined on the type of the item
+            if (ValidateByType(item, itemType, validationContext, impliedProperty))
+            {
+                foundRule = true;
+            }
 
-                // make sure we only validate the item only once per type/interface
-                validatedTypes.Add(itemType);
+            // make sure we only validate the item only once per type/interface
+            validatedTypes.Add(itemType);
 
-                // find any rules defined on an interface defined on the type of the item
-                foreach (Type interfaceType in itemType.GetInterfaces())
+            // find any rules defined on an interface defined on the type of the item
+            foreach (Type interfaceType in itemType.GetInterfaces())
+            {
+                if (validatedTypes.Add(interfaceType) && !this.unusedTypes.ContainsKey(interfaceType))
                 {
-                    if (validatedTypes.Add(interfaceType) && !this.unusedTypes.ContainsKey(interfaceType))
+                    if (ValidateByType(item, interfaceType, validationContext, impliedProperty))
                     {
-                        if (ValidateByType(item, interfaceType, validationContext, impliedProperty))
-                        {
-                            foundRule = true;
-                        }
-                        else
-                        {
-                            // there were no rules for this interface, so don't try it again
-                            this.unusedTypes.GetOrAdd(interfaceType, 0);
-                        }
+                        foundRule = true;
+                    }
+                    else
+                    {
+                        // there were no rules for this interface, so don't try it again
+                        this.unusedTypes.GetOrAdd(interfaceType, 0);
                     }
                 }
+            }
 
-                // repeat for any base type
-                foundRule = foundRule | ValidateTypeHierarchy(item, itemType.GetTypeInfo().BaseType, validationContext, validatedTypes, impliedProperty);
+            // repeat for any base type
+            foundRule = foundRule | ValidateTypeHierarchy(item, itemType.GetTypeInfo().BaseType, validationContext, validatedTypes, impliedProperty);
 
-                if (!foundRule)
-                {
-                    this.unusedTypes.GetOrAdd(itemType, 0);
-                }
+            if (!foundRule)
+            {
+                this.unusedTypes.GetOrAdd(itemType, 0);
             }
 
             return foundRule;

diff --git a/src/Microsoft.OData.Core/UrlValidation/ValidationEngine/PathSegmentValidator.cs b/src/Microsoft.OData.Core/UrlValidation/ValidationEngine/PathSegmentValidator.cs
@@ -271,7 +271,7 @@ private void ValidateItemAndType(ODataPathSegment segment)
             IEdmCollectionType collectionType = segment.EdmType as IEdmCollectionType;
             if (collectionType != null)
             {
-                ValidateItem(collectionType.ElementType);
+                ValidateItem(collectionType.ElementType.Definition);
             }
         }
     }

diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/UrlValidation/DeprecationTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/UrlValidation/DeprecationTests.cs
@@ -25,6 +25,7 @@ public class DeprecationTests
         [Theory]
         [InlineData(@"company", "name", "state")]
         [InlineData(@"company/employees", "employees")]
+        [InlineData(@"company/employees(1)/vehicles", "employees", "vehicle")]
         [InlineData(@"competitors", "competitors", "name", "state")]
         [InlineData(@"company/address", "state")]
         [InlineData(@"company/address/state", "state")]
@@ -64,6 +65,9 @@ public static void WithDeprecatedElementsGeneratesErrors(String request, params
                 object version;
                 Assert.True(error.ExtendedProperties.TryGetValue("Version", out version));
 
+                string elementNameAsString = elementName as string;
+                elementName =elementNameAsString.Substring(elementNameAsString.LastIndexOf('.') + 1);
+
                 Assert.Contains(elementName as string, expectedErrors);
                 Assert.Equal(date as Date?, expectedDate);
                 Assert.Equal(version as string, expectedDateAsString);
@@ -168,6 +172,25 @@ private static IEdmModel GetModel()
         <Property Name = ""firstName"" Type=""Edm.String""/>
         <Property Name = ""lastName"" Type=""Edm.String""/>
         <Property Name = ""title"" Type=""Edm.String""/>
+        <NavigationProperty Name = ""vehicles"" Type=""Collection(Jetsons.Models.vehicle)"" ContainsTarget=""true""/>
+     </EntityType>
+      <EntityType Name = ""vehicle"" >
+        <Key>
+          <PropertyRef Name=""license""/>
+        </Key>
+        <Property Name = ""license"" Type=""Edm.String"" Nullable=""false""/>
+        <Property Name = ""model"" Type=""Edm.String""/>
+        <Annotation Term = ""Core.Revisions"" >
+            <Collection>
+                <Record>
+                <PropertyValue Property=""Date"" Date=""2020-03-30""/>
+                <PropertyValue Property=""RemovalDate"" Date=""2022-03-30""/>
+                <PropertyValue Property=""Version"" String=""2020-03-30""/>
+                <PropertyValue Property = ""Kind"" EnumMember=""RevisionKind/Deprecated""/>
+                <PropertyValue Property = ""Description"" String=""'vehicle' is deprecated and will be retired on 2022-03-30.""/>
+                </Record>
+            </Collection>
+        </Annotation>
       </EntityType>
       <Action Name = ""ResetDataSource"" />
       <EntityContainer Name=""Container"">
@@ -192,4 +215,3 @@ private static IEdmModel GetModel()
 </edmx:Edmx>";
     }
 }
-