diff --git a/OnTopic.Tests/BindingModels/NestedReferenceAttributeTopicBindingModel.cs b/OnTopic.Tests/BindingModels/NestedReferenceAttributeTopicBindingModel.cs new file mode 100644 index 00000000..9ce23ae0 --- /dev/null +++ b/OnTopic.Tests/BindingModels/NestedReferenceAttributeTopicBindingModel.cs @@ -0,0 +1,26 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.ViewModels.BindingModels; + +namespace OnTopic.Tests.BindingModels; + +/*============================================================================================================================== +| BINDING MODEL: NESTED REFERENCE ATTRIBUTE TOPIC +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a minimal implementation of a custom topic binding model with both a scalar value and a reference property, for +/// use as an item within a collection. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +public class NestedReferenceAttributeTopicBindingModel : AttributeDescriptorTopicBindingModel { + + public NestedReferenceAttributeTopicBindingModel(string key) : base(key, "TextAttributeDescriptor") { } + + public AssociatedTopicBindingModel? BaseTopic { get; set; } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ITopicLazyLoadableTest.cs b/OnTopic.Tests/ITopicLazyLoadableTest.cs index e90a8ee6..664ff5fa 100644 --- a/OnTopic.Tests/ITopicLazyLoadableTest.cs +++ b/OnTopic.Tests/ITopicLazyLoadableTest.cs @@ -3,6 +3,7 @@ | Client Ignia, LLC | Project Topics Library \=============================================================================================================================*/ +using OnTopic.Associations; using OnTopic.Repositories; using OnTopic.Tests.TestDoubles; using Xunit; @@ -19,6 +20,11 @@ namespace OnTopic.Tests; [ExcludeFromCodeCoverage] public class ITopicLazyLoadableTest { + /*============================================================================================================================ + | PROPERTY: CANCELLATION TOKEN + \---------------------------------------------------------------------------------------------------------------------------*/ + private static CancellationToken CancellationToken => TestContext.Current.CancellationToken; + /*============================================================================================================================ | TEST: IS LOADED: NON-RECURSIVE: IGNORES UNLOADED CHILDREN \---------------------------------------------------------------------------------------------------------------------------*/ @@ -161,7 +167,39 @@ public void IsLoaded_NotLoadedChildren_NeverTriggersLoad() { [Fact] public void EnsureLoaded_NullResolver_DoesNotThrow() { var topic = new Topic("Topic", "Page"); - ((ITopicLazyLoadable)topic).EnsureLoaded(TopicPayload.All); + ((ITopicLazyLoadable)topic).EnsureLoaded(TopicPayload.All, CancellationToken); + } + + /*============================================================================================================================ + | TEST: ENSURE LOADED: CLEARED RELATIONSHIP: DOES NOT RESURRECT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Registers a deferred entry, then calls on that key. Confirms that a subsequent for never reaches the ; since already purged the deferred entry, there is nothing left to resolve, and the + /// previously cleared relationship isn't resurrected. + /// + [Fact] + public async Task EnsureLoaded_ClearedRelationship_DoesNotResurrect() { + + var topic = new Topic("Test", "Page", null, 1); + var rawLoadable = (ITopicLazyLoadable)topic; + var rawTopic = (ITopicBackingAccessor)topic; + var loader = new TrackingTopicLazyLoader(); + + // Set up and clear via the backing accessor so this doesn't itself trigger a load once LoadState flips to NotLoaded; the + // loader is stamped afterward, ahead of the explicit EnsureLoaded() call below + rawTopic.Relationships.Deferred.SetValue("Related", 999); + rawTopic.Relationships.Clear("Related"); + rawLoadable.Loader = loader; + + await rawLoadable.EnsureLoaded(TopicPayload.Relationships, CancellationToken); + + Assert.False(loader.WasCalled); + Assert.Empty(rawTopic.Relationships.GetValues("Related")); + } /*============================================================================================================================ diff --git a/OnTopic.Tests/ReverseTopicMappingServiceTest.cs b/OnTopic.Tests/ReverseTopicMappingServiceTest.cs index 6b352628..c1caa9a5 100644 --- a/OnTopic.Tests/ReverseTopicMappingServiceTest.cs +++ b/OnTopic.Tests/ReverseTopicMappingServiceTest.cs @@ -15,6 +15,7 @@ using OnTopic.TestDoubles.Metadata; using OnTopic.Tests.BindingModels; using OnTopic.Tests.Fixtures; +using OnTopic.Tests.TestDoubles; using Xunit; namespace OnTopic.Tests; @@ -325,6 +326,112 @@ public async Task Map_NestedTopics_ReturnsMappedTopic() { } + /*============================================================================================================================ + | TEST: MAP: NESTED TOPICS: STAGGERED COMPLETION: PRESERVES SOURCE ORDER + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a backed by a whose + /// per-item topic reference lookups resolve out of call order: The first-declared item resolves slowest, the last-declared + /// item resolves instantly. Confirms nested topics still land in the binding model's source order, since maps and adds each child sequentially rather than racing completions. + /// + [Fact] + public async Task Map_NestedTopics_StaggeredCompletion_PreservesSourceOrder() { + + // Declared in call order; delays fall in reverse, so the first-added item resolves last + List<(string UniqueKey, TimeSpan Delay)> attributes = [ + ("Root:Configuration:ContentTypes:Attributes:Key", TimeSpan.FromMilliseconds(120)), + ("Root:Configuration:ContentTypes:Attributes:ContentType", TimeSpan.FromMilliseconds(60)), + ("Root:Configuration:ContentTypes:Attributes:Title", TimeSpan.Zero) + ]; + + var delaysByKey = attributes.ToDictionary(attribute => attribute.UniqueKey, attribute => attribute.Delay); + var topicRepository = new StaggeredStubTopicRepository(delaysByKey); + var mappingService = new ReverseTopicMappingService(topicRepository); + var bindingModel = new ContentTypeDescriptorTopicBindingModel("Test"); + + for (var i = 0; i < attributes.Count; i++) { + bindingModel.Attributes.Add( + new NestedReferenceAttributeTopicBindingModel($"Attribute{i + 1}") { + BaseTopic = new() { + UniqueKey = attributes[i].UniqueKey + } + } + ); + } + + var topic = new ContentTypeDescriptor("Test", "ContentTypeDescriptor"); + var target = (ContentTypeDescriptor?)await mappingService.MapAsync(bindingModel, topic); + var container = target?.Children.GetValue("Attributes"); + + Assert.NotNull(container); + Assert.Equal( + Enumerable.Range(1, attributes.Count).Select(i => $"Attribute{i}"), + container.Children.Select(child => child.Key) + ); + + } + + /*============================================================================================================================ + | TEST: MAP: SPARSE TOPIC: FILLS EXTENDED ATTRIBUTES ONCE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Maps a scalar-only binding model onto a target stamped with a whose are . Confirms warms exactly once at the start of the map, rather than leaving it to the attribute + /// collection's own synchronous autoload. + /// + [Fact] + public async Task Map_ScalarProperties_FillsExtendedAttributesOnce() { + + var bindingModel = new TextAttributeTopicBindingModel("Test") { + ContentType = "TextAttributeDescriptor", + DefaultValue = "World" + }; + + var target = new TextAttributeDescriptor("Test", "TextAttributeDescriptor"); + var loader = new TrackingTopicLazyLoader(markLoaded: true); + + ((ITopicLazyLoadable)target).Loader = loader; + target.Attributes.LoadState = LoadState.NotLoaded; + + _ = await _mappingService.MapAsync(bindingModel, target); + + Assert.Equal(1, loader.CallCount); + Assert.Equal(TopicPayload.ExtendedAttributes, loader.Payloads[0]); + + } + + /*============================================================================================================================ + | TEST: MAP: NESTED TOPICS: FILLS CONTAINER CHILDREN + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Maps a nested-topic binding model onto a target whose Attributes container is stamped with its own and left , even though the target's own are already loaded. Confirms warms the container + /// independently before PopulateTargetCollectionAsync probes its existing children. + /// + [Fact] + public async Task Map_NestedTopics_FillsContainerChildren() { + + var bindingModel = new ContentTypeDescriptorTopicBindingModel("Test"); + + bindingModel.Attributes.Add(new TextAttributeTopicBindingModel("Attribute1")); + + var target = new ContentTypeDescriptor("Test", "ContentTypeDescriptor"); + var container = new Topic("Attributes", "List", target); + var containerLoader = new TrackingTopicLazyLoader(markLoaded: true); + + ((ITopicLazyLoadable)container).Loader = containerLoader; + container.Children.LoadState = LoadState.NotLoaded; + + _ = (ContentTypeDescriptor?)await _mappingService.MapAsync(bindingModel, target); + + Assert.Equal(1, containerLoader.CallCount); + Assert.Equal(TopicPayload.Children, containerLoader.Payloads[0]); + + } + /*============================================================================================================================ | TEST: MAP: TOPIC REFERENCES: RETURNS MAPPED TOPIC \---------------------------------------------------------------------------------------------------------------------------*/ diff --git a/OnTopic.Tests/TestDoubles/BlockingStubLazyLoadingTopicRepository.cs b/OnTopic.Tests/TestDoubles/BlockingStubLazyLoadingTopicRepository.cs index d3f0ef7e..d8aca6ca 100644 --- a/OnTopic.Tests/TestDoubles/BlockingStubLazyLoadingTopicRepository.cs +++ b/OnTopic.Tests/TestDoubles/BlockingStubLazyLoadingTopicRepository.cs @@ -76,6 +76,16 @@ internal sealed class BlockingStubLazyLoadingTopicRepository: StubLazyLoadingTop /// public void ReleaseEnsureLoadedGate() => _ensureLoadedGate?.SetResult(); + /*============================================================================================================================ + | METHOD: FAULT ENSURE LOADED GATE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Faults a suspended call "armed" via with the supplied + /// , so a test can simulate a lazy load that throws while a second pass awaits the same entry. + /// + /// The exception to surface from the suspended call. + public void FaultEnsureLoadedGate(Exception exception) => _ensureLoadedGate?.SetException(exception); + /*============================================================================================================================ | METHOD: LOAD \---------------------------------------------------------------------------------------------------------------------------*/ diff --git a/OnTopic.Tests/TestDoubles/FakeViewModelLookupService.cs b/OnTopic.Tests/TestDoubles/FakeViewModelLookupService.cs index fc379c2d..04f2ccde 100644 --- a/OnTopic.Tests/TestDoubles/FakeViewModelLookupService.cs +++ b/OnTopic.Tests/TestDoubles/FakeViewModelLookupService.cs @@ -35,12 +35,18 @@ public FakeViewModelLookupService() { Add(typeof(AmbiguousRelationTopicViewModel)); Add(typeof(AscendentSpecializedTopicViewModel)); Add(typeof(AscendentTopicViewModel)); + Add(typeof(CircularConstructorTopicViewModel)); Add(typeof(CircularTopicViewModel)); + Add(typeof(ConcurrentExpansionRootTopicViewModel)); + Add(typeof(ConcurrentExpansionSharedTopicViewModel)); + Add(typeof(ConcurrentReferenceTopicViewModel)); Add(typeof(ConstructedTopicViewModel)); Add(typeof(DefaultValueTopicViewModel)); Add(typeof(DescendentSpecializedTopicViewModel)); Add(typeof(DescendentTopicViewModel)); Add(typeof(DisableMappingTopicViewModel)); + Add(typeof(ExpansionParentTopicViewModel)); + Add(typeof(ExpansionSharedTopicViewModel)); Add(typeof(FallbackViewModel)); Add(typeof(FilteredTopicViewModel)); Add(typeof(FlattenChildrenTopicViewModel)); @@ -58,6 +64,7 @@ public FakeViewModelLookupService() { Add(typeof(RelationWithChildrenTopicViewModel)); Add(typeof(RequiredObjectTopicViewModel)); Add(typeof(RequiredTopicViewModel)); + Add(typeof(SharedConcurrentTopicViewModel)); Add(typeof(TopicReferenceAttributeDescriptorTopicViewModel)); Add(typeof(TopicReferenceTopicViewModel)); diff --git a/OnTopic.Tests/TestDoubles/RendezvousTopicLazyLoader.cs b/OnTopic.Tests/TestDoubles/RendezvousTopicLazyLoader.cs new file mode 100644 index 00000000..08c6fd1e --- /dev/null +++ b/OnTopic.Tests/TestDoubles/RendezvousTopicLazyLoader.cs @@ -0,0 +1,139 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.Repositories; + +namespace OnTopic.Tests.TestDoubles; + +/*============================================================================================================================== +| CLASS: RENDEZVOUS TOPIC LAZY LOADER +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// An that suspends each call until a fixed number +/// of concurrent passes have arrived, then releases them together, so a stress test can force two concurrent mapping passes +/// to modify a shared collection at the same time without timing hacks. Conceptually, this is an asynchronous, cyclic for a fixed number of mapping passes. +/// +/// +/// +/// Unlike a repository-backed lazy loader, this performs no fetching: The stress test wires the shared topic's associations +/// before mapping, so only needs to a) "rendezvous" the concurrent passes and +/// b) mark as so the base pass's nested-topic search, which +/// reads the autoloading getter, doesn't re-enter this loader. The rendezvous rearms after +/// each release, so a single instance serves every repetition. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +[ExcludeFromCodeCoverage] +internal sealed class RendezvousTopicLazyLoader: ITopicLazyLoader { + + /*============================================================================================================================ + | PRIVATE FIELDS + \---------------------------------------------------------------------------------------------------------------------------*/ + private readonly object _lock = new(); + private readonly int _participantCount; + private readonly TimeSpan _timeout; + private TaskCompletionSource _gate = new(TaskCreationOptions.RunContinuationsAsynchronously); + private int _arrivals; + + /*============================================================================================================================ + | CONSTRUCTOR + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Instantiates a new instance of the that releases once passes have arrived. + /// + /// The number of concurrent passes to await before releasing them together. + /// + /// The number of seconds an arrived pass waits for the others before throwing, guarding against a hang if the expected + /// concurrency never materializes. + /// + public RendezvousTopicLazyLoader(int participantCount = 2, int timeoutSeconds = 10) { + _participantCount = participantCount; + _timeout = TimeSpan.FromSeconds(timeoutSeconds); + } + + /*============================================================================================================================ + | METHOD: ENSURE LOADED + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + async Task ITopicLazyLoader.EnsureLoaded(Topic topic, TopicPayload payload, CancellationToken cancellationToken) { + + // Suspend until the concurrent passes rendezvous, so they resume together and race on the shared collection + await Rendezvous(cancellationToken).ConfigureAwait(false); + + // Mark children Loaded so the base pass's nested-topic probe, which reads the autoloading Topic.Children getter, doesn't + // re-enter this loader. Relationships needs no such treatment: Its targets are preloaded, so its (derived) LoadState is + // already Loaded and so relationship never autoload. + ((ITopicBackingAccessor)topic).Children.LoadState = LoadState.Loaded; + + } + + /*============================================================================================================================ + | METHOD: RENDEZVOUS + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Suspends the caller until passes have arrived, then releases them together and re-arms + /// for the next batch. + /// + /// A token used to cancel the wait. + private Task Rendezvous(CancellationToken cancellationToken) { + + /*-------------------------------------------------------------------------------------------------------------------------- + | Register arrival and, if last, re-arm the gate for the next batch + \-------------------------------------------------------------------------------------------------------------------------*/ + TaskCompletionSource gate; + bool release; + lock (_lock) { + gate = _gate; + release = ++_arrivals >= _participantCount; + if (release) { + _arrivals = 0; + _gate = new(TaskCreationOptions.RunContinuationsAsynchronously); + } + } + + /*-------------------------------------------------------------------------------------------------------------------------- + | The last arrival releases the batch and proceeds without waiting + \-------------------------------------------------------------------------------------------------------------------------*/ + if (release) { + gate.TrySetResult(); + return Task.CompletedTask; + } + + /*-------------------------------------------------------------------------------------------------------------------------- + | Earlier arrivals wait for the last, throwing on timeout so a broken assumption fails loudly instead of hanging + \-------------------------------------------------------------------------------------------------------------------------*/ + return AwaitGate(gate, cancellationToken); + + } + + /*============================================================================================================================ + | METHOD: AWAIT GATE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Waits for to be released, throwing a if the batch never + /// completes. + /// + /// The gate to wait on for the current batch. + /// A token used to cancel the wait. + private async Task AwaitGate(TaskCompletionSource gate, CancellationToken cancellationToken) { + + // Race the gate against a timeout, so an unexpected participant count can't hang the test suite + var completed = await Task.WhenAny(gate.Task, Task.Delay(_timeout, cancellationToken)).ConfigureAwait(false); + + // Surface a failed rendezvous as an exception rather than proceeding with a corrupt result + if (completed != gate.Task) { + throw new TimeoutException( + $"The rendezvous timed out after {_timeout.TotalSeconds:0} seconds waiting for {_participantCount} concurrent " + + $"passes; the expected concurrency did not occur." + ); + } + + } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/TestDoubles/StaggeredStubTopicRepository.cs b/OnTopic.Tests/TestDoubles/StaggeredStubTopicRepository.cs new file mode 100644 index 00000000..d87ae28e --- /dev/null +++ b/OnTopic.Tests/TestDoubles/StaggeredStubTopicRepository.cs @@ -0,0 +1,68 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.Repositories; +using OnTopic.TestDoubles; + +namespace OnTopic.Tests.TestDoubles; + +/*============================================================================================================================== +| CLASS: STAGGERED STUB TOPIC REPOSITORY +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// A that delays by a per-key , letting a test invert completion order relative to call order. +/// +/// +/// +/// This is similar to , except that it staggers calls to , not . +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +[ExcludeFromCodeCoverage] +internal sealed class StaggeredStubTopicRepository: StubTopicRepository { + + /*============================================================================================================================ + | PRIVATE FIELDS + \---------------------------------------------------------------------------------------------------------------------------*/ + private readonly IReadOnlyDictionary _delaysByKey; + + /*============================================================================================================================ + | CONSTRUCTOR + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Initializes a new instance of the with a delay for each unique key that + /// should complete out of call order. + /// + /// A map of unique topic key to the delay that should precede its resolution. + public StaggeredStubTopicRepository(IReadOnlyDictionary delaysByKey) { + _delaysByKey = delaysByKey; + } + + /*============================================================================================================================ + | METHOD: LOAD + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + public override async Task Load( + string uniqueKey, + Topic? referenceTopic = null, + TopicPayload payload = TopicPayload.None, + int depth = 0 + ) { + + // Delay resolution of this key, if configured + if (_delaysByKey.TryGetValue(uniqueKey, out var delay) && delay > TimeSpan.Zero) { + await Task.Delay(delay).ConfigureAwait(false); + } + + // Delegate to the base implementation to perform the actual lookup + return await base.Load(uniqueKey, referenceTopic, payload, depth).ConfigureAwait(false); + + } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/TestDoubles/StaggeredTopicLazyLoader.cs b/OnTopic.Tests/TestDoubles/StaggeredTopicLazyLoader.cs new file mode 100644 index 00000000..12d2e0a2 --- /dev/null +++ b/OnTopic.Tests/TestDoubles/StaggeredTopicLazyLoader.cs @@ -0,0 +1,41 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.Repositories; + +namespace OnTopic.Tests.TestDoubles; + +/*============================================================================================================================== +| CLASS: STAGGERED TOPIC LAZY LOADER +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// An that suspends for a fixed before completing, so a test can attach different instances to sibling topics and force their loads to +/// genuinely complete out of source order. +/// +/// +/// +/// This is similar to , except that it staggers calls to , not . +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +[ExcludeFromCodeCoverage] +internal sealed class StaggeredTopicLazyLoader(TimeSpan delay): ITopicLazyLoader { + + /*============================================================================================================================ + | METHOD: ENSURE LOADED + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + async Task ITopicLazyLoader.EnsureLoaded(Topic topic, TopicPayload payload, CancellationToken cancellationToken) { + if (delay > TimeSpan.Zero) { + await Task.Delay(delay, cancellationToken).ConfigureAwait(false); + } + ((ITopicBackingAccessor)topic).Children.LoadState = LoadState.Loaded; + } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/TestDoubles/TrackingTopicLazyLoader.cs b/OnTopic.Tests/TestDoubles/TrackingTopicLazyLoader.cs index e340a958..793f9d67 100644 --- a/OnTopic.Tests/TestDoubles/TrackingTopicLazyLoader.cs +++ b/OnTopic.Tests/TestDoubles/TrackingTopicLazyLoader.cs @@ -13,8 +13,20 @@ namespace OnTopic.Tests.TestDoubles; /// /// A minimal spy that records whether it was invoked, without performing any actual loading. /// +/// +/// By default, this doesn't mutate , so a stamped topic remains +/// even after a call, letting tests assert that a specific code path either suppresses or triggers autoloading. Pass +/// to instead simulate a real loader's fill, marking the requested payload on each call, when a test needs to confirm that a caller warms a payload exactly once +/// rather than relying on this spy's inertness to inflate the count. +/// [ExcludeFromCodeCoverage] -internal sealed class TrackingTopicLazyLoader : ITopicLazyLoader { +internal sealed class TrackingTopicLazyLoader(bool markLoaded = false) : ITopicLazyLoader { + + /*============================================================================================================================ + | PRIVATE FIELDS + \---------------------------------------------------------------------------------------------------------------------------*/ + private readonly List _payloads = []; /*============================================================================================================================ | PROPERTY: WAS CALLED @@ -22,14 +34,34 @@ internal sealed class TrackingTopicLazyLoader : ITopicLazyLoader { /// /// Returns if was invoked. /// - public bool WasCalled { get; private set; } + public bool WasCalled => _payloads.Count > 0; + + /*============================================================================================================================ + | PROPERTY: CALL COUNT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Returns the number of times was invoked. + /// + public int CallCount => _payloads.Count; + + /*============================================================================================================================ + | PROPERTY: PAYLOADS + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Returns the passed to each invocation of , in + /// call order. + /// + public IReadOnlyList Payloads => _payloads; /*============================================================================================================================ | METHOD: ENSURE LOADED \---------------------------------------------------------------------------------------------------------------------------*/ /// Task ITopicLazyLoader.EnsureLoaded(Topic topic, TopicPayload payload, CancellationToken cancellationToken) { - WasCalled = true; + _payloads.Add(payload); + if (markLoaded) { + ((ITopicLazyLoadable)topic).SetLoadState(payload, LoadState.Loaded); + } return Task.CompletedTask; } diff --git a/OnTopic.Tests/TopicMappingServiceTest.cs b/OnTopic.Tests/TopicMappingServiceTest.cs index 4c761d5a..b44fc322 100644 --- a/OnTopic.Tests/TopicMappingServiceTest.cs +++ b/OnTopic.Tests/TopicMappingServiceTest.cs @@ -6,16 +6,20 @@ using System.ComponentModel.DataAnnotations; using System.Globalization; using OnTopic.Data.Caching; +using OnTopic.Lookup; using OnTopic.Mapping; using OnTopic.Mapping.Internal; using OnTopic.Metadata; using OnTopic.Repositories; using OnTopic.TestDoubles; +using OnTopic.TestDoubles.LazyLoading; using OnTopic.TestDoubles.Metadata; using OnTopic.Tests.Entities; using OnTopic.Tests.Fixtures; +using OnTopic.Tests.TestDoubles; using OnTopic.Tests.ViewModels; using OnTopic.Tests.ViewModels.Metadata; +using OnTopic.ViewModels; using Xunit; namespace OnTopic.Tests; @@ -35,6 +39,7 @@ public class TopicMappingServiceTest { \---------------------------------------------------------------------------------------------------------------------------*/ readonly ITopicRepository _topicRepository; readonly ITopicMappingService _mappingService; + readonly ITypeLookupService _typeLookupService; /*============================================================================================================================ | CONSTRUCTOR @@ -60,6 +65,7 @@ public TopicMappingServiceTest(TopicInfrastructureFixture f \-------------------------------------------------------------------------------------------------------------------------*/ _topicRepository = fixture.CachedTopicRepository; _mappingService = fixture.MappingService; + _typeLookupService = fixture.TypeLookupService; } @@ -321,6 +327,48 @@ public async Task Map_AttributeDictionary_ReturnsNewModel() { } + /*============================================================================================================================ + | TEST: MAP: ATTRIBUTE DICTIONARY: NOT LOADED: RETURNS EXTENDED ATTRIBUTES + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a and maps a sparse topic whose extended attributes are still to a view model with an constructor. Confirms the extended + /// attribute is present in the mapped result, i.e., that + /// autoloads the blob rather than silently omitting it by enumerating only resident attributes. + /// + [Fact] + public async Task Map_AttributeDictionary_NotLoaded_ReturnsExtendedAttributes() { + + var records = new StubLazyLoadingTopicRepositoryBuilder() + .AddTopic( + 221, + "Sparse", + "Page", + null, + indexedAttributes : new Dictionary { + ["Title"] = "Value", + ["ShortTitle"] = "Short Title", + ["Subtitle"] = "Subtitle", + ["MetaTitle"] = "Meta Title", + ["MetaDescription"] = "Meta Description" + }, + extendedAttributes : new Dictionary { + ["MappedProperty"] = "Mapped Value" + } + ) + .Build(); + + var stub = new StubLazyLoadingTopicRepository(records); + var topic = await stub.Load("Root:Sparse"); + + Contract.Assume(topic); + + var target = await _mappingService.MapAsync(topic); + + Assert.Equal("Mapped Value", target?.MappedProperty); + + } + /*============================================================================================================================ | TEST: MAP: CONSTRUCTOR: RETURNS NEW MODEL \---------------------------------------------------------------------------------------------------------------------------*/ @@ -421,6 +469,196 @@ await _mappingService.MapAsync(topic).ConfigureAwait( } + /*============================================================================================================================ + | TEST: MAP: CONSTRUCTOR (RECORD): RETURNS NEW MODEL + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a and maps a positional record whose constructor accepts a + /// non-cyclic topic reference, confirming that the reference is resolved and the record is constructed as expected. + /// + /// + /// As this is a mapping of a positional record that carries constructor parameters, it also confirms that the + /// primary constructor is correctly selected and its parameters mapped. + /// + [Fact] + public async Task Map_ConstructorRecord_ReturnsNewModel() { + + var topic = new Topic("Parent", "CircularConstructor", null, 1); + var child = new Topic("Child", "CircularConstructor", null, 2); + + topic.References.SetValue("Self", child); + + var target = await _mappingService.MapAsync(topic); + + Assert.NotNull(target); + Assert.Equal("Parent", target.Key); + Assert.NotNull(target.Self); + Assert.Equal("Child", target.Self.Key); + Assert.Null(target.Self.Self); + + } + + /*============================================================================================================================ + | TEST: MAP: CONSTRUCTOR (RECORD): THROWS EXCEPTION + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a and maps a positional record whose constructor references the + /// topic being mapped, and confirms that this circular constructor reference is detected and a is thrown. + /// + [Fact] + public async Task Map_ConstructorRecord_ThrowsException() { + + var topic = new Topic("Topic", "CircularConstructor", null, 1); + + topic.References.SetValue("Self", topic); + + await Assert.ThrowsAsync(async () => + await _mappingService.MapAsync(topic).ConfigureAwait(false) + ); + + } + + /*============================================================================================================================ + | TEST: MAP: CONCURRENT SIBLINGS: RETURNS SHARED INSTANCE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Confirms that two concurrent branches mapping the same topic to the same type within a single pass share one instance, + /// rather than the second branch mistaking the first's still-initializing entry for a circular constructor reference. + /// + /// + /// Uses , which suspends inside its own EnsureLoaded until + /// released, to hold the branch that wins construction of the shared model mid-constructor, guaranteeing the second branch + /// reaches the still-initializing entry and thus must await its completion. The shared topic is loaded through the + /// repository so it is stamped for lazy loading, and its constructor's collection parameter actually engages the gate; the + /// root is a plain whose in-memory references cannot trip the gate before the shared model is + /// constructed. Asserting the in-process task has not completed proves the first branch actually suspended, so the test + /// cannot pass without exercising the await path. + /// + [Fact] + public async Task Map_ConcurrentSiblings_ReturnsSharedInstance() { + + var (inner, cache, mappingService) = CreateGatedMappingService(); + + var shared = await cache.Load("Web"); + + Contract.Assume(shared); + + var root = new Topic("ConcurrentRoot", "Container", null, 5); + + root.References.SetValue("FirstReference", shared); + root.References.SetValue("SecondReference", shared); + + // "Arm" the gate so the branch that constructs the shared model suspends mid-constructor + inner.ArmEnsureLoadedGate(); + + var mapTask = mappingService.MapAsync(root); + + // Prove the first branch is genuinely suspended, so the second must await its completion + Assert.False(mapTask.IsCompleted); + + inner.ReleaseEnsureLoadedGate(); + + var result = await mapTask; + + Assert.NotNull(result); + Assert.NotNull(result.FirstReference); + Assert.NotNull(result.SecondReference); + Assert.Same(result.FirstReference, result.SecondReference); + + } + + /*============================================================================================================================ + | TEST: MAP: CONCURRENT SIBLINGS: OBSERVES FAULT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Confirms that when the branch constructing a shared model faults, a concurrent branch awaiting the same entry observes + /// the exception rather than hanging on a mapping that will never complete. + /// + /// + /// Uses the same setup as , but releases the gate with a fault so + /// the constructing branch throws while the second branch is awaiting the entry's completion. That the map throws, rather + /// than deadlocking, is what confirms the faulted entry releases its waiter. + /// + [Fact] + public async Task Map_ConcurrentSiblings_ObservesFault() { + + var (inner, cache, mappingService) = CreateGatedMappingService(); + + var shared = await cache.Load("Web"); + + Contract.Assume(shared); + + var root = new Topic("ConcurrentRoot", "Container", null, 5); + + root.References.SetValue("FirstReference", shared); + root.References.SetValue("SecondReference", shared); + + // "Arm" the gate so the branch that constructs the shared model suspends mid-constructor + inner.ArmEnsureLoadedGate(); + + var mapTask = mappingService.MapAsync(root); + + // Prove the first branch is genuinely suspended, so the second must await its completion + Assert.False(mapTask.IsCompleted); + + inner.FaultEnsureLoadedGate(new InvalidOperationException("Simulated load failure.")); + + await Assert.ThrowsAsync(async () => await mapTask.ConfigureAwait(false)); + + } + + /*============================================================================================================================ + | TEST: MAP: CONCURRENT SHARED COLLECTION: POPULATES DETERMINISTICALLY + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Stress test confirming that two concurrent passes expanding the same shared model with disjoint associations, each + /// populating the same collection from a different source, deterministically produce the union of both sources without + /// corrupting the shared list. + /// + /// + /// A parent references one shared topic twice, with disjoint sets ( and ). Whichever pass wins + /// construction populates the shared list from one source; + /// the other expands it from the other, so the result should always be the union of both, regardless of which pass wins. A + /// holds the two passes at the collection warm-up until both arrive, then releases + /// them together, so their list mutations genuinely overlap. Repeated many times to give the race a chance to manifest; + /// without the shared-list mutation guard, the concurrent adds corrupt the list, dropping items or throwing. + /// + [Fact] + public async Task Map_ConcurrentSharedCollection_PopulatesDeterministically() { + + const int relationshipCount = 12; + const int incomingCount = 12; + const int repetitions = 100; + + var loader = new RendezvousTopicLazyLoader(participantCount: 2); + + for (var repetition = 0; repetition < repetitions; repetition++) { + + var root = BuildConcurrentExpansionGraph(loader, relationshipCount, incomingCount, out var shared); + + // Guard against an incomplete setup: Both sources must be loaded before mapping; read them via the backing accessor so + // the precondition check doesn't itself trip the loader's rendezvous and hang + var backing = (ITopicBackingAccessor)shared; + Assert.Equal(relationshipCount, backing.Relationships.GetValues("Related").Count); + Assert.Equal(incomingCount, shared.IncomingRelationships.GetValues("Related").Count); + + var result = await _mappingService.MapAsync(root); + + Assert.NotNull(result); + Assert.NotNull(result.RelationshipsView); + Assert.NotNull(result.IncomingView); + + // Both references resolve to the one shared instance, whose list holds the union of both sources + Assert.Same(result.RelationshipsView, result.IncomingView); + Assert.NotNull(result.RelationshipsView.Related); + Assert.Equal(relationshipCount + incomingCount, result.RelationshipsView.Related.Count); + + } + + } + /*============================================================================================================================ | TEST: MAP: DISABLED PROPERTY: RETURNS NULL \---------------------------------------------------------------------------------------------------------------------------*/ @@ -589,7 +827,7 @@ public async Task Map_AlternateAttributeKey_ReturnsMappedModel() { \---------------------------------------------------------------------------------------------------------------------------*/ /// /// Establishes a and then confirms that it is returned via . + /// "MappedTopicCache.TryGetValue"/>. /// [Fact] public void MappedTopicCache_TryGetValue_ReturnsEntry() { @@ -612,7 +850,7 @@ public void MappedTopicCache_TryGetValue_ReturnsEntry() { \---------------------------------------------------------------------------------------------------------------------------*/ /// /// Establishes a and then confirms that it is not returned via if the doesn't match. + /// .TryGetValue"/> if the doesn't match. /// [Fact] public void MappedTopicCache_TryGetValue_ReturnsNull() { @@ -715,8 +953,6 @@ public void MappedTopicCacheEntry_GetMissingAssociations_ReturnsDifference() { var difference = cacheEntry.GetMissingAssociations(associations); - cacheEntry.AddMissingAssociations(difference); - Assert.True(difference.HasFlag(AssociationTypes.References)); Assert.False(difference.HasFlag(AssociationTypes.Children)); Assert.False(difference.HasFlag(AssociationTypes.Parents)); @@ -724,24 +960,128 @@ public void MappedTopicCacheEntry_GetMissingAssociations_ReturnsDifference() { } /*============================================================================================================================ - | TEST: MAPPED TOPIC CACHE ENTRY: ADD MISSING ASSOCIATIONS: SETS UNION + | TEST: MAPPED TOPIC CACHE ENTRY: ADD MISSING ASSOCIATIONS: RETURNS NEWLY ADDED \---------------------------------------------------------------------------------------------------------------------------*/ /// - /// Establishes a with a set of , and then confirms that - /// its correctly extends the missing - /// associations. + /// Establishes a and then confirms that two overlapping calls to its return disjoint flags (each reporting only what it + /// newly added) whose union is the missing set, and that the recorded + /// reflect both calls. /// + /// + /// Each association may only be added once. Even though both requests include , + /// only the first call adds it; the second call sees it as already recorded and returns only the remainder. This is what + /// ensures two concurrent passes map disjoint associations rather than both mapping the overlap. + /// [Fact] - public void MappedTopicCacheEntry_AddMissingAssociations_SetsUnion() { + public void MappedTopicCacheEntry_AddMissingAssociations_ReturnsNewlyAdded() { var cacheEntry = new MappedTopicCacheEntry() { Associations = AssociationTypes.Children }; - var associations = AssociationTypes.Children | AssociationTypes.Parents; - cacheEntry.AddMissingAssociations(associations); + var firstResult = cacheEntry.AddMissingAssociations(AssociationTypes.Children | AssociationTypes.Parents); + var secondResult = cacheEntry.AddMissingAssociations(AssociationTypes.Parents | AssociationTypes.References); + + Assert.Equal(AssociationTypes.Parents, firstResult); + Assert.Equal(AssociationTypes.References, secondResult); + Assert.Equal(AssociationTypes.Children | AssociationTypes.Parents | AssociationTypes.References, cacheEntry.Associations); + + } + + /*============================================================================================================================ + | TEST: MAPPED TOPIC CACHE ENTRY: IS INITIALIZING: REFLECTS COMPLETION STATE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a set of instances and confirms that is derived from the completion state: A fresh entry is initializing, a + /// completed entry is not, and a faulted entry remains initializing so an awaiting pass observes the fault instead of a + /// null instance. + /// + [Fact] + public void MappedTopicCacheEntry_IsInitializing_ReflectsCompletionState() { + + var completed = new MappedTopicCacheEntry(); + var faulted = new MappedTopicCacheEntry(); + + Assert.True(completed.IsInitializing); + Assert.True(faulted.IsInitializing); + + completed.Complete(new EmptyViewModel(), AssociationTypes.None); + faulted.Fault(new InvalidOperationException()); + + Assert.False(completed.IsInitializing); + Assert.True(faulted.IsInitializing); + + // Observe the faulted task so its exception isn't surfaced as unobserved + Assert.NotNull(faulted.Completion.Exception); + + } + + /*============================================================================================================================ + | TEST: MAPPED TOPIC CACHE ENTRY: COMPLETION: SETTLES ON COMPLETE OR FAULT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a pair of instances and confirms that resolves when the entry is completed, and throws the recorded exception when the + /// entry is faulted, so that a second pass awaiting the entry is always released rather than left hanging. + /// + [Fact] + public async Task MappedTopicCacheEntry_Completion_SettlesOnCompleteOrFault() { + + var completed = new MappedTopicCacheEntry(); + var faulted = new MappedTopicCacheEntry(); + + completed.Complete(new EmptyViewModel(), AssociationTypes.None); + faulted.Fault(new InvalidOperationException("Construction failed.")); - Assert.Equal(AssociationTypes.Children | AssociationTypes.Parents, cacheEntry.Associations); + Assert.True(completed.Completion.IsCompletedSuccessfully); + await Assert.ThrowsAsync(async () => await faulted.Completion.ConfigureAwait(false)); + + } + + /*============================================================================================================================ + | TEST: MAPPED TOPIC CACHE ENTRY: COMPLETE: RETAINS FIRST RESULT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a and confirms that only the first call takes effect: A later duplicate registration is + /// ignored, keeping both the and its stable. + /// + [Fact] + public void MappedTopicCacheEntry_Complete_RetainsFirstResult() { + + var entry = new MappedTopicCacheEntry(); + var first = new EmptyViewModel(); + var second = new EmptyViewModel(); + + entry.Complete(first, AssociationTypes.Children); + entry.Complete(second, AssociationTypes.Parents); + + Assert.Same(first, entry.MappedTopic); + Assert.Equal(AssociationTypes.Children, entry.Associations); + + } + + /*============================================================================================================================ + | TEST: MAP PATH: CONTAINS: DETECTS PAIRS ON PATH + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a two-frame and confirms that recognizes a + /// topic and view model type pair anywhere on the path, whether at the current frame or an ancestor, while rejecting pairs + /// that are not on the path, including one whose topic identifier matches but whose type does not. + /// + [Fact] + public void MapPath_Contains_DetectsPairsOnPath() { + + var root = new MapPath(1, typeof(EmptyViewModel), null); + var child = new MapPath(2, typeof(KeyOnlyTopicViewModel), root); + + Assert.True(child.Contains(2, typeof(KeyOnlyTopicViewModel))); + Assert.True(child.Contains(1, typeof(EmptyViewModel))); + Assert.False(child.Contains(3, typeof(EmptyViewModel))); + Assert.False(child.Contains(1, typeof(KeyOnlyTopicViewModel))); } @@ -932,6 +1272,42 @@ public async Task Map_Children_ReturnsMappedModel() { )); } + /*============================================================================================================================ + | TEST: MAP: CHILDREN: STAGGERED COMPLETION: PRESERVES SOURCE ORDER + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Establishes a with children whose mapping tasks are forced to complete in reverse of + /// source order, and tests that the mapped collection nonetheless preserves source order. + /// + /// + /// Each child is stamped with its own and left + /// for , so mapping it genuinely awaits a delay before completing. The first child gets + /// the longest delay and the last gets none, so completion order is the reverse of source order; if collection population + /// added results in completion order rather than source order, this would come back reversed. + /// + [Fact] + public async Task Map_Children_StaggeredCompletion_PreservesSourceOrder() { + + var topic = new Topic("Test", "Descendent"); + var childKeys = new[] { "ChildTopic1", "ChildTopic2", "ChildTopic3", "ChildTopic4" }; + + for (var index = 0; index < childKeys.Length; index++) { + var child = new Topic(childKeys[index], "Descendent", topic); + var delay = TimeSpan.FromMilliseconds((childKeys.Length - index) * 25); + ((ITopicLazyLoadable)child).Loader = new StaggeredTopicLazyLoader(delay); + ((ITopicBackingAccessor)child).Children.LoadState = LoadState.NotLoaded; + } + + var target = await _mappingService.MapAsync(topic); + + Assert.NotNull(target); + Assert.Equal(childKeys.Length, target.Children.Count); + for (var index = 0; index < childKeys.Length; index++) { + Assert.Equal(childKeys[index], target.Children[index].Key); + } + + } + /*============================================================================================================================ | TEST: MAP: WITH DISABLED: SKIPS DISABLED \---------------------------------------------------------------------------------------------------------------------------*/ @@ -1036,6 +1412,36 @@ public async Task Map_MapAs_ReturnsRelationships() { } + /*============================================================================================================================ + | TEST: MAP: RELATIONSHIP ONLY: DOES NOT FILL CHILDREN + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Maps a , whose only collection is explicitly typed , against a , and confirms that no + /// synchronous fill occurs. Prior to fixing GetSourceCollectionAsync's + /// collection probes, the NestedTopics probe's signature (source.Children.Contains) was evaluated + /// unconditionally when the argument was constructed, silently triggering the lazy loading of the children regardless of + /// which collection type the view model actually requested. + /// + [Fact] + public async Task Map_RelationshipOnly_DoesNotFillChildren() { + + var stub = new StubLazyLoadingTopicRepository(); + var cache = new CachedTopicRepository(stub); + var typeLookupService = new CompositeTypeLookupService(new TopicViewModelLookupService(), new FakeViewModelLookupService()); + var mappingService = new TopicMappingService(cache, typeLookupService); + + var topic = await cache.Load("Root:Web:Web_0"); + + Contract.Assume(topic); + + var target = await mappingService.MapAsync(topic); + + Assert.NotNull(target); + Assert.Equal(0, stub.GetFetchCount(topic.Id, TopicPayload.Children)); + + } + /*============================================================================================================================ | TEST: MAP: TOPIC REFERENCES AS ATTRIBUTE: RETURNS MAPPED MODEL \---------------------------------------------------------------------------------------------------------------------------*/ @@ -1287,6 +1693,78 @@ public async Task Map_CachedTopic_ReturnsProgressiveReference() { } + /*============================================================================================================================ + | TEST: MAP: EXPANSION PASS: DOES NOT DUPLICATE NESTED TOPICS + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Maps an , which encounters the same source topic twice with disjoint + /// associations, confirming that the second (expansion) pass does not re-append the ungated nested-topics collection that + /// the initial pass already populated. + /// + /// + /// Reliability rests on the eager repository mapping the two collections sequentially, so the encounters are strictly + /// ordered: The first builds and fills the cached view model, and the second, requesting a disjoint association, hits the + /// cache and runs an expansion pass (rather than a second, concurrent initial pass). is then filled once by the ungated nested-topics probe on the initial pass + /// and skipped on the expansion pass, so the count is 2 whichever collection reflection maps first. + /// + [Fact] + public async Task Map_ExpansionPass_DoesNotDuplicateNestedTopics() { + + var parent = new Topic("Parent", "ExpansionParent", null, 700); + var shared = new Topic("Shared", "ExpansionShared", parent, 701); + var categories = new Topic("Categories", "List", shared, 702); + _ = new Topic("Category1", "KeyOnly", categories, 703); + _ = new Topic("Category2", "KeyOnly", categories, 704); + + parent.Relationships.SetValue("Related", shared); + + var target = await _mappingService.MapAsync(parent); + var mappedShared = target?.Children.FirstOrDefault(); + + //Assert.Same confirms both collections resolved to the same cached instance, so the second reach was a cache hit and, given + //the disjoint associations, ran an expansion pass + Assert.NotNull(mappedShared); + Assert.Same(mappedShared, target?.Related.FirstOrDefault()); + Assert.Equal(2, mappedShared.Categories.Count); + + } + + /*============================================================================================================================ + | TEST: MAP: EXPANSION PASS: DOES NOT REMAP COMPATIBLE PROPERTY + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Maps an , which encounters the same source topic twice with disjoint + /// associations, and confirms that the second (expansion) pass does not reassign the compatible property that the initial pass already mapped. + /// + /// + /// Reliability rests on the eager repository mapping the two collections sequentially, so the encounters are strictly + /// ordered: The first builds and fills the cached view model, and the second, requesting a disjoint association, hits the + /// cache and runs an expansion pass (rather than a second, concurrent initial pass). The compatible is then assigned once on the initial pass and skipped on the expansion pass, so + /// is 1 whichever collection reflection maps first. + /// + [Fact] + public async Task Map_ExpansionPass_DoesNotRemapCompatibleProperty() { + + var parent = new Topic("Parent", "ExpansionParent", null, 710); + var shared = new Topic("Shared", "ExpansionShared", parent, 711); + + parent.Relationships.SetValue("Related", shared); + + var target = await _mappingService.MapAsync(parent); + var mappedShared = target?.Children.FirstOrDefault(); + + //Assert.Same confirms both collections resolved to the same cached instance, so the second reach was a cache hit and, given + //the disjoint associations, ran an expansion pass rather than passing vacuously + Assert.NotNull(mappedShared); + Assert.Same(mappedShared, target?.Related.FirstOrDefault()); + Assert.Equal("Shared", mappedShared.Key); + Assert.Equal(1, mappedShared.KeyMapCount); + + } + /*============================================================================================================================ | TEST: MAP: CIRCULAR REFERENCE: RETURNS MAPPED PARENT \---------------------------------------------------------------------------------------------------------------------------*/ @@ -1333,6 +1811,34 @@ public async Task Map_FilterByCollectionType_ReturnsFilteredCollection() { } + /*============================================================================================================================ + | TEST: MAP: DESCENDENT: DOES NOT FILL RELATIONSHIPS + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Maps a , whose only collection is , against a + /// , and confirms that no synchronous + /// fill occurs. The inverse of : The relationship probe's method call + /// (source.Relationships.Contains) was likewise evaluated unconditionally. + /// + [Fact] + public async Task Map_Descendent_DoesNotFillRelationships() { + + var stub = new StubLazyLoadingTopicRepository(); + var cache = new CachedTopicRepository(stub); + var typeLookupService = new CompositeTypeLookupService(new TopicViewModelLookupService(), new FakeViewModelLookupService()); + var mappingService = new TopicMappingService(cache, typeLookupService); + + var topic = await cache.Load("Root:Web:Web_0"); + + Contract.Assume(topic); + + var target = await mappingService.MapAsync(topic); + + Assert.NotNull(target); + Assert.Equal(0, stub.GetFetchCount(topic.Id, TopicPayload.Relationships)); + + } + /*============================================================================================================================ | TEST: MAP: GETTER METHODS: MAP METHOD OUTPUT \---------------------------------------------------------------------------------------------------------------------------*/ @@ -1668,4 +2174,95 @@ public async Task Map_CachedTopic_ReturnsUniqueReferencePerType() { public static TopicViewModel? GetChildTopic(IEnumerable? topicCollection, string key) => topicCollection?.FirstOrDefault((t) => t.Key.StartsWith(key, StringComparison.Ordinal)); + /*============================================================================================================================ + | METHOD: CREATE GATED MAPPING SERVICE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Assembles a fresh over a , + /// returning the service together with that gated repository and the wrapping it. + /// + /// + /// Each call returns a new, isolated set so concurrent-mapping tests can "arm", release, or fault their own gate without + /// interfering with one another. The stateless is reused, so + /// only the gated repository is constructed per test. + /// + /// The gated repository, the cache over it, and the mapping service. + private ( + BlockingStubLazyLoadingTopicRepository Repository, + CachedTopicRepository Cache, + ITopicMappingService MappingService + ) CreateGatedMappingService() { + var inner = new BlockingStubLazyLoadingTopicRepository(); + var cache = new CachedTopicRepository(inner); + return (inner, cache, new TopicMappingService(cache, _typeLookupService)); + } + + /*============================================================================================================================ + | METHOD: BUILD CONCURRENT EXPANSION GRAPH + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Builds an in-memory graph for : A parent that + /// references a single topic twice, that topic having + /// outgoing relationships and incoming relationships under the key Related. + /// + /// + /// Everything is preloaded, so performs no fetching; only the topic is + /// stamped for lazy loading, since only its EnsureLoaded needs to "rendezvous" the two passes. Its is left so the mapper's collection warm-up actually calls + /// the loader, which then marks it before the base pass's nested-topic probe reads it. The + /// relationships are already . The shared topic deliberately has no Related child, so + /// the nested-topic probe never displaces the relationship and incoming-relationship sources. + /// + /// The lazy loader to stamp on the shared topic. + /// The number of outgoing relationships to wire under Related. + /// The number of incoming relationships to wire under Related. + /// The shared topic referenced twice by the returned parent. + /// The parent topic to map. + private static Topic BuildConcurrentExpansionGraph( + ITopicLazyLoader loader, + int relationshipCount, + int incomingCount, + out Topic shared + ) { + + /*-------------------------------------------------------------------------------------------------------------------------- + | Establish the parent and the shared target + \-------------------------------------------------------------------------------------------------------------------------*/ + var identity = 1; + var root = new Topic("ConcurrentExpansionRoot", "ConcurrentExpansionRoot", null, identity++); + shared = new Topic("Shared", "ConcurrentExpansionShared", null, identity++); + var backing = (ITopicBackingAccessor)shared; + + /*-------------------------------------------------------------------------------------------------------------------------- + | Wire the shared target's outgoing relationships (the first source for the shared collection) + \-------------------------------------------------------------------------------------------------------------------------*/ + for (var index = 0; index < relationshipCount; index++) { + backing.Relationships.SetValue("Related", new($"Relationship_{index}", "KeyOnly", null, identity++)); + } + + /*-------------------------------------------------------------------------------------------------------------------------- + | Wire the shared target's incoming relationships (the second source), via each origin's reciprocal outgoing relationship + \-------------------------------------------------------------------------------------------------------------------------*/ + for (var index = 0; index < incomingCount; index++) { + var origin = new Topic($"Incoming_{index}", "KeyOnly", null, identity++); + origin.Relationships.SetValue("Related", shared); + } + + /*-------------------------------------------------------------------------------------------------------------------------- + | Stamp the shared target and leave children NotLoaded so the collection warm-up calls the loader exactly once per pass; the + | relationships are already Loaded (no deferred targets), so the relationship probe reads them without autoloading + \-------------------------------------------------------------------------------------------------------------------------*/ + ((ITopicLazyLoadable)shared).Loader = loader; + backing.Children.LoadState = LoadState.NotLoaded; + + /*-------------------------------------------------------------------------------------------------------------------------- + | Reference the shared target twice from the parent, so both references map it concurrently + \-------------------------------------------------------------------------------------------------------------------------*/ + root.References.SetValue("RelationshipsView", shared); + root.References.SetValue("IncomingView", shared); + + return root; + + } + } //Class \ No newline at end of file diff --git a/OnTopic.Tests/TopicRelationshipMultiMapTest.cs b/OnTopic.Tests/TopicRelationshipMultiMapTest.cs index bc563d74..4b5604e5 100644 --- a/OnTopic.Tests/TopicRelationshipMultiMapTest.cs +++ b/OnTopic.Tests/TopicRelationshipMultiMapTest.cs @@ -491,6 +491,30 @@ public void Clear_NoTopics_IsNotDirty() { } + /*============================================================================================================================ + | TEST: CLEAR: DEFERRED ENTRIES: REMOVES DEFERRED ENTRIES AND IS DIRTY + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Registers a entry with no corresponding target and + /// calls , confirming that the deferred entry is purged and reports true, even though no target topic was removed. + /// + [Fact] + public void Clear_DeferredEntries_RemovesDeferredEntriesAndIsDirty() { + + var topic = new Topic("Test", "Page", null, 1); + var relationships = new TopicRelationshipMultiMap(topic); + + relationships.Deferred.SetValue("Related", 999); + relationships.Deferred.SetValue("Other", 998); + relationships.Clear("Related"); + + Assert.False(relationships.Deferred.Remove("Related")); + Assert.True(relationships.Deferred.Remove("Other")); + Assert.True(relationships.IsDirty()); + + } + /*============================================================================================================================ | TEST: SET VALUE: MARK NOT DIRTY: IS NOT DIRTY \---------------------------------------------------------------------------------------------------------------------------*/ diff --git a/OnTopic.Tests/ViewModels/AttributeDictionaryConstructorTopicViewModel.cs b/OnTopic.Tests/ViewModels/AttributeDictionaryConstructorTopicViewModel.cs index 942493f2..e6d800f8 100644 --- a/OnTopic.Tests/ViewModels/AttributeDictionaryConstructorTopicViewModel.cs +++ b/OnTopic.Tests/ViewModels/AttributeDictionaryConstructorTopicViewModel.cs @@ -13,7 +13,10 @@ namespace OnTopic.Tests.ViewModels; /// Provides a strongly-typed data transfer object for testing a constructor with a . /// /// -/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// and are decorated with +/// so they can only be populated via the constructor, not the reflection-based property +/// mapper's fallback pass; this isolates tests to the constructor-dictionary path they're meant to exercise. This is a sample +/// class intended for test purposes only; it is not designed for use in a production environment. /// public record AttributeDictionaryConstructorTopicViewModel: PageTopicViewModel { @@ -38,7 +41,10 @@ public AttributeDictionaryConstructorTopicViewModel() { } /*============================================================================================================================ | PROPERTIES \---------------------------------------------------------------------------------------------------------------------------*/ + [DisableMapping] public string? MappedProperty { get; init; } + + [DisableMapping] public string? UnmappedProperty { get; init; } diff --git a/OnTopic.Tests/ViewModels/CircularConstructorTopicViewModel.cs b/OnTopic.Tests/ViewModels/CircularConstructorTopicViewModel.cs new file mode 100644 index 00000000..ded41685 --- /dev/null +++ b/OnTopic.Tests/ViewModels/CircularConstructorTopicViewModel.cs @@ -0,0 +1,34 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.Mapping; + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: CIRCULAR CONSTRUCTOR TOPIC +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a strongly typed data transfer object, implemented as a positional record, for testing constructor mapping +/// of a topic reference that may form a circular reference. +/// +/// +/// +/// Unlike , which expresses its circular reference through settable properties, this +/// model maps its reference through a positional constructor parameter on a record. This allows the to be exercised for two distinct behaviors: A non-cyclic reference should map successfully, +/// while a true self-reference should be detected as a constructor cycle and throw a , +/// since a partially constructed instance cannot be returned from a constructor. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +/// The key of the mapped topic. +/// An optional reference to another . +public record CircularConstructorTopicViewModel( + string Key, + [Include(AssociationTypes.References)] CircularConstructorTopicViewModel? Self +); \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/ConcurrentExpansionRootTopicViewModel.cs b/OnTopic.Tests/ViewModels/ConcurrentExpansionRootTopicViewModel.cs new file mode 100644 index 00000000..55d1c093 --- /dev/null +++ b/OnTopic.Tests/ViewModels/ConcurrentExpansionRootTopicViewModel.cs @@ -0,0 +1,36 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: CONCURRENT EXPANSION ROOT +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a parent view model that references the same source topic twice with disjoint associations, forcing two +/// concurrent mapping passes over the shared target. +/// +/// +/// +/// Both and resolve to the same source topic, but request +/// disjoint associations via . Because both reference properties are mapped concurrently (via +/// the property-level Task.WhenAll), one pass constructs the shared instance while the other expands it, each +/// populating the target's list from a different source; this +/// is the scenario that the SetCollectionValueAsync() and PopulateTargetCollectionAsync() locks protect against. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +public class ConcurrentExpansionRootTopicViewModel { + + [Include(AssociationTypes.Relationships)] + public ConcurrentExpansionSharedTopicViewModel? RelationshipsView { get; set; } + + [Include(AssociationTypes.IncomingRelationships)] + public ConcurrentExpansionSharedTopicViewModel? IncomingView { get; set; } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/ConcurrentExpansionSharedTopicViewModel.cs b/OnTopic.Tests/ViewModels/ConcurrentExpansionSharedTopicViewModel.cs new file mode 100644 index 00000000..b14393d5 --- /dev/null +++ b/OnTopic.Tests/ViewModels/ConcurrentExpansionSharedTopicViewModel.cs @@ -0,0 +1,41 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: CONCURRENT EXPANSION SHARED +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a shared target for two concurrent mapping passes with disjoint associations, exposing a single collection () populated from two different sources. +/// +/// +/// +/// This is the referenced view model in the concurrency stress test: A single source topic is referenced twice from a +/// parent , once with and +/// once with . Both passes populate the same +/// list: One from the source's outgoing relationships, the other from its incoming relationships, thus exercising the locks +/// on SetCollectionValueAsyn() and PopulateTargetCollectionAsync(). The property is left nullable and settable so the +/// mapper both creates the backing list (SetCollectionValueAsyn()) and adds to it (PopulateTargetCollectionAsync()). +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +[SuppressMessage( + "Usage", + "CA2227:Collection properties should be read only", + Justification = "This view model intentionally exposes a settable, nullable collection property so the TopicMappingService's list-creation runs, which the concurrency test relies on to establish a creation race." +)] +public class ConcurrentExpansionSharedTopicViewModel { + + public string? Key { get; set; } + + [Collection("Related")] + public Collection? Related { get; set; } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/ConcurrentReferenceTopicViewModel.cs b/OnTopic.Tests/ViewModels/ConcurrentReferenceTopicViewModel.cs new file mode 100644 index 00000000..8308cc70 --- /dev/null +++ b/OnTopic.Tests/ViewModels/ConcurrentReferenceTopicViewModel.cs @@ -0,0 +1,36 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.Mapping; + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: CONCURRENT REFERENCE TOPIC +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a strongly typed data transfer object with two topic references, both intended to resolve to the same shared . +/// +/// +/// +/// Both references are mapped as properties, so the resolves them concurrently within a +/// single mapping pass. When both point at the same topic, this drives two branches to map that topic to the same type at +/// once, which is a supported sibling concurrency scenario. The pins the mapped view model +/// type so the scenario does not depend on the shared topic's content type. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +public class ConcurrentReferenceTopicViewModel { + + [MapAs(typeof(SharedConcurrentTopicViewModel))] + public SharedConcurrentTopicViewModel? FirstReference { get; set; } + + [MapAs(typeof(SharedConcurrentTopicViewModel))] + public SharedConcurrentTopicViewModel? SecondReference { get; set; } + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/ExpansionParentTopicViewModel.cs b/OnTopic.Tests/ViewModels/ExpansionParentTopicViewModel.cs new file mode 100644 index 00000000..49f15956 --- /dev/null +++ b/OnTopic.Tests/ViewModels/ExpansionParentTopicViewModel.cs @@ -0,0 +1,57 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: EXPANSION PARENT +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a view model whose two collections both map the same source topic to , +/// but request disjoint associations so that mapping it can exercise an association expansion pass. +/// +/// +/// +/// The relationship and the collection are populated from the same source +/// topic and mapped to the same instance, but request disjoint associations +/// ( and ). Neither association maps +/// anything on , which has no association-typed members: The disjoint requests +/// exist only so the cache sees the second encounter as missing an association and runs an expansion pass, rather than +/// returning the cached instance unchanged. What that expansion pass must not do is redo the target's non-association work. +/// +/// +/// The disjointness is all this view model contributes, and only conditionally: If one encounter finds the instance the +/// other already cached, that encounter has a missing association. Whether the encounters actually resolve that way, as an +/// ordered initial pass followed by a cache-hit expansion pass rather than two concurrent initial passes, is a property +/// of the mapping runtime, not of this view model. The tests that depend on the ordered outcome document why it holds for +/// them. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +public class ExpansionParentTopicViewModel { + + /*============================================================================================================================ + | PROPERTY: RELATED + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// A relationship collection that reaches the shared topic while requesting only . + /// + [Collection("Related", Type = CollectionType.Relationship)] + [Include(AssociationTypes.Children)] + public Collection Related { get; } = new(); + + /*============================================================================================================================ + | PROPERTY: CHILDREN + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// A children collection that reaches the shared topic while requesting only . + /// + [Include(AssociationTypes.Relationships)] + public Collection Children { get; } = new(); + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/ExpansionSharedTopicViewModel.cs b/OnTopic.Tests/ViewModels/ExpansionSharedTopicViewModel.cs new file mode 100644 index 00000000..3c3efbcb --- /dev/null +++ b/OnTopic.Tests/ViewModels/ExpansionSharedTopicViewModel.cs @@ -0,0 +1,73 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: EXPANSION SHARED +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a view model that encounters more than once during a single mapping +/// operation, so that the second encounter triggers an association expansion pass (mapAssociationsOnly), while the +/// first only includes the properties. +/// +/// +/// +/// This has no association-typed members by design. Its content is , an ungated (gate ) nested-topics collection, and , a "compatible" property (i.e., mapped +/// directly from a first-class property on ). Unlike a gated association, which the cache claims once +/// and its flag check then skips on later passes, neither of these is tied to an association, so an expansion pass would +/// redundantly remap both unless the mapper explicitly skips non-association work. records how +/// many times is assigned, so a test can confirm the compatible property is not reassigned again during +/// the expansion pass. +/// +/// +/// This is only reachable in tandem with , whose two collections perform the +/// initial and expansion passes against a single, cached instance of this view model. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +public class ExpansionSharedTopicViewModel { + + /*============================================================================================================================ + | PRIVATE VARIABLES + \---------------------------------------------------------------------------------------------------------------------------*/ + private int _keyMapCount; + + /*============================================================================================================================ + | PROPERTY: KEY + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// A compatible property, mapped one-to-one from the source . Records each assignment via . + /// + public string? Key { + get; + set { + field = value; + _keyMapCount++; + } + } + + /*============================================================================================================================ + | PROPERTY: KEY MAP COUNT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// The number of times has been assigned by the mapping service. + /// + public int KeyMapCount => _keyMapCount; + + /*============================================================================================================================ + | PROPERTY: CATEGORIES + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// An ungated nested-topics collection, mapped from the source topic's nested Categories container. + /// + public Collection Categories { get; } = new(); + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/RelationshipOnlyTopicViewModel.cs b/OnTopic.Tests/ViewModels/RelationshipOnlyTopicViewModel.cs new file mode 100644 index 00000000..6a8d8682 --- /dev/null +++ b/OnTopic.Tests/ViewModels/RelationshipOnlyTopicViewModel.cs @@ -0,0 +1,33 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: RELATIONSHIP ONLY +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a simple view model with a single, explicitly typed property (). +/// +/// +/// +/// Intended as a stand-in for cases where a very simple view model is required for test purposes, without introducing other +/// mapping scenarios that might introduce errors, even though they've not part of the test. Unlike , whose maps , is explicitly typed as a relationship, so it exercises only the +/// relationship probe in TopicMappingService.GetSourceCollectionAsync. +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +public class RelationshipOnlyTopicViewModel: KeyOnlyTopicViewModel { + + [Collection("Related", Type = CollectionType.Relationship)] + public Collection Related { get; } = new(); + +} //Class \ No newline at end of file diff --git a/OnTopic.Tests/ViewModels/SharedConcurrentTopicViewModel.cs b/OnTopic.Tests/ViewModels/SharedConcurrentTopicViewModel.cs new file mode 100644 index 00000000..a51c84ed --- /dev/null +++ b/OnTopic.Tests/ViewModels/SharedConcurrentTopicViewModel.cs @@ -0,0 +1,36 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ +using OnTopic.Mapping; + +namespace OnTopic.Tests.ViewModels; + +/*============================================================================================================================== +| VIEW MODEL: SHARED CONCURRENT TOPIC +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Provides a strongly typed data transfer object, implemented as a positional record, for testing that two concurrent +/// branches mapping the same topic to the same view model type share a single instance. +/// +/// +/// +/// The collection is mapped through a constructor parameter, so constructing this model requires the +/// source topic's payload to be loaded. When paired with a repository that suspends inside its lazy load, this lets a test +/// hold one branch mid-construction while a second branch reaches the same still-initializing cache entry, evaluating the +/// 's support of sibling concurrency. +/// +/// +/// The actual sibling references are set up in the accompanying . +/// +/// +/// This is a sample class intended for test purposes only; it is not designed for use in a production environment. +/// +/// +/// The key of the mapped topic. +/// A collection mapped from a constructor parameter, forcing the source payload to be loaded. +public record SharedConcurrentTopicViewModel( + string Key, + [Collection("Related")] Collection? Related +); \ No newline at end of file diff --git a/OnTopic/Associations/TopicRelationshipMultiMap.cs b/OnTopic/Associations/TopicRelationshipMultiMap.cs index 94021454..dfcdafa1 100644 --- a/OnTopic/Associations/TopicRelationshipMultiMap.cs +++ b/OnTopic/Associations/TopicRelationshipMultiMap.cs @@ -67,20 +67,35 @@ internal void Clear() { } /// - /// Removes all objects grouped by a specific . + /// Removes all objects grouped by a specific , as well as any entries registered under that key. /// /// - /// If there are any objects in the specified , then the will be marked as . Delegates to for each entry so the reciprocal relationship is also removed from each target's . + /// If there are any objects or entries registered under the specified , then the will be marked as . Delegates to for each resolved entry so the + /// reciprocal relationship is also removed from each target's . Clearing the entries prevents a subsequent from resolving and + /// resurrecting relationships this call just removed. /// /// The key of the relationship to be cleared. public void Clear(string relationshipKey) { + Contract.Requires(!String.IsNullOrWhiteSpace(relationshipKey), nameof(relationshipKey)); + + var hadLoadedValues = _storage.GetValues(relationshipKey).Count > 0; + var hadDeferredEntries = Deferred.Remove(relationshipKey); + foreach (var topic in _storage.GetValues(relationshipKey).ToArray()) { Remove(relationshipKey, topic); } + + // Remove() already marks the key dirty for each resident topic it removes; if only deferred entries existed, mark it here + // so the clear isn't silently lost + if (!hadLoadedValues && hadDeferredEntries) { + _dirtyKeys.MarkAs(relationshipKey, markDirty: !_parent.IsNew); + } + } /// diff --git a/OnTopic/Attributes/AttributeCollection.cs b/OnTopic/Attributes/AttributeCollection.cs index e43a30b0..c3d0a9f2 100644 --- a/OnTopic/Attributes/AttributeCollection.cs +++ b/OnTopic/Attributes/AttributeCollection.cs @@ -21,12 +21,12 @@ namespace OnTopic.Attributes; /// The class tracks these through its property, which is an instance of /// the class. /// -/// When is , iterating the collection (e.g., via foreach, -/// LINQ operators, or ) returns only the indexed attributes already present and -/// does not fetch the deferred extended attribute blob. Only a keyed lookup autoloads on a miss. Callers that require a -/// complete set of attributes must first await with . Otherwise, a decision that depends on seeing every attribute may act on a partial -/// view without any error being raised. +/// When is , iterating the collection directly (e.g., via +/// foreach or LINQ operators) returns only the indexed attributes already present and does not fetch the deferred +/// extended attribute blob; only a keyed lookup or autoloads. Callers that +/// enumerate the collection directly and require a complete set of attributes must first await with . Otherwise, a decision that +/// depends on seeing every attribute may act on a partial view without any error being raised. /// /// public class AttributeCollection : TrackedRecordCollection { @@ -142,7 +142,7 @@ public bool IsDirty(bool excludeLastModified) bool autoLoad = true ) { if (autoLoad && LoadState is LoadState.NotLoaded && !Contains(key)) { - ((ITopicLazyLoadable)AssociatedTopic).EnsureLoaded(TopicPayload.ExtendedAttributes); + ((ITopicLazyLoadable)AssociatedTopic).EnsureLoaded(TopicPayload.ExtendedAttributes).GetAwaiter().GetResult(); } return base.GetValue(key, defaultValue, inheritFromParent, maxHops, autoLoad); } @@ -217,8 +217,9 @@ public void SetValue( /// /// The method will exclude attributes which correspond to properties on /// which contain specialized getter logic, such as and . Like any enumeration over the collection, this reads only the resident attributes; see the remarks for the completeness contract on a topic. + /// "Topic.LastModified"/>. Unlike a direct enumeration of the collection, this autoloads the extended attribute blob for + /// each source (the current collection, and, if is true, each in the chain) that is , so the result is always complete. /// /// /// Determines if attributes from the should be included. Defaults to false. @@ -229,6 +230,10 @@ public AttributeDictionary AsAttributeDictionary(bool inheritFromBase = false) { var attributes = new AttributeDictionary(); var count = 0; while (sourceAttributes is not null && ++count < 5) { + if (sourceAttributes.LoadState is LoadState.NotLoaded) { + var associatedTopic = (ITopicLazyLoadable)sourceAttributes.AssociatedTopic; + associatedTopic.EnsureLoaded(TopicPayload.ExtendedAttributes).GetAwaiter().GetResult(); + } foreach (var attribute in sourceAttributes) { if (count is 1 || !attributes.ContainsKey(attribute.Key)) { attributes.TryAdd(attribute.Key, attribute.Value); diff --git a/OnTopic/Mapping/Internal/MapPath.cs b/OnTopic/Mapping/Internal/MapPath.cs new file mode 100644 index 00000000..1403fabd --- /dev/null +++ b/OnTopic/Mapping/Internal/MapPath.cs @@ -0,0 +1,71 @@ +/*============================================================================================================================== +| Author Ignia, LLC +| Client Ignia, LLC +| Project Topics Library +\=============================================================================================================================*/ + +namespace OnTopic.Mapping.Internal; + +/*============================================================================================================================== +| CLASS: MAP PATH +\-----------------------------------------------------------------------------------------------------------------------------*/ +/// +/// Represents a single frame in the depth-first path of an in-progress mapping operation, tracking the +/// and target currently being constructed, along with a reference to the frame that preceded it. +/// +/// +/// The allows the to distinguish a genuine constructor cycle, in +/// which a topic is mapped to a type that is already being constructed higher up the same call chain, from sibling +/// concurrency, in which two independent branches happen to map the same topic to the same type at the same time. The former +/// is a true circular reference and must throw; the latter is benign, and the joiner should await the in-progress result. +/// +/// The of the topic being mapped at this frame. +/// The target being constructed at this frame. +/// The preceding frame, or null if this frame is the path root. +internal sealed class MapPath(int topicId, Type type, MapPath? parent) { + + /*============================================================================================================================ + | PROPERTY: TOPIC ID + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// The of the topic being mapped at this frame. + /// + internal int TopicId { get; } = topicId; + + /*============================================================================================================================ + | PROPERTY: TYPE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// The target being constructed at this frame. + /// + internal Type Type { get; } = type; + + /*============================================================================================================================ + | PROPERTY: PARENT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// The preceding frame, or null if this frame is the root of the path. + /// + internal MapPath? Parent { get; } = parent; + + /*============================================================================================================================ + | METHOD: CONTAINS + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Determines whether the supplied and pair already appears anywhere on + /// the current path, indicating a constructor cycle. + /// + /// The to search for. + /// The target to search for. + /// Returns true if the pair is already on the path, and otherwise false. + internal bool Contains(int topicId, Type type) { + // Walk up the parent chain, comparing each frame against the requested pair + for (var frame = this; frame is not null; frame = frame.Parent) { + if (frame.TopicId == topicId && frame.Type == type) { + return true; + } + } + return false; + } + +} //Class \ No newline at end of file diff --git a/OnTopic/Mapping/Internal/MappedTopicCache.cs b/OnTopic/Mapping/Internal/MappedTopicCache.cs index 11be1494..4481f4d0 100644 --- a/OnTopic/Mapping/Internal/MappedTopicCache.cs +++ b/OnTopic/Mapping/Internal/MappedTopicCache.cs @@ -32,9 +32,23 @@ internal sealed class MappedTopicCache { /// The associated with the cache entry. /// The that the has been mapped to. /// The containing the cached instance and metadata. + /// + /// Determines whether an entry that is still should be returned. Left + /// false by default, so callers continue to see only fully constructed entries; set true only by callers who + /// are prepared to distinguish a constructor cycle from sibling concurrency and to await an in-progress entry's completion. + /// /// Returns true if a cached entry could be found, and otherwise false. - internal bool TryGetValue(int topicId, Type type, [NotNullWhen(true)] out MappedTopicCacheEntry? cacheEntry) { - if (_cache.TryGetValue(GetCacheKey(topicId, type), out var existingCacheEntry) && !existingCacheEntry.IsInitializing) { + internal bool TryGetValue( + int topicId, + Type type, + [NotNullWhen(true)] + out MappedTopicCacheEntry? cacheEntry, + bool includeInitializing = false + ) { + if ( + _cache.TryGetValue(GetCacheKey(topicId, type), out var existingCacheEntry) && + (includeInitializing || !existingCacheEntry.IsInitializing) + ) { cacheEntry = existingCacheEntry; return true; }; @@ -59,21 +73,14 @@ internal void Register(int topicId, AssociationTypes associations, object viewMo \-------------------------------------------------------------------------------------------------------------------------*/ var type = viewModel.GetType(); var cacheKey = GetCacheKey(topicId, type); - var cacheEntry = new MappedTopicCacheEntry() { - MappedTopic = viewModel, - Associations = associations - }; + var cacheEntry = new MappedTopicCacheEntry(); /*-------------------------------------------------------------------------------------------------------------------------- | Get or add entry \-------------------------------------------------------------------------------------------------------------------------*/ - if (topicId > 0 && !type.Equals(typeof(object))) { + if (topicId > 0 && type != typeof(object)) { cacheEntry = _cache.GetOrAdd(cacheKey, cacheEntry); - if (cacheEntry.IsInitializing) { - cacheEntry.IsInitializing = false; - cacheEntry.MappedTopic = viewModel; - cacheEntry.Associations = associations; - } + cacheEntry.Complete(viewModel, associations); } } @@ -83,34 +90,31 @@ internal void Register(int topicId, AssociationTypes associations, object viewMo \---------------------------------------------------------------------------------------------------------------------------*/ /// /// Attempts to preregister a for a that is in the process of - /// being mapped to . + /// being mapped to , returning the entry along with whether this call created it. /// + /// + /// The returned IsNew flag is true when this call established the entry, and therefore owns its construction; + /// it is false when a concurrent pass had already preregistered the same and , in which case the returned entry is that concurrent pass's entry. + /// /// The associated with the cache entry. /// The that the is being mapped to. - internal MappedTopicCacheEntry Preregister(int topicId, Type type) { + internal (MappedTopicCacheEntry Entry, bool IsNew) Preregister(int topicId, Type type) { /*-------------------------------------------------------------------------------------------------------------------------- | Construct cache entry \-------------------------------------------------------------------------------------------------------------------------*/ var cacheKey = GetCacheKey(topicId, type); - var cacheEntry = new MappedTopicCacheEntry() { - IsInitializing = true - }; + var cacheEntry = new MappedTopicCacheEntry(); /*-------------------------------------------------------------------------------------------------------------------------- | Get or add entry \-------------------------------------------------------------------------------------------------------------------------*/ if (topicId > 0 && !type.Equals(typeof(object))) { var existingCacheEntry = _cache.GetOrAdd(cacheKey, cacheEntry); - if (existingCacheEntry != cacheEntry) { - throw new TopicMappingException( - $"An attempt has been made to map '{topicId}' to a {type.Name} has resulted in a circular reference during the " + - $"construction of the {type.Name} instance. This is not allowed. Circular must be be mapped as properties, not " + - $"as constructor parameters, so that cached entries can be returned." - ); - } + return (existingCacheEntry, existingCacheEntry == cacheEntry); } - return cacheEntry; + return (cacheEntry, true); } diff --git a/OnTopic/Mapping/Internal/MappedTopicCacheEntry.cs b/OnTopic/Mapping/Internal/MappedTopicCacheEntry.cs index e37fb3b6..b9745a4c 100644 --- a/OnTopic/Mapping/Internal/MappedTopicCacheEntry.cs +++ b/OnTopic/Mapping/Internal/MappedTopicCacheEntry.cs @@ -15,21 +15,34 @@ namespace OnTopic.Mapping.Internal; /// /// /// In addition to the actual , this also includes a property for -/// tracking what associations were mapped to the . This allows the to be update the cached object with any missing associations, which can be identified using the method. In turn, the cache can then be updated to reflect those new -/// associations by using . This ensures that even if a topic has -/// already been mapped, its scope can be expanded without duplicating effort. +/// tracking what associations were mapped to the . This allows the +/// to expand the cached object with any missing associations. A caller may peek at the missing associations using the +/// method, or record them and receive the newly added subset in a +/// single atomic operation using , so that concurrent passes don't both +/// end up mapping the same associations. This ensures that even if a topic has already been mapped, its scope can be expanded +/// without duplicating effort. /// internal sealed class MappedTopicCacheEntry { + /*============================================================================================================================ + | PRIVATE VARIABLES + \---------------------------------------------------------------------------------------------------------------------------*/ + private readonly object _lock = new(); + private readonly TaskCompletionSource _completionSource = new(TaskCreationOptions.RunContinuationsAsynchronously); + /*============================================================================================================================ | PROPERTY: MAPPED TOPIC \---------------------------------------------------------------------------------------------------------------------------*/ /// /// Provides a reference to the mapped object. /// - internal object MappedTopic { get; set; } = null!; + /// + /// Assigned only by , which also settles the task, + /// so the mapped instance is never published outside the completion cycle. This topic is fully constructed, but may not yet + /// have all of its properties mapped; the only prevents two instances of + /// the same view model from being constructed, but doesn't guarantee that mapping is finished. + /// + internal object MappedTopic { get; private set; } = null!; /*============================================================================================================================ | PROPERTY: IS INITIALIZING @@ -40,11 +53,12 @@ internal sealed class MappedTopicCacheEntry { /// /// The property allows an entry to be pre-cached prior to the object being completed. This /// allows the to detect circular references within the object initialization sequence. - /// This is important because, unlikely property mapping where a cached reference can be returned, a circular reference - /// in constructor mapping is expected to throw an exception. By registering that an object is being initialized, the - /// is able to detect circuluar references during constructor mapping. + /// This is important because, unlike property mapping where a cached reference can be returned, a circular reference in + /// constructor mapping is expected to throw an exception. It is derived from the task rather than + /// stored, so a faulted entry remains initializing, ensuring an awaiting pass observes the fault instead of a null . /// - internal bool IsInitializing { get; set; } + internal bool IsInitializing => !_completionSource.Task.IsCompletedSuccessfully; /*============================================================================================================================ | PROPERTY: ASSOCIATIONS @@ -54,6 +68,24 @@ internal sealed class MappedTopicCacheEntry { /// internal AssociationTypes Associations { get; set; } = AssociationTypes.None; + /*============================================================================================================================ + | PROPERTY: COMPLETION + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Returns a that completes once the has been constructed and registered, even + /// if not all properties have yet been mapped, thus allowing a second pass to await a first pass that is still initializing + /// the same entry, instead of duplicating its work or, worse yet, failing. + /// + /// + /// The task is settled by once the target has been constructed, or by + /// if construction throws, so a second pass awaiting it never hangs. It is settled during + /// registration, after the constructor runs but before the first pass maps the target's properties, so a second pass may + /// observe an instance whose constructor parameters are set but whose properties are not yet mapped. This early publication + /// is what allows a property-level circular reference to resolve to the cached (if partially populated) instance instead of + /// recursing indefinitely, while still catching constructor-level circular references. + /// + internal Task Completion => _completionSource.Task; + /*============================================================================================================================ | METHOD: GET MISSING ASSOCIATIONS \---------------------------------------------------------------------------------------------------------------------------*/ @@ -61,15 +93,69 @@ internal sealed class MappedTopicCacheEntry { /// Given a target , identifies any associations not covered by /// and returns them as a new instance. /// + /// + /// This is intended as a quick hint to decide e.g., whether an expansion is needed at all, without any side effects. It + /// does not record the result; a caller that intends to map the missing associations should instead use , so that concurrent passes cannot both map the same associations. + /// internal AssociationTypes GetMissingAssociations(AssociationTypes associations) => Associations ^ (associations | Associations); /*============================================================================================================================ | METHOD: ADD MISSING ASSOCIATIONS \---------------------------------------------------------------------------------------------------------------------------*/ /// - /// Given a target , adds any missing to the property. + /// Given a target , adds any not already covered by and returns + /// the subset that this call just added. + /// + /// + /// This is the mutating counterpart to : It adds any associations + /// that aren't already covered, and reports which ones were added back to the caller so the caller knows which associations + /// to process. Because the delta is calculated and saved under a single lock, two concurrent passes over the same cached + /// instance receive disjoint results, ensuring each association is mapped by exactly one caller. A caller that receives + /// has nothing left to map and should return the cached instance. + /// + internal AssociationTypes AddMissingAssociations(AssociationTypes associations) { + lock (_lock) { + var missing = GetMissingAssociations(associations); + Associations = associations | Associations; + return missing; + } + } + + /*============================================================================================================================ + | METHOD: COMPLETE + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Publishes the constructed and its as the entry's and , and settles the task, releasing any second pass + /// awaiting this entry. + /// + /// + /// This is the sole writer of and the initial writer of , so the + /// instance, its associations, and the completion signal are always published together under a single lock. Only the first + /// completion takes effect; a later duplicate registration is ignored, keeping the cached instance stable, while remaining + /// unobservable before the entry is completed. + /// + /// The constructed view model associated with the entry. + /// The associations that the view model was mapped with. + internal void Complete(object viewModel, AssociationTypes associations) { + lock (_lock) { + if (!_completionSource.Task.IsCompleted) { + MappedTopic = viewModel; + Associations = associations; + _completionSource.TrySetResult(); + } + } + } + + /*============================================================================================================================ + | METHOD: FAULT + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Faults the task with the supplied so that any pass awaiting the + /// entry observes the failure instead of hanging when construction of the entry throws. /// - internal void AddMissingAssociations(AssociationTypes associations) => Associations = associations | Associations; + /// The exception that occurred while constructing the entry. + internal void Fault(Exception exception) => _completionSource.TrySetException(exception); } //Class \ No newline at end of file diff --git a/OnTopic/Mapping/Reverse/ReverseTopicMappingService.cs b/OnTopic/Mapping/Reverse/ReverseTopicMappingService.cs index 57d4acc1..ca892ce5 100644 --- a/OnTopic/Mapping/Reverse/ReverseTopicMappingService.cs +++ b/OnTopic/Mapping/Reverse/ReverseTopicMappingService.cs @@ -25,7 +25,6 @@ public class ReverseTopicMappingService : IReverseTopicMappingService { | PRIVATE VARIABLES \---------------------------------------------------------------------------------------------------------------------------*/ readonly ITopicRepository _topicRepository; - readonly ContentTypeDescriptorCollection _contentTypeDescriptors; /*============================================================================================================================ | CONSTRUCTOR @@ -44,16 +43,6 @@ public ReverseTopicMappingService(ITopicRepository topicRepository) { | Set dependencies \-------------------------------------------------------------------------------------------------------------------------*/ _topicRepository = topicRepository; - _contentTypeDescriptors = topicRepository.GetContentTypeDescriptors(); - - /*-------------------------------------------------------------------------------------------------------------------------- - | Validate dependencies - \-------------------------------------------------------------------------------------------------------------------------*/ - Contract.Assume( - _contentTypeDescriptors, - $"The {nameof(ITopicRepository.GetContentTypeDescriptors)}() method returned null. This could indicate a corrupt " + - $"or data source." - ); } @@ -133,7 +122,7 @@ public ReverseTopicMappingService(ITopicRepository topicRepository) { Contract.Assume(source.ContentType, nameof(source.ContentType)); //Ensure the content type is valid - if (!_contentTypeDescriptors.Contains(source.ContentType)) { + if (!GetContentTypeDescriptors().Contains(source.ContentType)) { throw new MappingModelValidationException( $"The {nameof(source)} object (with the key '{source.Key}') has a content type of '{source.ContentType}'. There " + $"are no matching content types in the ITopicRepository provided. This suggests that the binding model is invalid. " + @@ -168,6 +157,30 @@ public ReverseTopicMappingService(ITopicRepository topicRepository) { } + /*============================================================================================================================ + | PRIVATE: GET CONTENT TYPE DESCRIPTORS + \---------------------------------------------------------------------------------------------------------------------------*/ + /// + /// Retrieves the from the . + /// + /// + /// Called per-use rather than cached into a field, since content types can be added after this service is constructed, and + /// a local cache would silently exclude any updates for the life of the service. Further, , + /// the base class for every production in this library, already caches the result after the + /// first call and maintains the live collection in place (e.g. Delete refreshes it), so the per-call access here is + /// expected to be cheap, acknowledging that's a property of that base class, not a guarantee of the interface itself. + /// + private ContentTypeDescriptorCollection GetContentTypeDescriptors() { + var contentTypeDescriptors = _topicRepository.GetContentTypeDescriptors(); + Contract.Assume( + contentTypeDescriptors, + $"The {nameof(ITopicRepository.GetContentTypeDescriptors)}() method returned null. This could indicate a corrupt " + + $"data source." + ); + return contentTypeDescriptors; + } + /*============================================================================================================================ | PRIVATE: MAP (TOPIC) \---------------------------------------------------------------------------------------------------------------------------*/ @@ -183,6 +196,13 @@ public ReverseTopicMappingService(ITopicRepository topicRepository) { /// /// An instance of provided with attributes appropriately mapped. /// + /// + /// Properties are mapped sequentially, in source order, rather than concurrently; this avoids concurrent mutation of the + /// association collections (, , etc.), which aren't thread + /// safe, and which individual property mappers write to on the shared . As a result, an exception + /// thrown while mapping one property surfaces immediately, without waiting for or aggregating exceptions from subsequent + /// properties, and any properties mapped before the failure remain applied to . + /// private async Task MapAsync(object? source, Topic target, string? attributePrefix) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -190,22 +210,28 @@ public ReverseTopicMappingService(ITopicRepository topicRepository) { \-------------------------------------------------------------------------------------------------------------------------*/ if (source is null) return target; + /*-------------------------------------------------------------------------------------------------------------------------- + | Warm extended attributes + >--------------------------------------------------------------------------------------------------------------------------- + | Without this, TrackedRecordCollection.SetValue() potentially runs against an unloaded extended attributes, and thus marks + | attributes as dirty even if they're unchanged, causing needless version rows on save. + \-------------------------------------------------------------------------------------------------------------------------*/ + await ((ITopicLazyLoadable)target).EnsureLoaded(TopicPayload.ExtendedAttributes).ConfigureAwait(false); + /*-------------------------------------------------------------------------------------------------------------------------- | Validate model \-------------------------------------------------------------------------------------------------------------------------*/ var typeAccessor = TypeAccessorCache.GetTypeAccessor(source.GetType()); - var contentTypeDescriptor = _contentTypeDescriptors.GetValue(target.ContentType); + var contentTypeDescriptor = GetContentTypeDescriptors().GetValue(target.ContentType); BindingModelValidator.ValidateModel(typeAccessor, contentTypeDescriptor, attributePrefix); /*-------------------------------------------------------------------------------------------------------------------------- | Loop through properties, mapping each one \-------------------------------------------------------------------------------------------------------------------------*/ - List taskQueue = []; foreach (var property in typeAccessor.GetMembers(MemberTypes.Property)) { - taskQueue.Add(SetPropertyAsync(source, target, property, attributePrefix)); + await SetPropertyAsync(source, target, property, attributePrefix).ConfigureAwait(false); } - await Task.WhenAll([.. taskQueue]).ConfigureAwait(false); /*-------------------------------------------------------------------------------------------------------------------------- | Return result @@ -239,7 +265,7 @@ private async Task SetPropertyAsync( | Establish per-property variables \-------------------------------------------------------------------------------------------------------------------------*/ var configuration = memberAccessor.Configuration; - var contentTypeDescriptor = _contentTypeDescriptors.GetValue(target.ContentType); + var contentTypeDescriptor = GetContentTypeDescriptors().GetValue(target.ContentType); var compositeAttributeKey = configuration.GetCompositeAttributeKey(attributePrefix); Contract.Assume(contentTypeDescriptor, nameof(contentTypeDescriptor)); @@ -444,6 +470,13 @@ private async Task SetNestedTopicsAsync( \-------------------------------------------------------------------------------------------------------------------------*/ var sourceList = (IList?)memberAccessor.GetValue(source) ?? new List(); + /*-------------------------------------------------------------------------------------------------------------------------- + | Warm target's children + >--------------------------------------------------------------------------------------------------------------------------- + | Replaces the Children getter's synchronous autoload with an explicit, asynchronous warm-up prior to the below probe + \-------------------------------------------------------------------------------------------------------------------------*/ + await ((ITopicLazyLoadable)target).EnsureLoaded(TopicPayload.Children).ConfigureAwait(false); + /*-------------------------------------------------------------------------------------------------------------------------- | Establish target collection to store mapped topics \-------------------------------------------------------------------------------------------------------------------------*/ @@ -453,6 +486,14 @@ private async Task SetNestedTopicsAsync( container.IsHidden = true; } + /*-------------------------------------------------------------------------------------------------------------------------- + | Warm container's children + >--------------------------------------------------------------------------------------------------------------------------- + | The container can be NotLoaded even when target is loaded; PopulateTargetCollectionAsync()'s Contains() check for existing + | children as well as it's check for orphans require the complete set + \-------------------------------------------------------------------------------------------------------------------------*/ + await ((ITopicLazyLoadable)container).EnsureLoaded(TopicPayload.Children).ConfigureAwait(false); + /*-------------------------------------------------------------------------------------------------------------------------- | Map the topics from the source collection, and add them to the target collection \-------------------------------------------------------------------------------------------------------------------------*/ @@ -535,27 +576,16 @@ private async Task SetReference( /// /// The to pull the binding models from. /// The target to add the mapped objects to. + /// + /// Children are mapped and added sequentially, in order, rather than concurrently; this + /// avoids concurrent mutation on the shared target and guarantees 's + /// resulting order matches the binding model, instead of varying with completion order. + /// private async Task PopulateTargetCollectionAsync( IList sourceList, KeyedTopicCollection targetList ) { - /*-------------------------------------------------------------------------------------------------------------------------- - | Queue up mapping tasks - \-------------------------------------------------------------------------------------------------------------------------*/ - List> taskQueue = []; - - //Map child binding model to target collection on the target - foreach (ITopicBindingModel childBindingModel in sourceList) { - Contract.Assume(childBindingModel.Key); - if (targetList.Contains(childBindingModel.Key)) { - taskQueue.Add(MapAsync(childBindingModel, targetList.GetValue(childBindingModel.Key)!)); - } - else { - taskQueue.Add(MapAsync(childBindingModel)); - } - } - /*-------------------------------------------------------------------------------------------------------------------------- | Remove orphaned topics \-------------------------------------------------------------------------------------------------------------------------*/ @@ -567,15 +597,23 @@ KeyedTopicCollection targetList } /*-------------------------------------------------------------------------------------------------------------------------- - | Process mapping tasks + | Map and add children in source order + >--------------------------------------------------------------------------------------------------------------------------- + | Sequential by design: concurrent MapAsync() calls would mutate non-thread-safe collections on the shared target Topic in + | parallel, and completion-order nondeterminism would make targetList's resulting order unpredictable. \-------------------------------------------------------------------------------------------------------------------------*/ - while (taskQueue.Count > 0) { - var topicTask = await Task.WhenAny(taskQueue).ConfigureAwait(false); - taskQueue.Remove(topicTask); - var topic = await topicTask.ConfigureAwait(false); - if (topic is not null && !targetList.Contains(topic.Key)) { + foreach (ITopicBindingModel childBindingModel in sourceList) { + + Contract.Assume(childBindingModel.Key); + + var topic = targetList.Contains(childBindingModel.Key) + ? await MapAsync(childBindingModel, targetList.GetValue(childBindingModel.Key)!).ConfigureAwait(false) + : await MapAsync(childBindingModel).ConfigureAwait(false); + + if (topic is not null && !targetList.Contains(topic.Key)) { targetList.Add(topic); } + } } diff --git a/OnTopic/Mapping/TopicMappingService.cs b/OnTopic/Mapping/TopicMappingService.cs index a9cab4ce..4ae26e71 100644 --- a/OnTopic/Mapping/TopicMappingService.cs +++ b/OnTopic/Mapping/TopicMappingService.cs @@ -69,12 +69,14 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe /// Determines what associations the mapping should include, if any. /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. + /// The current mapping request's path, used to detect circular references during construction. /// An instance of the dynamically determined View Model with properties appropriately mapped. private async Task MapAsync( Topic? topic, AssociationTypes associations, MappedTopicCache cache, - string? attributePrefix = null + string? attributePrefix = null, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -99,7 +101,7 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe /*-------------------------------------------------------------------------------------------------------------------------- | Perform mapping \-------------------------------------------------------------------------------------------------------------------------*/ - return await MapAsync(topic, viewModelType, associations, cache, attributePrefix).ConfigureAwait(false); + return await MapAsync(topic, viewModelType, associations, cache, attributePrefix, mapPath).ConfigureAwait(false); } @@ -128,13 +130,15 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe /// Determines what associations the mapping should include, if any. /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. + /// The current mapping request's path, used to detect circular references during construction. /// An instance of the dynamically determined View Model with properties appropriately mapped. private async Task MapAsync( Topic? topic, Type type, AssociationTypes associations, MappedTopicCache cache, - string? attributePrefix = null + string? attributePrefix = null, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -146,16 +150,14 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe /*-------------------------------------------------------------------------------------------------------------------------- | Handle cached objects - \-------------------------------------------------------------------------------------------------------------------------*/ - var target = (object?)null; - - if (cache.TryGetValue(topic.Id, type, out var cacheEntry)) { - target = cacheEntry.MappedTopic; - if (cacheEntry.GetMissingAssociations(associations) == AssociationTypes.None) { - return target; - } - //Call MapAsync() with target object to map missing attributes - return await MapAsync(topic, target, associations, cache, attributePrefix).ConfigureAwait(false); + >--------------------------------------------------------------------------------------------------------------------------- + | Included entries that are still initializing, so a circular constructor reference (i.e., the same topic and type are + | already under construction higher up the current chain) can be distinguished from concurrent siblings (i.e., two + | independent branches mapping the same topic and type at once). The former throws; the latter awaits for the first one to + | finish construction and then uses the same cached view model. + \-------------------------------------------------------------------------------------------------------------------------*/ + if (cache.TryGetValue(topic.Id, type, out var cacheEntry, includeInitializing: true)) { + return await resolveCachedEntry(cacheEntry, mapPath).ConfigureAwait(false); } /*-------------------------------------------------------------------------------------------------------------------------- @@ -171,77 +173,112 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe /*-------------------------------------------------------------------------------------------------------------------------- | Pre-cache entry >------------------------------------------------------------------------------------------------------------------------- - | In property mapping, we deal with circular references by returning a cached reference. That isn't practical with - | circular references in constructor mapping. To help avoid these, we register a pre-cache entry as IsInitializing, but - | without a mapped object; the TopicMappingCache is expected to throw an exception if an attempt to map that topic to that - | type occurs again prior to the constructor mapping being completed. + | In property mapping, we deal with circular references by returning a cached reference. That isn't practical with circular + | references in constructor mapping. To help avoid these, we register a pre-cache entry as IsInitializing, but without a + | mapped object. If a concurrent sibling wins the race to preregister the same topic and view model type, we defer to its + | result rather than constructing a duplicate. If we're constructing the same topic and view model type as we're already in + | the middle of constructing further up the MapPath chain, however, that's a true circular construction reference, handled + | via the local resolveCachedEntry() function. \-------------------------------------------------------------------------------------------------------------------------*/ - cache.Preregister(topic.Id, type); - - /*-------------------------------------------------------------------------------------------------------------------------- - | Handle AttributeDictionary constructor - >------------------------------------------------------------------------------------------------------------------------- - | A model may optionally expose a constructor with a single parameter accepting an AttributeDictionary. In this scenario, - | the TopicMappingService may optionally pass a lightweight AttributeDictionary, allowing the model's constructor to - | populate scalar values, instead of relying on reflection. - \-------------------------------------------------------------------------------------------------------------------------*/ - if (parameters.Count is 1 && parameters[0].Type == typeof(AttributeDictionary)) { - - // This strategy is only performant if there are quite a several scalar properties and they are well-covered by the - // attributes. As a fast heuristic to evaluate this, we expect five or more attributes and three or more compatible - // properties. In practice, this should be benefitial with any more than mapped attributes, but we also expect that most - // topics will have 2-3 excluded or unmapped attributes (e.g., Title, LastModified). With models, we can be a bit more - // intelligent, by excluding any members that are likely compatible with Topic properties, thus exluding e.g., Id, Key, - // WebPath, etc. This doesn't guarantee that the attributes map to the properties, but a more accurate evaluation would - // undermine the performance benefits of this optimization. - if (topic.Attributes.Count >= 5 && properties.Count(p => !p.MaybeCompatible) >= 3) { - var attributes = topic.Attributes.AsAttributeDictionary(true); - arguments[0] = attributes; - attributeArguments = attributes; - } - else { - parameters = new(); - arguments = []; - } + var (entry, isNew) = cache.Preregister(topic.Id, type); + if (!isNew) { + return await resolveCachedEntry(entry, mapPath).ConfigureAwait(false); } /*-------------------------------------------------------------------------------------------------------------------------- - | Handle other constructors - >------------------------------------------------------------------------------------------------------------------------- - | A model may optionally expose a constructor with multiple parameters, which can be defined via reflection in the same - | way as properties would be. This is especially useful for records using the positional syntax (i.e., where properties - | are defined using the constructor). This also, optionally, provides the model with more control, where needed, over how - | it's constructed. - \-------------------------------------------------------------------------------------------------------------------------*/ - else { + | Establish mapping path chain + >--------------------------------------------------------------------------------------------------------------------------- + | Now that this pass owns constructing a new view model, establish the initial topic and view model type in the MapPath, so + | any nested mapping that arrives back to the same pair while it is still initializing is recognized as a true circular + | constructor reference rather than harmless concurrency among independent siblings. + \-------------------------------------------------------------------------------------------------------------------------*/ + mapPath = new(topic.Id, type, mapPath); + + /*-------------------------------------------------------------------------------------------------------------------------- + | Construct and register the target + >--------------------------------------------------------------------------------------------------------------------------- + | Only the construction span is guarded, so the entry's completion is always settled: cache.Register() settles it on + | success and the catch faults it on failure, so a pass awaiting this entry observes the failure instead of hanging. + | Property mapping runs afterward, outside the guard, since the entry is already settled by then. + \-------------------------------------------------------------------------------------------------------------------------*/ + object? target; + + try { + + /*------------------------------------------------------------------------------------------------------------------------ + | Handle AttributeDictionary constructor + >------------------------------------------------------------------------------------------------------------------------- + | A model may optionally expose a constructor with a single parameter accepting an AttributeDictionary. In this scenario, + | the TopicMappingService may optionally pass a lightweight AttributeDictionary, allowing the model's constructor to + | populate scalar values, instead of relying on reflection. + \-----------------------------------------------------------------------------------------------------------------------*/ + if (parameters.Count is 1 && parameters[0].Type == typeof(AttributeDictionary)) { + + // This strategy is only performant if there are quite a several scalar properties and they are well-covered by the + // attributes. As a fast heuristic to evaluate this, we expect five or more attributes and three or more compatible + // properties. In practice, this should be benefitial with any more than mapped attributes, but we also expect that most + // topics will have 2-3 excluded or unmapped attributes (e.g., Title, LastModified). With models, we can be a bit more + // intelligent, by excluding any members that are likely compatible with Topic properties, thus exluding e.g., Id, Key, + // WebPath, etc. This doesn't guarantee that the attributes map to the properties, but a more accurate evaluation would + // undermine the performance benefits of this optimization. + if (topic.Attributes.Count >= 5 && properties.Count(p => !p.MaybeCompatible) >= 3) { + var attributes = topic.Attributes.AsAttributeDictionary(true); + arguments[0] = attributes; + attributeArguments = attributes; + } + else { + parameters = new(); + arguments = []; + } - foreach (var parameter in parameters) { - parameterQueue.Add(parameter.ParameterInfo.Position, GetParameterAsync(topic, associations, parameter, cache, attributePrefix)); } - await Task.WhenAll(parameterQueue.Values).ConfigureAwait(false); + /*------------------------------------------------------------------------------------------------------------------------ + | Handle other constructors + >------------------------------------------------------------------------------------------------------------------------- + | A model may optionally expose a constructor with multiple parameters, which can be defined via reflection in the same + | way as properties would be. This is especially useful for records using the positional syntax (i.e., where properties + | are defined using the constructor). This also, optionally, provides the model with more control, where needed, over how + | it's constructed. + \-----------------------------------------------------------------------------------------------------------------------*/ + else { + + foreach (var parameter in parameters) { + parameterQueue.Add(parameter.ParameterInfo.Position, GetParameterAsync(topic, associations, parameter, cache, attributePrefix, mapPath)); + } + + await Task.WhenAll(parameterQueue.Values).ConfigureAwait(false); + + foreach (var parameter in parameterQueue) { + arguments[parameter.Key] = await parameter.Value.ConfigureAwait(false); + } - foreach (var parameter in parameterQueue) { - arguments[parameter.Key] = await parameter.Value.ConfigureAwait(false); } - } + /*------------------------------------------------------------------------------------------------------------------------ + | Initialize object + \-----------------------------------------------------------------------------------------------------------------------*/ + target = Activator.CreateInstance(type, arguments); - /*-------------------------------------------------------------------------------------------------------------------------- - | Initialize object - \-------------------------------------------------------------------------------------------------------------------------*/ - target = Activator.CreateInstance(type, arguments); + Contract.Assume( + target, + $"The target type '{type}' could not be properly constructed, as required to map the topic '{topic.GetUniqueKey()}'." + ); - Contract.Assume( - target, - $"The target type '{type}' could not be properly constructed, as required to map the topic '{topic.GetUniqueKey()}'." - ); + /*------------------------------------------------------------------------------------------------------------------------ + | Cache object + \-----------------------------------------------------------------------------------------------------------------------*/ + cache.Register(topic.Id, associations, target); - /*-------------------------------------------------------------------------------------------------------------------------- - | Cache object - \-------------------------------------------------------------------------------------------------------------------------*/ - cache.Register(topic.Id, associations, target); + } + + // Construction failed before the entry was registered, so fault it; a concurrent pass awaiting this entry's completion then + // observes the failure instead of hanging on a map that will never complete + catch (Exception exception) { + entry.Fault(exception); + throw; + } /*-------------------------------------------------------------------------------------------------------------------------- | Loop through properties, mapping each one @@ -251,17 +288,52 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe foreach (var property in typeAccessor.GetMembers(MemberTypes.Property)) { if (!mappedParameters.Contains(property.Name, StringComparer.OrdinalIgnoreCase)) { - propertyQueue.Add(SetPropertyAsync(topic, target, associations, property, cache, attributePrefix, false)); + propertyQueue.Add(SetPropertyAsync(topic, target, associations, property, cache, attributePrefix, false, mapPath)); } } - await Task.WhenAll([.. propertyQueue]).ConfigureAwait(false); + await Task.WhenAll(propertyQueue).ConfigureAwait(false); /*-------------------------------------------------------------------------------------------------------------------------- | Return target \-------------------------------------------------------------------------------------------------------------------------*/ return target; + /*-------------------------------------------------------------------------------------------------------------------------- + | Resolve cached entry + >--------------------------------------------------------------------------------------------------------------------------- + | Returns a cached instance, expanding it with any missing associations when needed. If the entry is still initializing, it + | is either a true circular constructor reference on the current path (in which we throw an exception) or it's concurrent + | mapping of two independent siblings of the same topic and view model (in which we simply await its completion, then + | return the shared instance as a normal cache hit). + \-------------------------------------------------------------------------------------------------------------------------*/ + async Task resolveCachedEntry(MappedTopicCacheEntry pendingEntry, MapPath? path) { + + // Distinguish a constructor cycle from sibling concurrency for an entry that is still being constructed + if (pendingEntry.IsInitializing) { + if (path?.Contains(topic.Id, type) == true) { + throw new TopicMappingException( + $"A circular reference was detected while constructing the '{type.Name}' instance for topic '{topic.Id}'. Circular " + + $"references must be mapped as properties, not as constructor parameters, so that a cached instance can be returned." + ); + } + // Not on the current path: a concurrent sibling owns construction, so await its completion before resolving. A mutual + // constructor cycle split across two concurrently mapped branches is the one case this cannot distinguish and would + // deadlock; such cycles are unsupported, and still throw when reached from a single branch via the path check above. + await pendingEntry.Completion.ConfigureAwait(false); + } + + // Return the cached instance as-is when it already covers the requested associations + var cachedTarget = pendingEntry.MappedTopic; + if (pendingEntry.GetMissingAssociations(associations) is AssociationTypes.None) { + return cachedTarget; + } + + // Otherwise expand the cached instance, mapping only the missing associations + return await MapAsync(topic, cachedTarget, associations, cache, attributePrefix, path).ConfigureAwait(false); + + } + } /*============================================================================================================================ @@ -292,6 +364,7 @@ public class TopicMappingService(ITopicRepository topicRepository, ITypeLookupSe /// Determines what associations the mapping should include, if any. /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. + /// The current mapping request's path, used to detect circular references during construction. /// /// This internal version passes a private cache of mapped objects from this run. This helps prevent problems with /// recursion in case is referred to multiple times (e.g., a Children collection with MapAsync( object target, AssociationTypes associations, MappedTopicCache cache, - string? attributePrefix = null + string? attributePrefix = null, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -324,17 +398,17 @@ private async Task MapAsync( /*-------------------------------------------------------------------------------------------------------------------------- | Handle cached objects - >------------------------------------------------------------------------------------------------------------------------- + >--------------------------------------------------------------------------------------------------------------------------- | If the cache contains an entry, check to make sure it includes all of the requested associations. If it does, return it. - | If it doesn't, determine the missing associations and request to have those mapped. + | Otherwise, add the missing associations to the cache entry and map only the subset this pass added, so that a concurrent + | pass doesn't remap the same associations. \-------------------------------------------------------------------------------------------------------------------------*/ if (cache.TryGetValue(topic.Id, target.GetType(), out var cacheEntry)) { - associations = cacheEntry.GetMissingAssociations(associations); + associations = cacheEntry.AddMissingAssociations(associations); target = cacheEntry.MappedTopic; if (associations is AssociationTypes.None) { return cacheEntry.MappedTopic; } - cacheEntry.AddMissingAssociations(associations); } else if (!topic.IsNew) { cache.Register( @@ -351,9 +425,9 @@ private async Task MapAsync( var typeAccessor = TypeAccessorCache.GetTypeAccessor(target.GetType()); foreach (var property in typeAccessor.GetMembers(MemberTypes.Property)) { - taskQueue.Add(SetPropertyAsync(topic, target, associations, property, cache, attributePrefix, cacheEntry is not null)); + taskQueue.Add(SetPropertyAsync(topic, target, associations, property, cache, attributePrefix, cacheEntry is not null, mapPath)); } - await Task.WhenAll([.. taskQueue]).ConfigureAwait(false); + await Task.WhenAll(taskQueue).ConfigureAwait(false); /*-------------------------------------------------------------------------------------------------------------------------- | Return result @@ -374,12 +448,14 @@ private async Task MapAsync( /// Information related to the current parameter. /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. + /// The current mapping request's path, used to detect circular references during construction. private async Task GetParameterAsync( Topic source, AssociationTypes associations, ParameterMetadata parameter, MappedTopicCache cache, - string? attributePrefix = null + string? attributePrefix = null, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -403,14 +479,15 @@ private async Task MapAsync( parameter.Type, associations, cache, - configuration.AttributePrefix + attributePrefix + configuration.AttributePrefix + attributePrefix, + mapPath ).ConfigureAwait(false); } /*-------------------------------------------------------------------------------------------------------------------------- | Determine value \-------------------------------------------------------------------------------------------------------------------------*/ - var value = await GetValue(source, parameter.Type, associations, parameter, cache, attributePrefix, false).ConfigureAwait(false); + var value = await GetValue(source, parameter.Type, associations, parameter, cache, attributePrefix, false, mapPath).ConfigureAwait(false); if (value is null && parameter.IsList) { return await getList(parameter.Type).ConfigureAwait(false); @@ -423,14 +500,14 @@ private async Task MapAsync( \-------------------------------------------------------------------------------------------------------------------------*/ async Task getList(Type targetType) { - var sourceList = await GetSourceCollectionAsync(source, associations, parameter, attributePrefix).ConfigureAwait(false); + var sourceList = await GetSourceCollectionAsync(source, associations, parameter, attributePrefix, false).ConfigureAwait(false); var targetList = InitializeCollection(targetType); if (targetList is null) { return null; } - await PopulateTargetCollectionAsync(sourceList, targetList, parameter, cache).ConfigureAwait(false); + await PopulateTargetCollectionAsync(sourceList, targetList, parameter, cache, targetList, mapPath).ConfigureAwait(false); return targetList; @@ -452,6 +529,7 @@ private async Task MapAsync( /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. /// Determines if properties not associated with associations should be mapped. + /// The current mapping request's path, used to detect circular references during construction. private async Task SetPropertyAsync( Topic source, object target, @@ -459,7 +537,8 @@ private async Task SetPropertyAsync( MemberAccessor propertyAccessor, MappedTopicCache cache, string? attributePrefix = null, - bool mapAssociationsOnly = false + bool mapAssociationsOnly = false, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -485,7 +564,8 @@ await MapAsync( targetProperty, associations, cache, - configuration.AttributePrefix + attributePrefix + configuration.AttributePrefix + attributePrefix, + mapPath ).ConfigureAwait(false); } } @@ -494,9 +574,9 @@ await MapAsync( | Determine value \-------------------------------------------------------------------------------------------------------------------------*/ else { - var value = await GetValue(source, propertyAccessor.Type, associations, propertyAccessor, cache, attributePrefix, mapAssociationsOnly).ConfigureAwait(false); + var value = await GetValue(source, propertyAccessor.Type, associations, propertyAccessor, cache, attributePrefix, mapAssociationsOnly, mapPath).ConfigureAwait(false); if (value is null && propertyAccessor.IsList) { - await SetCollectionValueAsync(source, target, associations, propertyAccessor, cache, attributePrefix).ConfigureAwait(false); + await SetCollectionValueAsync(source, target, associations, propertyAccessor, cache, attributePrefix, mapAssociationsOnly, mapPath).ConfigureAwait(false); } else if (value != null && propertyAccessor.CanWrite) { propertyAccessor.SetValue(target, value, true); @@ -523,6 +603,7 @@ await MapAsync( /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. /// Determines if properties not associated with associations should be mapped. + /// The current mapping request's path, used to detect circular references during construction. private async Task GetValue( Topic source, Type targetType, @@ -530,7 +611,8 @@ await MapAsync( ItemMetadata itemMetadata, MappedTopicCache cache, string? attributePrefix = "", - bool mapAssociationsOnly = false + bool mapAssociationsOnly = false, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -549,7 +631,7 @@ await MapAsync( /*-------------------------------------------------------------------------------------------------------------------------- | Handle by type, attribute \-------------------------------------------------------------------------------------------------------------------------*/ - if (TryGetCompatibleProperty(source, targetType, itemMetadata, attributePrefix, out var compatibleValue)) { + if (!mapAssociationsOnly && TryGetCompatibleProperty(source, targetType, itemMetadata, attributePrefix, out var compatibleValue)) { value = compatibleValue; } else if (itemMetadata.IsConvertible) { @@ -562,7 +644,7 @@ await MapAsync( } else if (configuration.GetCompositeAttributeKey(attributePrefix) is "Parent") { if (associations.HasFlag(AssociationTypes.Parents) && source.Parent is not null) { - value = await GetTopicReferenceAsync(source.Parent, targetType, itemMetadata, cache).ConfigureAwait(false); + value = await GetTopicReferenceAsync(source.Parent, targetType, itemMetadata, cache, mapPath).ConfigureAwait(false); } } else if (configuration.MapToParent) { @@ -571,7 +653,7 @@ await MapAsync( else if (itemMetadata.Type.IsClass && associations.HasFlag(AssociationTypes.References)) { var topicReference = await getTopicReference().ConfigureAwait(false); if (topicReference is not null) { - value = await GetTopicReferenceAsync(topicReference, targetType, itemMetadata, cache).ConfigureAwait(false); + value = await GetTopicReferenceAsync(topicReference, targetType, itemMetadata, cache, mapPath).ConfigureAwait(false); } } @@ -729,22 +811,43 @@ await MapAsync( /// The with details about the property's attributes. /// A cache to keep track of already-mapped object instances. /// The prefix to apply to the attributes. + /// Determines if properties not associated with associations should be mapped. + /// The current mapping request's path, used to detect circular references during construction. private async Task SetCollectionValueAsync( Topic source, object target, AssociationTypes associations, MemberAccessor memberAccessor, MappedTopicCache cache, - string? attributePrefix + string? attributePrefix, + bool mapAssociationsOnly, + MapPath? mapPath = null ) { + /*-------------------------------------------------------------------------------------------------------------------------- + | Establish per-entry lock + >--------------------------------------------------------------------------------------------------------------------------- + | Two concurrent passes expanding the same cached target with disjoint associations (see e.g., the cache-hit path in + | MapAsync) can populate the same member's target list from different sources, so its creation and population are + | synchronized on the shared cache entry. A target that isn't cached (i.e., a new topic) is never shared across passes, so + | its own instance serves as a sufficient and uncontended lock. + \-------------------------------------------------------------------------------------------------------------------------*/ + cache.TryGetValue(source.Id, target.GetType(), out var cacheEntry); + var collectionLock = (object?)cacheEntry?? target; + /*-------------------------------------------------------------------------------------------------------------------------- | Ensure target list is created + >--------------------------------------------------------------------------------------------------------------------------- + | Locked so two concurrent passes can't both observe a null list and instantiate competing instances, which would orphan the + | items added to whichever collection is overwritten. The lock is synchronous and never held across an await. \-------------------------------------------------------------------------------------------------------------------------*/ - var targetList = (IList?)memberAccessor.GetValue(target); - if (targetList is null) { - targetList = InitializeCollection(memberAccessor.Type); - memberAccessor.SetValue(target, targetList); + IList? targetList; + lock (collectionLock) { + targetList = (IList?)memberAccessor.GetValue(target); + if (targetList is null) { + targetList = InitializeCollection(memberAccessor.Type); + memberAccessor.SetValue(target, targetList); + } } Contract.Assume( @@ -756,7 +859,7 @@ private async Task SetCollectionValueAsync( /*-------------------------------------------------------------------------------------------------------------------------- | Establish source collection to store topics to be mapped \-------------------------------------------------------------------------------------------------------------------------*/ - var sourceList = await GetSourceCollectionAsync(source, associations, memberAccessor, attributePrefix).ConfigureAwait(false); + var sourceList = await GetSourceCollectionAsync(source, associations, memberAccessor, attributePrefix, mapAssociationsOnly).ConfigureAwait(false); /*-------------------------------------------------------------------------------------------------------------------------- | Validate that source collection was identified @@ -766,7 +869,7 @@ private async Task SetCollectionValueAsync( /*-------------------------------------------------------------------------------------------------------------------------- | Map the topics from the source collection, and add them to the target collection \-------------------------------------------------------------------------------------------------------------------------*/ - await PopulateTargetCollectionAsync(sourceList, targetList, memberAccessor, cache).ConfigureAwait(false); + await PopulateTargetCollectionAsync(sourceList, targetList, memberAccessor, cache, collectionLock, mapPath).ConfigureAwait(false); } @@ -788,11 +891,13 @@ private async Task SetCollectionValueAsync( /// Determines what associations the mapping should include, if any. /// The with details about the property's attributes. /// The prefix to apply to the attributes. + /// Determines if properties not associated with associations should be mapped. private async Task> GetSourceCollectionAsync( Topic source, AssociationTypes associations, ItemMetadata itemMetadata, - string? attributePrefix + string? attributePrefix, + bool mapAssociationsOnly ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -823,7 +928,7 @@ private async Task> GetSourceCollectionAsync( \-------------------------------------------------------------------------------------------------------------------------*/ listSource = getCollection( CollectionType.Relationship, - source.Relationships.Contains, + key => source.Relationships.Contains(key), () => source.Relationships.GetValues(collectionKey) ); @@ -832,7 +937,7 @@ private async Task> GetSourceCollectionAsync( \-------------------------------------------------------------------------------------------------------------------------*/ listSource = getCollection( CollectionType.NestedTopics, - source.Children.Contains, + key => source.Children.Contains(key), () => source.Children[collectionKey].Children ); @@ -841,7 +946,7 @@ private async Task> GetSourceCollectionAsync( \-------------------------------------------------------------------------------------------------------------------------*/ listSource = getCollection( CollectionType.IncomingRelationship, - source.IncomingRelationships.Contains, + key => source.IncomingRelationships.Contains(key), () => source.IncomingRelationships.GetValues(collectionKey) ); @@ -850,9 +955,11 @@ private async Task> GetSourceCollectionAsync( \-------------------------------------------------------------------------------------------------------------------------*/ //The following allows a target collection to be mapped to an IList source collection. This is valuable for custom, //curated collections defined on e.g. derivatives of Topic, but which don't otherwise map to a specific collection type. - //For example, the ContentTypeDescriptor's AttributeDescriptors collection, which provides a rollup of - //AttributeDescriptors from the current ContentTypeDescriptor, as well as all of its ascendents. - if (listSource.Count == 0) { + //For example, the ContentTypeDescriptor's AttributeDescriptors collection, which provides a rollup of AttributeDescriptors + //from the current ContentTypeDescriptor, as well as all of its ascendants. On an expansion pass, this fallback runs only + //when MappedCollections is among the claimed associations, avoiding a redundant reflective source-property read (and its + //re-enumeration) for passes that don't claim it. + if (listSource.Count == 0 && (!mapAssociationsOnly || associations.HasFlag(AssociationTypes.MappedCollections))) { var sourceProperty = TypeAccessorCache.GetTypeAccessor(source.GetType()).GetMember(configuration.GetCompositeAttributeKey(attributePrefix)); if ( sourceProperty?.GetValue(source) is IList sourcePropertyValue && @@ -870,9 +977,9 @@ sourcePropertyValue[0] is Topic /*-------------------------------------------------------------------------------------------------------------------------- | Handle Metadata relationship \-------------------------------------------------------------------------------------------------------------------------*/ - if (listSource.Count == 0 && !String.IsNullOrWhiteSpace(configuration.MetadataKey)) { + if (!mapAssociationsOnly && listSource.Count == 0 && !String.IsNullOrWhiteSpace(configuration.MetadataKey)) { var metadataKey = $"Root:Configuration:Metadata:{configuration.MetadataKey}:LookupList"; - var metadataParent = await _topicRepository.Load(metadataKey, source).ConfigureAwait(false); + var metadataParent = await _topicRepository.Load(metadataKey, source, TopicPayload.Children).ConfigureAwait(false); if (metadataParent is not null) { listSource = [.. metadataParent.Children]; } @@ -896,6 +1003,7 @@ IList getCollection(CollectionType collection, Func contain var targetAssociations = AssociationMap.Mappings[collection]; var preconditionsMet = listSource.Count == 0 && + (!mapAssociationsOnly || targetAssociations is not AssociationTypes.None) && (collectionType is CollectionType.Any || collectionType.Equals(collection)) && (collectionType is CollectionType.Children || collection is not CollectionType.Children) && (targetAssociations is AssociationTypes.None || associations.HasFlag(targetAssociations)) && @@ -915,11 +1023,19 @@ IList getCollection(CollectionType collection, Func contain /// The target to add the mapped objects to. /// The with details about the property's attributes. /// A cache to keep track of already-mapped object instances. + /// + /// The object to lock on while adding to , so concurrent passes populating a shared target's + /// list from disjoint sources don't modify it simultaneously. Callers pass the shared cache entry for a cached target, or + /// the (unshared) list itself when populating a constructor parameter. + /// + /// The current mapping request's path, used to detect circular references during construction. private async Task PopulateTargetCollectionAsync( IList sourceList, IList targetList, ItemMetadata itemMetadata, - MappedTopicCache cache + MappedTopicCache cache, + object collectionLock, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -974,7 +1090,7 @@ configuration.ContentTypeFilter is not null && if (!typeof(Topic).IsAssignableFrom(listType)) { var mappingType = GetValidatedMappingType(configuration.MapAs, listType)?? GetValidatedMappingType(childTopic, listType); if (mappingType is not null) { - taskQueue.Add(MapAsync(childTopic, mappingType, configuration.IncludeAssociations, cache)); + taskQueue.Add(MapAsync(childTopic, mappingType, configuration.IncludeAssociations, cache, mapPath: mapPath)); } } else { @@ -985,11 +1101,13 @@ configuration.ContentTypeFilter is not null && /*-------------------------------------------------------------------------------------------------------------------------- | Process mapping tasks + >--------------------------------------------------------------------------------------------------------------------------- + | Awaited as a batch, then added in the order the tasks were queued in, rather than completion order; this keeps sibling + | order deterministic regardless of which child mappings genuinely await \-------------------------------------------------------------------------------------------------------------------------*/ - while (taskQueue.Count > 0) { - var dtoTask = await Task.WhenAny(taskQueue).ConfigureAwait(false); - var dto = await dtoTask.ConfigureAwait(false); - taskQueue.Remove(dtoTask); + var dtos = await Task.WhenAll(taskQueue).ConfigureAwait(false); + + foreach (var dto in dtos) { if (dto is not null) { addToList(dto); } @@ -997,14 +1115,19 @@ configuration.ContentTypeFilter is not null && /*-------------------------------------------------------------------------------------------------------------------------- | Function: Add to List + >--------------------------------------------------------------------------------------------------------------------------- + | Locked so a concurrent pass populating the same shared list from a disjoint source can't add at the same time; the lock + | is synchronous and never held across an await, as child mapping happens outside of it via the above task queue. \-------------------------------------------------------------------------------------------------------------------------*/ void addToList(object dto) { - try { - targetList.Add(dto); - } - catch (ArgumentException) { - //Ignore exceptions caused by duplicate keys, in case the IList represents a keyed collection - //We would defensively check for this, except IList doesn't provide a suitable method to do so + lock (collectionLock) { + try { + targetList.Add(dto); + } + catch (ArgumentException) { + //Ignore exceptions caused by duplicate keys, in case the IList represents a keyed collection + //We would defensively check for this, except IList doesn't provide a suitable method to do so + } } } @@ -1048,11 +1171,13 @@ void addToList(object dto) { /// The expected for the mapped . /// The with details about the item's attributes. /// A cache to keep track of already-mapped object instances. + /// The current mapping request's path, used to detect circular references during construction. private async Task GetTopicReferenceAsync( Topic source, Type targetType, ItemMetadata itemMetadata, - MappedTopicCache cache + MappedTopicCache cache, + MapPath? mapPath = null ) { /*-------------------------------------------------------------------------------------------------------------------------- @@ -1076,7 +1201,7 @@ MappedTopicCache cache var mappingType = GetValidatedMappingType(configuration.MapAs, targetType)?? GetValidatedMappingType(source, targetType); if (mappingType is not null) { - topicDto = await MapAsync(source, mappingType, configuration.IncludeAssociations, cache).ConfigureAwait(false); + topicDto = await MapAsync(source, mappingType, configuration.IncludeAssociations, cache, mapPath: mapPath).ConfigureAwait(false); } /*--------------------------------------------------------------------------------------------------------------------------