Merged PR 8063: FEAT: Return the correct upgrade message if the prope…

…rty was not found in the data file, but there is an explicit property getter for it in the data for the engine.

#4197

Related work items: #4197

FiftyOne.Pipeline.Engines.TestHelpers/GenericEngine.cs

@@ -0,0 +1,43 @@
+using FiftyOne.Pipeline.Core.Data;
+using FiftyOne.Pipeline.Core.FlowElements;
+using FiftyOne.Pipeline.Engines.Data;
+using FiftyOne.Pipeline.Engines.FlowElements;
+using Microsoft.Extensions.Logging;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace FiftyOne.Pipeline.Engines.TestHelpers
+{
+    public class GenericEngine<T> : AspectEngineBase<T, IAspectPropertyMetaData>
+        where T : IAspectData
+    {
+        public override string DataSourceTier { get; }
+
+        public override string ElementDataKey { get; }
+
+        public override IEvidenceKeyFilter EvidenceKeyFilter => new EvidenceKeyFilterWhitelist(new List<string>());
+
+        public override IList<IAspectPropertyMetaData> Properties { get; }
+
+        public GenericEngine(
+            ILogger<GenericEngine<T>> logger,
+            string tier,
+            string dataKey,
+            IList<IAspectPropertyMetaData> properties) :
+            base(logger, (p, e) => default(T))
+        {
+            DataSourceTier = tier;
+            ElementDataKey = dataKey;
+            Properties = properties;
+        }
+
+        protected override void ProcessEngine(IFlowData data, T aspectData)
+        {
+        }
+
+        protected override void UnmanagedResourcesCleanup()
+        {
+        }
+    }
+}

FiftyOne.Pipeline.Engines/Services/MissingPropertyService.cs

@@ -40,6 +40,14 @@ public class MissingPropertyService : IMissingPropertyService
         private static IMissingPropertyService _instance;
         private static object _lock = new object();
 
+        /// <summary>
+        /// Used to store the results of looking up whether a property is available or not.
+        /// <seealso cref="EngineDataContainsPropertyGetter"/>
+        /// The key is the engine type. The inner dictionary is keyed on property name.
+        /// </summary>
+        private static Dictionary<Type, Dictionary<string, bool>> _propertyAvailable = 
+            new Dictionary<Type, Dictionary<string, bool>>();
+
         /// <summary>
         /// Get the singleton instance of this service.
         /// </summary>
@@ -149,6 +157,14 @@ public MissingPropertyResult GetMissingPropertyReason(string propertyName, IAspe
                     reason = MissingPropertyReason.PropertyNotAccessibleWithResourceKey;
                 }
             }
+            else if (reason == MissingPropertyReason.Unknown &&
+                EngineDataContainsPropertyGetter(propertyName, engine))
+            {
+                // If the property meta data is not available, but the engine
+                // data class defines a getter, it's safe to assume that the data
+                // file needs upgrading.
+                reason = MissingPropertyReason.DataFileUpgradeRequired;
+            }
 
             // Build the message string to return to the caller.
             StringBuilder message = new StringBuilder();
@@ -163,7 +179,9 @@ public MissingPropertyResult GetMissingPropertyReason(string propertyName, IAspe
                     message.Append(
                         string.Format(CultureInfo.InvariantCulture,
                             Messages.MissingPropertyMessageDataUpgradeRequired,
-                            string.Join(",", property.DataTiersWherePresent),
+                            property == null ?
+                                "Unknown" :
+                                string.Join(",", property.DataTiersWherePresent),
                             engine.GetType().Name));
                     break;
                 case MissingPropertyReason.PropertyExcludedFromEngineConfiguration:
@@ -193,5 +211,62 @@ public MissingPropertyResult GetMissingPropertyReason(string propertyName, IAspe
             result.Reason = reason;
             return result;
         }
+
+        /// <summary>
+        /// Return true if there is an explicit property getter for the name provided
+        /// in the data type returned by the engine.
+        /// </summary>
+        /// <param name="propertyName"></param>
+        /// <param name="engine"></param>
+        /// <returns></returns>
+        private bool EngineDataContainsPropertyGetter(string propertyName, IAspectEngine engine)
+        {
+            // Get the property dictionary for this engine
+            Dictionary<string, bool> engineProperties;
+            if(_propertyAvailable.TryGetValue(engine.GetType(), out engineProperties) == false)
+            {
+                lock (_propertyAvailable)
+                {
+                    if (_propertyAvailable.TryGetValue(engine.GetType(), out engineProperties) == false)
+                    {
+                        engineProperties = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);
+                        _propertyAvailable.Add(engine.GetType(), engineProperties);
+                    }
+                }
+            }
+
+            // If we don't have a stored result in the dictionary for this property then use
+            // reflection to figure it out.
+            if (engineProperties.TryGetValue(propertyName, out var result) == false)
+            {
+                result = false;
+
+                foreach (var dataType in engine.GetType().GetInterfaces().SelectMany(i => i.GetGenericArguments())
+                    .Where(i => typeof(IAspectData).IsAssignableFrom(i)))
+                {
+                    if (dataType != null && result == false)
+                    {
+                        foreach (var property in dataType.GetProperties())
+                        {
+                            if (property.Name.Equals(propertyName, StringComparison.InvariantCultureIgnoreCase))
+                            {
+                                result = true;
+                                break;
+                            }
+                        }
+                    }
+                }
+
+                // Add the result to the property dictionary.
+                lock (engineProperties)
+                {
+                    if (engineProperties.ContainsKey(propertyName) == false)
+                    {
+                        engineProperties.Add(propertyName, result);
+                    }
+                }
+            }
+            return result;
+        }
     }
 }

