From 47179f48292baed3d819914e09275299bea1a6b0 Mon Sep 17 00:00:00 2001 From: Ben Shillito Date: Fri, 13 Jan 2023 14:32:25 +0000 Subject: [PATCH] Merged PR 8063: FEAT: Return the correct upgrade message if the property 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 --- .../GenericEngine.cs | 43 +++++++++++ .../Services/MissingPropertyService.cs | 77 ++++++++++++++++++- .../Services/MissingPropertyServiceTests.cs | 72 +++++++++++++++++ 3 files changed, 191 insertions(+), 1 deletion(-) create mode 100644 FiftyOne.Pipeline.Engines.TestHelpers/GenericEngine.cs diff --git a/FiftyOne.Pipeline.Engines.TestHelpers/GenericEngine.cs b/FiftyOne.Pipeline.Engines.TestHelpers/GenericEngine.cs new file mode 100644 index 00000000..f25dc443 --- /dev/null +++ b/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 : AspectEngineBase + where T : IAspectData + { + public override string DataSourceTier { get; } + + public override string ElementDataKey { get; } + + public override IEvidenceKeyFilter EvidenceKeyFilter => new EvidenceKeyFilterWhitelist(new List()); + + public override IList Properties { get; } + + public GenericEngine( + ILogger> logger, + string tier, + string dataKey, + IList 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() + { + } + } +} diff --git a/FiftyOne.Pipeline.Engines/Services/MissingPropertyService.cs b/FiftyOne.Pipeline.Engines/Services/MissingPropertyService.cs index 9519b028..cae9a5e8 100644 --- a/FiftyOne.Pipeline.Engines/Services/MissingPropertyService.cs +++ b/FiftyOne.Pipeline.Engines/Services/MissingPropertyService.cs @@ -40,6 +40,14 @@ public class MissingPropertyService : IMissingPropertyService private static IMissingPropertyService _instance; private static object _lock = new object(); + /// + /// Used to store the results of looking up whether a property is available or not. + /// + /// The key is the engine type. The inner dictionary is keyed on property name. + /// + private static Dictionary> _propertyAvailable = + new Dictionary>(); + /// /// Get the singleton instance of this service. /// @@ -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; } + + /// + /// Return true if there is an explicit property getter for the name provided + /// in the data type returned by the engine. + /// + /// + /// + /// + private bool EngineDataContainsPropertyGetter(string propertyName, IAspectEngine engine) + { + // Get the property dictionary for this engine + Dictionary engineProperties; + if(_propertyAvailable.TryGetValue(engine.GetType(), out engineProperties) == false) + { + lock (_propertyAvailable) + { + if (_propertyAvailable.TryGetValue(engine.GetType(), out engineProperties) == false) + { + engineProperties = new Dictionary(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; + } } } diff --git a/Tests/FiftyOne.Pipeline.Engines.Tests/Services/MissingPropertyServiceTests.cs b/Tests/FiftyOne.Pipeline.Engines.Tests/Services/MissingPropertyServiceTests.cs index d7e2ddd3..b2bba410 100644 --- a/Tests/FiftyOne.Pipeline.Engines.Tests/Services/MissingPropertyServiceTests.cs +++ b/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; } + } + + /// + /// 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. + /// + [TestMethod] + public void MissingPropertyService_GetReason_UnknownUpgrade() + { + // Arrange + GenericEngine engine = new GenericEngine( + new Mock>>().Object, + "lite", + "test", + new List()); + + // Act + var result = _service.GetMissingPropertyReason("testProperty", engine); + + // Assert + Assert.AreEqual(MissingPropertyReason.DataFileUpgradeRequired, result.Reason); + } + + public class InheritedTestEngine : GenericEngine + { + public InheritedTestEngine( + ILogger> logger, + string tier, + string dataKey, + IList properties) + : base(logger, tier, dataKey, properties) + { + } + } + + /// + /// 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. + /// + [TestMethod] + public void MissingPropertyService_GetReason_UnknownUpgrade_Inherited() + { + // Arrange + InheritedTestEngine engine = new InheritedTestEngine( + new Mock>().Object, + "lite", + "test", + new List()); + + // Act + var result = _service.GetMissingPropertyReason("testProperty", engine); + + // Assert + Assert.AreEqual(MissingPropertyReason.DataFileUpgradeRequired, result.Reason); + } + /// /// Check that the missing property service works as expected when /// the property has been excluded from the result set