From 6d6ce5371e5557ba7bb7845e417ba0b4cd325cc7 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Wed, 28 Jun 2017 16:55:53 -0700 Subject: [PATCH 1/3] When building attribute cache, use field for WorkItemType rather than WorkItemType.Name property The new `IWorkItem` interface has a property named `WorkItemType`, which contains the name of the work item type for that work item. This is equivalent to the property `IWorkItem.Type.Name`, except that the work item store does not receive a query to load WIT meta data for just the name. --- src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs | 2 +- src/Qwiq.Mapper/Attributes/WorkItemLinksMapperStrategy.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs b/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs index 3865be94..a66c4f8a 100644 --- a/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs +++ b/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs @@ -143,7 +143,7 @@ private static IEnumerable PropertiesOnWorkItemCache(IPropertyInsp { // Composite key: work item type and target type - var workItemType = workItem.Type.Name; + var workItemType = workItem.WorkItemType; var key = new Tuple(workItemType, targetType.TypeHandle); return PropertiesThatExistOnWorkItem.GetOrAdd( diff --git a/src/Qwiq.Mapper/Attributes/WorkItemLinksMapperStrategy.cs b/src/Qwiq.Mapper/Attributes/WorkItemLinksMapperStrategy.cs index d7f474fd..e7072378 100644 --- a/src/Qwiq.Mapper/Attributes/WorkItemLinksMapperStrategy.cs +++ b/src/Qwiq.Mapper/Attributes/WorkItemLinksMapperStrategy.cs @@ -45,7 +45,7 @@ public WorkItemLinksMapperStrategy(IPropertyInspector inspector, IWorkItemStore { // Composite key: work item type and target type - var workItemType = workItem.Type.Name; + var workItemType = workItem.WorkItemType; var key = new Tuple(workItemType, targetType.TypeHandle); return PropertiesThatExistOnWorkItem.GetOrAdd( From 2f81d27a967ee2e7d4b0220e347541c734348f61 Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Thu, 29 Jun 2017 13:00:18 -0700 Subject: [PATCH 2/3] Provide better error message when unexpected values encountered for WIT In order to load the `Type` property on `WorkItem`, the `System.TeamProject` and `System.WorkItemType` fields must have values. Additionally, checks are performed against the project and work item types collections to verify a project and WIT are present, avoiding another KeyNotFoundException at runtime. The changes are targeted to the REST client because the SOAP client will lazily retrieve the fields when asked whereas REST will not. This change includes new integration tests to verify the functionality. --- src/Qwiq.Core.Rest/Query.cs | 46 ++++++++-- src/Qwiq.Core/WorkItem.cs | 20 ++--- .../Mapper/AttributeMapperTests.cs | 85 +++++++++++++++++++ ...WiqlAttributeMapperContextSpecification.cs | 66 ++++++++++++++ .../Qwiq.IntegrationTests.csproj | 6 ++ test/Qwiq.Mocks/MockWorkItem.cs | 20 ++--- 6 files changed, 218 insertions(+), 25 deletions(-) create mode 100644 test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs create mode 100644 test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs diff --git a/src/Qwiq.Core.Rest/Query.cs b/src/Qwiq.Core.Rest/Query.cs index fe0fc7cd..748866dc 100644 --- a/src/Qwiq.Core.Rest/Query.cs +++ b/src/Qwiq.Core.Rest/Query.cs @@ -92,9 +92,7 @@ private WorkItem CreateItemEager(TeamFoundation.WorkItemTracking.WebApi.Models.W { return new WorkItem( workItem, - // REVIEW: Allocate for reference type - _workItemStore.Projects[(string)workItem.Fields[CoreFieldRefNames.TeamProject]] - .WorkItemTypes[(string)workItem.Fields[CoreFieldRefNames.WorkItemType]], + LookUpWorkItemType(workItem), // REVIEW: Delegate allocation from method group LinkFunc); } @@ -103,8 +101,7 @@ private WorkItem CreateItemLazy(TeamFoundation.WorkItemTracking.WebApi.Models.Wo { IWorkItemType WorkItemTypeFactory() { - return _workItemStore.Projects[(string)workItem.Fields[CoreFieldRefNames.TeamProject]] - .WorkItemTypes[(string)workItem.Fields[CoreFieldRefNames.WorkItemType]]; + return LookUpWorkItemType(workItem); } return new WorkItem(workItem, new Lazy(WorkItemTypeFactory), LinkFunc); @@ -160,6 +157,45 @@ private List LoadWorkItemsEagerly(List RunkLinkQueryImpl() { // Eager loading for the link type ID (which is not returned by the REST API) causes ~250ms delay diff --git a/src/Qwiq.Core/WorkItem.cs b/src/Qwiq.Core/WorkItem.cs index d51f979c..724aaa93 100644 --- a/src/Qwiq.Core/WorkItem.cs +++ b/src/Qwiq.Core/WorkItem.cs @@ -27,19 +27,19 @@ public abstract class WorkItem : WorkItemCommon, IWorkItem, IRevisionInternal, I private bool _useFields = true; - protected internal WorkItem([NotNull] IWorkItemType type, [CanBeNull] Dictionary fields) + protected internal WorkItem([NotNull] IWorkItemType workItemType, [CanBeNull] Dictionary fields) : base(fields) { - Contract.Requires(type != null); + Contract.Requires(workItemType != null); - _type = type ?? throw new ArgumentNullException(nameof(type)); + _type = workItemType ?? throw new ArgumentNullException(nameof(workItemType)); } - protected internal WorkItem([NotNull] IWorkItemType type) + protected internal WorkItem([NotNull] IWorkItemType workItemType) { - Contract.Requires(type != null); + Contract.Requires(workItemType != null); - _type = type ?? throw new ArgumentNullException(nameof(type)); + _type = workItemType ?? throw new ArgumentNullException(nameof(workItemType)); } protected internal WorkItem([NotNull] Lazy type) @@ -48,11 +48,11 @@ protected internal WorkItem([NotNull] Lazy type) _lazyType = type; } - protected internal WorkItem([NotNull] IWorkItemType type, [NotNull] Func fieldCollectionFactory) + protected internal WorkItem([NotNull] IWorkItemType workItemType, [NotNull] Func fieldCollectionFactory) { - Contract.Requires(type != null); + Contract.Requires(workItemType != null); Contract.Requires(fieldCollectionFactory != null); - _type = type ?? throw new ArgumentNullException(nameof(type)); + _type = workItemType ?? throw new ArgumentNullException(nameof(workItemType)); _fieldFactory = fieldCollectionFactory ?? throw new ArgumentNullException(nameof(fieldCollectionFactory)); } @@ -110,7 +110,7 @@ public virtual string Keywords public virtual IEnumerable Revisions => throw new NotSupportedException(); - public virtual IWorkItemType Type => _type ?? _lazyType?.Value ?? throw new NotSupportedException(); + public virtual IWorkItemType Type => _type ?? _lazyType?.Value ?? throw new InvalidOperationException($"No value specified for {nameof(Type)}."); public abstract Uri Uri { get; } diff --git a/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs b/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs new file mode 100644 index 00000000..d5ab5892 --- /dev/null +++ b/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs @@ -0,0 +1,85 @@ +using System; +using System.Linq; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Should; + +namespace Microsoft.Qwiq.Mapper +{ + [TestClass] + public class + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified : + WiqlAttributeMapperContextSpecification + { + [TestMethod] + [TestCategory("localOnly")] + [TestCategory("REST")] + [ExpectedException(typeof(InvalidOperationException))] + public void An_InvalidOperationException_is_thrown() + { + Bugs.ToList().Count.ShouldEqual(1); + } + + protected override void ConfigureOptions() + { + WorkItemStore.Configuration.DefaultFields = new[] + { + //CoreFieldRefNames.TeamProject, + //CoreFieldRefNames.WorkItemType, + CoreFieldRefNames.Id, + CoreFieldRefNames.State + }; + } + } + + [TestClass] + public class + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_WorkItemType_specified : + WiqlAttributeMapperContextSpecification + { + [TestMethod] + [TestCategory("localOnly")] + [TestCategory("REST")] + [ExpectedException(typeof(InvalidOperationException))] + public void An_InvalidOperationException_is_thrown() + { + Bugs.ToList().Count.ShouldEqual(1); + } + + protected override void ConfigureOptions() + { + WorkItemStore.Configuration.DefaultFields = new[] + { + CoreFieldRefNames.TeamProject, + //CoreFieldRefNames.WorkItemType, + CoreFieldRefNames.Id, + CoreFieldRefNames.State + }; + } + } + + [TestClass] + public class + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified_Eager : + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified + { + protected override void ConfigureOptions() + { + base.ConfigureOptions(); + WorkItemStore.Configuration.LazyLoadingEnabled = false; + } + } + + [TestClass] + public class + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_WorkItemType_specified_Eager : + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_WorkItemType_specified + { + + + protected override void ConfigureOptions() + { + base.ConfigureOptions(); + WorkItemStore.Configuration.LazyLoadingEnabled = false; + } + } +} diff --git a/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs b/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs new file mode 100644 index 00000000..2d1ac45a --- /dev/null +++ b/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs @@ -0,0 +1,66 @@ +using Microsoft.Qwiq.Linq; +using Microsoft.Qwiq.Linq.Visitors; +using Microsoft.Qwiq.Mapper.Attributes; +using Microsoft.Qwiq.Tests.Common; +using System.Linq; + +namespace Microsoft.Qwiq.Mapper +{ + public abstract class WiqlAttributeMapperContextSpecification : TimedContextSpecification + { + private int[] _ids; + public IQueryable Bugs { get; set; } + protected IWorkItemStore WorkItemStore { get; private set; } + private Query Query { get; set; } + public override void Cleanup() + { + WorkItemStore?.Dispose(); + base.Cleanup(); + } + + public override void Given() + { + base.Given(); + + WorkItemStore = TimedAction(() => IntegrationSettings.CreateRestStore(), "REST", "WIS Create"); + + ConfigureOptions(); + + var pr = new PropertyReflector(); + var pi = new PropertyInspector(pr); + var attMapper = new AttributeMapperStrategy(pi); + var mapper = new WorkItemMapper(new IWorkItemMapperStrategy[] { attMapper }); + var translator = new WiqlTranslator(); + var pe = new PartialEvaluator(); + var qr = new QueryRewriter(); + var wqb = new WiqlQueryBuilder(translator, pe, qr); + var qp = new MapperTeamFoundationServerWorkItemQueryProvider( + WorkItemStore, + wqb, + mapper); + + Query = new Query(qp, wqb); + + _ids = new[] + { + 8663955 + }; + } + + public override void When() + { + Bugs = Query.Where(b => _ids.Contains(b.Id.Value)); + } + + protected abstract void ConfigureOptions(); + [WorkItemType("Bug")] + public class Bug : IIdentifiable + { + [FieldDefinition(CoreFieldRefNames.Id, true)] + public int? Id { get; set; } + + [FieldDefinition(CoreFieldRefNames.State)] + public string State { get; set; } + } + } +} \ No newline at end of file diff --git a/test/Qwiq.Integration.Tests/Qwiq.IntegrationTests.csproj b/test/Qwiq.Integration.Tests/Qwiq.IntegrationTests.csproj index ae4298e0..eb718ed2 100644 --- a/test/Qwiq.Integration.Tests/Qwiq.IntegrationTests.csproj +++ b/test/Qwiq.Integration.Tests/Qwiq.IntegrationTests.csproj @@ -195,6 +195,8 @@ + + @@ -264,6 +266,10 @@ {BE25CF2D-FA53-4455-85B1-4EEC1D979FB1} Qwiq.Mapper.Identity + + {016E8D93-4195-4639-BCD5-77633E8E1681} + Qwiq.Mapper + {B45C92B0-AC36-409D-86A5-5428C87384C3} Qwiq.Tests.Common diff --git a/test/Qwiq.Mocks/MockWorkItem.cs b/test/Qwiq.Mocks/MockWorkItem.cs index 8c469445..a59b69f5 100644 --- a/test/Qwiq.Mocks/MockWorkItem.cs +++ b/test/Qwiq.Mocks/MockWorkItem.cs @@ -82,31 +82,31 @@ public MockWorkItem([CanBeNull] string workItemType, [CanBeNull] params IField[] } } - public MockWorkItem([NotNull] IWorkItemType type, int id) - : this(type, new KeyValuePair(CoreFieldRefNames.Id, id)) + public MockWorkItem([NotNull] IWorkItemType workItemType, int id) + : this(workItemType, new KeyValuePair(CoreFieldRefNames.Id, id)) { Contract.Requires(id > 0); } - public MockWorkItem([NotNull] IWorkItemType type, int id, [CanBeNull] params KeyValuePair[] fieldValues) + public MockWorkItem([NotNull] IWorkItemType workItemType, int id, [CanBeNull] params KeyValuePair[] fieldValues) : this( - type, + workItemType, fieldValues?.Union(new[] { new KeyValuePair(CoreFieldRefNames.Id, id) }) .ToDictionary(k => k.Key, e => e.Value, StringComparer.OrdinalIgnoreCase) ?? new Dictionary(StringComparer.OrdinalIgnoreCase) { { CoreFieldRefNames.Id, id } }) { Contract.Requires(id > 0); } - public MockWorkItem([NotNull] IWorkItemType type, [CanBeNull] params KeyValuePair[] fieldValues) - : this(type, fieldValues?.ToDictionary(k => k.Key, e => e.Value, StringComparer.OrdinalIgnoreCase)) + public MockWorkItem([NotNull] IWorkItemType workItemType, [CanBeNull] params KeyValuePair[] fieldValues) + : this(workItemType, fieldValues?.ToDictionary(k => k.Key, e => e.Value, StringComparer.OrdinalIgnoreCase)) { } - public MockWorkItem([NotNull] IWorkItemType type, [CanBeNull] Dictionary fields = null) - : base(type, NormalizeFields(type, fields)) + public MockWorkItem([NotNull] IWorkItemType workItemType, [CanBeNull] Dictionary fields = null) + : base(workItemType, NormalizeFields(workItemType, fields)) { - SetFieldValue(type.FieldDefinitions[CoreFieldRefNames.WorkItemType], type.Name); - SetFieldValue(type.FieldDefinitions[CoreFieldRefNames.RevisedDate], new DateTime(9999, 1, 1, 0, 0, 0)); + SetFieldValue(workItemType.FieldDefinitions[CoreFieldRefNames.WorkItemType], workItemType.Name); + SetFieldValue(workItemType.FieldDefinitions[CoreFieldRefNames.RevisedDate], new DateTime(9999, 1, 1, 0, 0, 0)); if (IsNew) { From 5fae763b2acf514ad09bd8f6169e5883446a921a Mon Sep 17 00:00:00 2001 From: Richard Murillo Date: Thu, 29 Jun 2017 13:52:30 -0700 Subject: [PATCH 3/3] Update AttributeMapperStrategy to not require WIT.Fields When building the property cache for a work item type and model, the attribute mapper would verify the decorated property/field existed as a field in the WIT. This is not necessary, as an invalid field would return `null`. In REST, we use the object accessor to get the initial field value, which would try to load the WIT and fail. `WorkItemCore` has been updated to catch the `InvalidOperationException` thrown by the REST client and attempt to read values from that point forward using the dictionary supplied by the REST API. --- src/Qwiq.Core.Rest/WorkItem.cs | 7 +++ src/Qwiq.Core/WorkItem.cs | 4 ++ .../Attributes/AttributeMapperStrategy.cs | 6 +- .../Mapper/AttributeMapperTests.cs | 57 ++++++++++++------- ...WiqlAttributeMapperContextSpecification.cs | 3 + 5 files changed, 52 insertions(+), 25 deletions(-) diff --git a/src/Qwiq.Core.Rest/WorkItem.cs b/src/Qwiq.Core.Rest/WorkItem.cs index 61c9bfaa..3a5b2e11 100644 --- a/src/Qwiq.Core.Rest/WorkItem.cs +++ b/src/Qwiq.Core.Rest/WorkItem.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.Contracts; using System.Linq; @@ -143,6 +144,9 @@ protected override object GetValue(string name) if (string.IsNullOrEmpty(name)) return null; _item.Fields.TryGetValue(name, out object value); +#if DEBUG + Trace.WriteLine($"Get \'{name}\': {value.ToUsefulString()}"); +#endif return value; } @@ -151,6 +155,9 @@ protected override void SetValue(string name, object value) if (string.IsNullOrEmpty(name)) return; _item.Fields[name] = value; +#if DEBUG + Trace.WriteLine($"Set \'{name}\' to {value.ToUsefulString()}"); +#endif } [ContractInvariantMethod] diff --git a/src/Qwiq.Core/WorkItem.cs b/src/Qwiq.Core/WorkItem.cs index 724aaa93..f83f17ce 100644 --- a/src/Qwiq.Core/WorkItem.cs +++ b/src/Qwiq.Core/WorkItem.cs @@ -124,6 +124,10 @@ public virtual string Keywords { return Fields[name].Value; } + catch (InvalidOperationException ioex) when (ioex.Source == "Microsoft.Qwiq.Client.Rest") + { + _useFields = false; + } catch (NotSupportedException) { _useFields = false; diff --git a/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs b/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs index a66c4f8a..bd12148f 100644 --- a/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs +++ b/src/Qwiq.Mapper/Attributes/AttributeMapperStrategy.cs @@ -156,9 +156,9 @@ private static IEnumerable PropertiesOnWorkItemCache(IPropertyInsp property => new { property, fieldName = PropertyInfoFieldCache(inspector, property)?.FieldName }) .Where( - @t => - !string.IsNullOrEmpty(@t.fieldName) && workItem.Fields.Contains(@t.fieldName)) - .Select(@t => @t.property) + t => + !string.IsNullOrEmpty(t.fieldName)) + .Select(t => t.property) .ToList(); }); } diff --git a/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs b/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs index d5ab5892..cfe46865 100644 --- a/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs +++ b/test/Qwiq.Integration.Tests/Mapper/AttributeMapperTests.cs @@ -1,7 +1,7 @@ -using System; -using System.Linq; -using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.VisualStudio.TestTools.UnitTesting; using Should; +using System; +using System.Linq; namespace Microsoft.Qwiq.Mapper { @@ -13,8 +13,7 @@ public class [TestMethod] [TestCategory("localOnly")] [TestCategory("REST")] - [ExpectedException(typeof(InvalidOperationException))] - public void An_InvalidOperationException_is_thrown() + public void The_work_items_are_mapped_to_their_model() { Bugs.ToList().Count.ShouldEqual(1); } @@ -31,6 +30,27 @@ protected override void ConfigureOptions() } } + [TestClass] + public class + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified_Eager : + Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified + { + [TestMethod] + [TestCategory("localOnly")] + [TestCategory("REST")] + [ExpectedException(typeof(InvalidOperationException), "The fields '" + CoreFieldRefNames.TeamProject + "' and '" + CoreFieldRefNames.WorkItemType + "' are required to load the Type property.")] + public new void The_work_items_are_mapped_to_their_model() + { + Bugs.ToList().Count.ShouldEqual(1); + } + + protected override void ConfigureOptions() + { + base.ConfigureOptions(); + WorkItemStore.Configuration.LazyLoadingEnabled = false; + } + } + [TestClass] public class Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_WorkItemType_specified : @@ -39,8 +59,7 @@ public class [TestMethod] [TestCategory("localOnly")] [TestCategory("REST")] - [ExpectedException(typeof(InvalidOperationException))] - public void An_InvalidOperationException_is_thrown() + public void The_work_items_are_mapped_to_their_model() { Bugs.ToList().Count.ShouldEqual(1); } @@ -56,25 +75,19 @@ protected override void ConfigureOptions() }; } } - - [TestClass] - public class - Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified_Eager : - Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_TeamProject_and_WorkItemType_specified - { - protected override void ConfigureOptions() - { - base.ConfigureOptions(); - WorkItemStore.Configuration.LazyLoadingEnabled = false; - } - } - [TestClass] public class Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_WorkItemType_specified_Eager : Given_an_AttributeMapper_with_a_WorkItemStore_DefaultFields_without_WorkItemType_specified { - + [TestMethod] + [TestCategory("localOnly")] + [TestCategory("REST")] + [ExpectedException(typeof(InvalidOperationException), "The fields '" + CoreFieldRefNames.TeamProject + "' and '" + CoreFieldRefNames.WorkItemType + "' are required to load the Type property.")] + public new void The_work_items_are_mapped_to_their_model() + { + Bugs.ToList().Count.ShouldEqual(1); + } protected override void ConfigureOptions() { @@ -82,4 +95,4 @@ protected override void ConfigureOptions() WorkItemStore.Configuration.LazyLoadingEnabled = false; } } -} +} \ No newline at end of file diff --git a/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs b/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs index 2d1ac45a..cd6f54cc 100644 --- a/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs +++ b/test/Qwiq.Integration.Tests/Mapper/WiqlAttributeMapperContextSpecification.cs @@ -61,6 +61,9 @@ public class Bug : IIdentifiable [FieldDefinition(CoreFieldRefNames.State)] public string State { get; set; } + + [FieldDefinition("InvalidField")] + public string Invalid { get; set; } } } } \ No newline at end of file