Tests/FiftyOne.Pipeline.Engines.Tests/Services/MissingPropertyServiceTests.cs

@@ -20,9 +20,16 @@
  * such notice(s) shall fulfill the requirements of that article.
  * ********************************************************************* */
 
+using FiftyOne.Pipeline.Core.Data;
+using FiftyOne.Pipeline.Core.FlowElements;
+using FiftyOne.Pipeline.Core.TypedMap;
+using FiftyOne.Pipeline.Engines.Caching;
+using FiftyOne.Pipeline.Engines.Configuration;
 using FiftyOne.Pipeline.Engines.Data;
 using FiftyOne.Pipeline.Engines.FlowElements;
 using FiftyOne.Pipeline.Engines.Services;
+using FiftyOne.Pipeline.Engines.TestHelpers;
+using Microsoft.Extensions.Logging;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Moq;
 using System;
@@ -62,6 +69,71 @@ public void MissingPropertyService_GetReason_Upgrade()
             Assert.AreEqual(MissingPropertyReason.DataFileUpgradeRequired, result.Reason);
         }
 
+        public interface ITestData : IAspectData
+        {
+            public string TestProperty { get; }
+        }
+
+        /// <summary>
+        /// Check that when the property meta data is not present, so the data tier
+        /// cannot be checked, that an upgrade message is returned correctly.
+        /// This will be the case if the property is not present in the data file,
+        /// but there is an explicit getter for it in the data class.
+        /// </summary>
+        [TestMethod]
+        public void MissingPropertyService_GetReason_UnknownUpgrade()
+        {
+            // Arrange
+            GenericEngine<ITestData> engine = new GenericEngine<ITestData>(
+                new Mock<ILogger<GenericEngine<ITestData>>>().Object,
+                "lite",
+                "test",
+                new List<IAspectPropertyMetaData>());
+
+            // Act
+            var result = _service.GetMissingPropertyReason("testProperty", engine);
+
+            // Assert
+            Assert.AreEqual(MissingPropertyReason.DataFileUpgradeRequired, result.Reason);
+        }
+
+        public class InheritedTestEngine : GenericEngine<ITestData>
+        {
+            public InheritedTestEngine(
+                ILogger<GenericEngine<ITestData>> logger,
+                string tier,
+                string dataKey,
+                IList<IAspectPropertyMetaData> properties)
+                : base(logger, tier, dataKey, properties)
+            {
+            }
+        }
+
+        /// <summary>
+        /// Check that when the property meta data is not present, so the data tier
+        /// cannot be checked, that an upgrade message is returned correctly.
+        /// This will be the case if the property is not present in the data file,
+        /// but there is an explicit getter for it in the data class.
+        /// This test uses a further inherited class to check that the data type
+        /// can still be worked out if the generic data type is burried.
+        /// </summary>
+        [TestMethod]
+        public void MissingPropertyService_GetReason_UnknownUpgrade_Inherited()
+        {
+            // Arrange
+            InheritedTestEngine engine = new InheritedTestEngine(
+                new Mock<ILogger<InheritedTestEngine>>().Object,
+                "lite",
+                "test",
+                new List<IAspectPropertyMetaData>());
+
+            // Act
+            var result = _service.GetMissingPropertyReason("testProperty", engine);
+
+            // Assert
+            Assert.AreEqual(MissingPropertyReason.DataFileUpgradeRequired, result.Reason);
+        }
+
         /// <summary>
         /// Check that the missing property service works as expected when
         /// the property has been excluded from the result set