From e0dec440d3dd27c02d9bf425ed0dd7839a771d0d Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 10:46:01 +0100 Subject: [PATCH 1/9] fix: discovery issue related to assemblies --- .../Hooks/Discovery/HooksDiscovery.cs | 60 ++++++---------- .../Discovery/LoadDatabaseValuesAttribute.cs | 4 +- .../Hooks/Execution/DiffableEntityHashSet.cs | 2 +- .../UnitTests/ResourceHooks/DiscoveryTests.cs | 70 +++++++++++-------- 4 files changed, 68 insertions(+), 68 deletions(-) diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index 9b95e24562..0aba0c7c39 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -1,16 +1,17 @@ using System; using System.Collections.Generic; using System.Linq; -using JsonApiDotNetCore.Graph; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Models; +using JsonApiDotNetCore.Services; +using Microsoft.Extensions.DependencyInjection; namespace JsonApiDotNetCore.Hooks { /// /// The default implementation for IHooksDiscovery /// - public class HooksDiscovery : IHooksDiscovery where TResource : class, IIdentifiable + public class HooksDiscovery : IHooksDiscovery where TEntity : class, IIdentifiable { private readonly ResourceHook[] _allHooks; private readonly ResourceHook[] _databaseValuesAttributeAllowed = @@ -19,71 +20,56 @@ public class HooksDiscovery : IHooksDiscovery where TResou ResourceHook.BeforeUpdateRelationship, ResourceHook.BeforeDelete }; + /// public ResourceHook[] ImplementedHooks { get; private set; } public ResourceHook[] DatabaseValuesEnabledHooks { get; private set; } public ResourceHook[] DatabaseValuesDisabledHooks { get; private set; } - - public HooksDiscovery() + public HooksDiscovery(IScopedServiceProvider provider) { _allHooks = Enum.GetValues(typeof(ResourceHook)) .Cast() .Where(h => h != ResourceHook.None) .ToArray(); - DiscoverImplementedHooksForModel(); + var container = provider.GetService(typeof(ResourceDefinition)); + if (container == null) + return; + DiscoverImplementedHooksForModel(container.GetType()); } /// /// Discovers the implemented hooks for a model. /// /// The implemented hooks for model. - void DiscoverImplementedHooksForModel() + void DiscoverImplementedHooksForModel(Type containerType) { - Type parameterizedResourceDefinition = typeof(ResourceDefinition); - var derivedTypes = TypeLocator.GetDerivedTypes(typeof(TResource).Assembly, parameterizedResourceDefinition).ToList(); - - var implementedHooks = new List(); - var enabledHooks = new List() { ResourceHook.BeforeImplicitUpdateRelationship } ; + var enabledHooks = new List() { ResourceHook.BeforeImplicitUpdateRelationship }; var disabledHooks = new List(); - Type targetType = null; - try - { - targetType = derivedTypes.SingleOrDefault(); // multiple containers is not supported - } - catch - { - throw new JsonApiSetupException($"It is currently not supported to" + - "implement hooks across multiple implementations of ResourceDefinition"); - } - if (targetType != null) + if (containerType != null) { foreach (var hook in _allHooks) { - var method = targetType.GetMethod(hook.ToString("G")); - if (method.DeclaringType != parameterizedResourceDefinition) + var method = containerType.GetMethod(hook.ToString("G")); + implementedHooks.Add(hook); + var attr = method.GetCustomAttributes(true).OfType().SingleOrDefault(); + if (attr != null) { - implementedHooks.Add(hook); - var attr = method.GetCustomAttributes(true).OfType().SingleOrDefault(); - if (attr != null) + if (!_databaseValuesAttributeAllowed.Contains(hook)) { - if (!_databaseValuesAttributeAllowed.Contains(hook)) - { - throw new JsonApiSetupException($"DatabaseValuesAttribute cannot be used on hook" + - $"{hook.ToString("G")} in resource definition {parameterizedResourceDefinition.Name}"); - } - var targetList = attr.value ? enabledHooks : disabledHooks; - targetList.Add(hook); + throw new JsonApiSetupException($"DatabaseValuesAttribute cannot be used on hook" + + $"{hook.ToString("G")} in resource definition {containerType.Name}"); } - } + var targetList = attr.value ? enabledHooks : disabledHooks; + targetList.Add(hook); + } } } ImplementedHooks = implementedHooks.ToArray(); DatabaseValuesDisabledHooks = disabledHooks.ToArray(); DatabaseValuesEnabledHooks = enabledHooks.ToArray(); - } } -} +} \ No newline at end of file diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/LoadDatabaseValuesAttribute.cs b/src/JsonApiDotNetCore/Hooks/Discovery/LoadDatabaseValuesAttribute.cs index 1477cc0ec1..6a47e9d2a0 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/LoadDatabaseValuesAttribute.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/LoadDatabaseValuesAttribute.cs @@ -1,10 +1,10 @@ using System; namespace JsonApiDotNetCore.Hooks { - public class LoaDatabaseValues : Attribute + public class LoadDatabaseValues : Attribute { public readonly bool value; - public LoaDatabaseValues(bool mode = true) + public LoadDatabaseValues(bool mode = true) { value = mode; } diff --git a/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs b/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs index c43ae530c4..0088bf53b3 100644 --- a/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs +++ b/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs @@ -90,7 +90,7 @@ public IEnumerable> GetDiffs() private HashSet ThrowNoDbValuesError() { - throw new MemberAccessException("Cannot iterate over the diffs if the LoaDatabaseValues option is set to false"); + throw new MemberAccessException("Cannot iterate over the diffs if the LoadDatabaseValues option is set to false"); } } diff --git a/test/UnitTests/ResourceHooks/DiscoveryTests.cs b/test/UnitTests/ResourceHooks/DiscoveryTests.cs index 3a37184784..69f59c00b4 100644 --- a/test/UnitTests/ResourceHooks/DiscoveryTests.cs +++ b/test/UnitTests/ResourceHooks/DiscoveryTests.cs @@ -5,6 +5,9 @@ using JsonApiDotNetCore.Builders; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Internal.Contracts; +using System; +using Moq; +using JsonApiDotNetCore.Services; namespace UnitTests.ResourceHooks { @@ -19,11 +22,19 @@ public DummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource entities, ResourcePipeline pipeline, bool succeeded) { } } + private IScopedServiceProvider MockProvider(object service) + { + var mock = new Mock(); + mock.Setup(m => m.GetService(It.IsAny())).Returns(service); + return mock.Object; + } + + [Fact] public void Hook_Discovery() { // Arrange & act - var hookConfig = new HooksDiscovery(); + var hookConfig = new HooksDiscovery(MockProvider(new DummyResourceDefinition())); // Assert Assert.Contains(ResourceHook.BeforeDelete, hookConfig.ImplementedHooks); Assert.Contains(ResourceHook.AfterDelete, hookConfig.ImplementedHooks); @@ -33,7 +44,9 @@ public void Hook_Discovery() public class AnotherDummy : Identifiable { } public abstract class ResourceDefintionBase : ResourceDefinition where T : class, IIdentifiable { - protected ResourceDefintionBase(IResourceGraph resourceGraph) : base(resourceGraph) { } + public ResourceDefintionBase(IResourceGraph resourceGraph) : base(resourceGraph) + { + } public override IEnumerable BeforeDelete(IEntityHashSet affected, ResourcePipeline pipeline) { return affected; } public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } @@ -41,13 +54,13 @@ public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, public class AnotherDummyResourceDefinition : ResourceDefintionBase { - public AnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource().Build()) { } + public AnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource().Build()) { } } [Fact] public void Hook_Discovery_With_Inheritance() { // Arrange & act - var hookConfig = new HooksDiscovery(); + var hookConfig = new HooksDiscovery(MockProvider(new AnotherDummyResourceDefinition())); // Assert Assert.Contains(ResourceHook.BeforeDelete, hookConfig.ImplementedHooks); Assert.Contains(ResourceHook.AfterDelete, hookConfig.ImplementedHooks); @@ -61,43 +74,44 @@ public YetAnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().Add public override IEnumerable BeforeDelete(IEntityHashSet affected, ResourcePipeline pipeline) { return affected; } - [LoaDatabaseValues(false)] + [LoadDatabaseValues(false)] public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } } [Fact] - public void LoaDatabaseValues_Attribute_Not_Allowed() + public void LoadDatabaseValues_Attribute_Not_Allowed() { // assert Assert.Throws(() => { // Arrange & act - var hookConfig = new HooksDiscovery(); + var hookConfig = new HooksDiscovery(MockProvider(new YetAnotherDummyResourceDefinition())); }); } - public class DoubleDummy : Identifiable { } - public class DoubleDummyResourceDefinition1 : ResourceDefinition - { - public DoubleDummyResourceDefinition1() : base(new ResourceGraphBuilder().AddResource().Build()) { } + //public class DoubleDummy : Identifiable { } + //public class DoubleDummyResourceDefinition1 : ResourceDefinition + //{ + // public DoubleDummyResourceDefinition1() : base(new ResourceGraphBuilder().AddResource().Build()) { } - public override IEnumerable BeforeDelete(IEntityHashSet affected, ResourcePipeline pipeline) { return affected; } - } - public class DoubleDummyResourceDefinition2 : ResourceDefinition - { - public DoubleDummyResourceDefinition2() : base(new ResourceGraphBuilder().AddResource().Build()) { } + // public override IEnumerable BeforeDelete(IEntityHashSet affected, ResourcePipeline pipeline) { return affected; } + //} + //public class DoubleDummyResourceDefinition2 : ResourceDefinition + //{ + // public DoubleDummyResourceDefinition2() : base(new ResourceGraphBuilder().AddResource().Build()) { } - public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } - } - [Fact] - public void Multiple_Implementations_Of_ResourceDefinitions() - { - // assert - Assert.Throws(() => - { - // Arrange & act - var hookConfig = new HooksDiscovery(); - }); - } + // public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } + //} + //[Fact] + //public void Multiple_Implementations_Of_ResourceDefinitions() + //{ + // // assert + // Assert.Throws(() => + // { + // // Arrange & act + // new List<> { new DoubleDummyResourceDefinition1(), new DoubleDummyResourceDefinition2() }; + // var hookConfig = new HooksDiscovery(MockProvider( )); + // }); + //} } } From 3c7412514d0cf1060b2fcb78cb71697fd7981a1c Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 11:03:46 +0100 Subject: [PATCH 2/9] fix: LoaDatabaseValue typo --- .../Hooks/Discovery/HooksDiscovery.cs | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index 0aba0c7c39..cb41e4266d 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -4,7 +4,6 @@ using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Models; using JsonApiDotNetCore.Services; -using Microsoft.Extensions.DependencyInjection; namespace JsonApiDotNetCore.Hooks { @@ -13,6 +12,7 @@ namespace JsonApiDotNetCore.Hooks /// public class HooksDiscovery : IHooksDiscovery where TEntity : class, IIdentifiable { + private readonly Type _boundResourceDefinitionType = typeof(ResourceDefinition); private readonly ResourceHook[] _allHooks; private readonly ResourceHook[] _databaseValuesAttributeAllowed = { @@ -32,10 +32,10 @@ public HooksDiscovery(IScopedServiceProvider provider) .Cast() .Where(h => h != ResourceHook.None) .ToArray(); - var container = provider.GetService(typeof(ResourceDefinition)); - if (container == null) + var containerType = provider.GetService(_boundResourceDefinitionType)?.GetType(); + if (containerType == null || containerType == _boundResourceDefinitionType) return; - DiscoverImplementedHooksForModel(container.GetType()); + DiscoverImplementedHooksForModel(containerType); } /// @@ -45,28 +45,28 @@ public HooksDiscovery(IScopedServiceProvider provider) void DiscoverImplementedHooksForModel(Type containerType) { var implementedHooks = new List(); - var enabledHooks = new List() { ResourceHook.BeforeImplicitUpdateRelationship }; + var enabledHooks = new List { ResourceHook.BeforeImplicitUpdateRelationship }; var disabledHooks = new List(); - if (containerType != null) + foreach (var hook in _allHooks) { - foreach (var hook in _allHooks) + var method = containerType.GetMethod(hook.ToString("G")); + if (method.DeclaringType == _boundResourceDefinitionType) + continue; + + implementedHooks.Add(hook); + var attr = method.GetCustomAttributes(true).OfType().SingleOrDefault(); + if (attr != null) { - var method = containerType.GetMethod(hook.ToString("G")); - implementedHooks.Add(hook); - var attr = method.GetCustomAttributes(true).OfType().SingleOrDefault(); - if (attr != null) + if (!_databaseValuesAttributeAllowed.Contains(hook)) { - if (!_databaseValuesAttributeAllowed.Contains(hook)) - { - throw new JsonApiSetupException($"DatabaseValuesAttribute cannot be used on hook" + - $"{hook.ToString("G")} in resource definition {containerType.Name}"); - } - var targetList = attr.value ? enabledHooks : disabledHooks; - targetList.Add(hook); + throw new JsonApiSetupException($"DatabaseValuesAttribute cannot be used on hook" + + $"{hook.ToString("G")} in resource definition {containerType.Name}"); } + var targetList = attr.value ? enabledHooks : disabledHooks; + targetList.Add(hook); } - } + ImplementedHooks = implementedHooks.ToArray(); DatabaseValuesDisabledHooks = disabledHooks.ToArray(); DatabaseValuesEnabledHooks = enabledHooks.ToArray(); From ef10ebfe4de5bebf7ca874c873d777b0822025a3 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 11:04:06 +0100 Subject: [PATCH 3/9] fix: set EnableResourceHooks to true by default --- src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs b/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs index cb76720a75..88be3369af 100644 --- a/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs +++ b/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs @@ -44,7 +44,7 @@ public class JsonApiOptions : IJsonApiOptions /// /// Default is set to for backward compatibility. /// - public bool EnableResourceHooks { get; set; } = false; + public bool EnableResourceHooks { get; set; } = true; /// /// Whether or not database values should be included by default From ce6a0de3f190978245725151d4ee833000424c9f Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 11:06:57 +0100 Subject: [PATCH 4/9] chore: add comment in code in hooksdiscovery --- .../Hooks/Discovery/HooksDiscovery.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index cb41e4266d..ce9c107d68 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -45,8 +45,8 @@ public HooksDiscovery(IScopedServiceProvider provider) void DiscoverImplementedHooksForModel(Type containerType) { var implementedHooks = new List(); - var enabledHooks = new List { ResourceHook.BeforeImplicitUpdateRelationship }; - var disabledHooks = new List(); + var databaseValuesEnabledHooks = new List { ResourceHook.BeforeImplicitUpdateRelationship }; // this hook can only be used with enabled database values + var databaseValuesDisabledHooks = new List(); foreach (var hook in _allHooks) { var method = containerType.GetMethod(hook.ToString("G")); @@ -62,14 +62,14 @@ void DiscoverImplementedHooksForModel(Type containerType) throw new JsonApiSetupException($"DatabaseValuesAttribute cannot be used on hook" + $"{hook.ToString("G")} in resource definition {containerType.Name}"); } - var targetList = attr.value ? enabledHooks : disabledHooks; + var targetList = attr.value ? databaseValuesEnabledHooks : databaseValuesDisabledHooks; targetList.Add(hook); } } ImplementedHooks = implementedHooks.ToArray(); - DatabaseValuesDisabledHooks = disabledHooks.ToArray(); - DatabaseValuesEnabledHooks = enabledHooks.ToArray(); + DatabaseValuesDisabledHooks = databaseValuesDisabledHooks.ToArray(); + DatabaseValuesEnabledHooks = databaseValuesEnabledHooks.ToArray(); } } } \ No newline at end of file From 914f790683d02bbdf47d29f94dcd2fc3eff9f3a8 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 11:21:53 +0100 Subject: [PATCH 5/9] fix: use IServiceProvider instead of IScopedServiceProvider --- .../Hooks/Discovery/HooksDiscovery.cs | 7 +++-- .../UnitTests/ResourceHooks/DiscoveryTests.cs | 29 ++----------------- 2 files changed, 6 insertions(+), 30 deletions(-) diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index ce9c107d68..fd3000f7c5 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -3,7 +3,7 @@ using System.Linq; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Models; -using JsonApiDotNetCore.Services; +using Microsoft.Extensions.DependencyInjection; namespace JsonApiDotNetCore.Hooks { @@ -26,13 +26,14 @@ public class HooksDiscovery : IHooksDiscovery where TEntity : public ResourceHook[] DatabaseValuesEnabledHooks { get; private set; } public ResourceHook[] DatabaseValuesDisabledHooks { get; private set; } - public HooksDiscovery(IScopedServiceProvider provider) + public HooksDiscovery(IServiceProvider provider) { _allHooks = Enum.GetValues(typeof(ResourceHook)) .Cast() .Where(h => h != ResourceHook.None) .ToArray(); - var containerType = provider.GetService(_boundResourceDefinitionType)?.GetType(); + using var scope = provider.CreateScope(); + var containerType = scope.ServiceProvider.GetService(_boundResourceDefinitionType)?.GetType(); if (containerType == null || containerType == _boundResourceDefinitionType) return; DiscoverImplementedHooksForModel(containerType); diff --git a/test/UnitTests/ResourceHooks/DiscoveryTests.cs b/test/UnitTests/ResourceHooks/DiscoveryTests.cs index 69f59c00b4..165f86f5d7 100644 --- a/test/UnitTests/ResourceHooks/DiscoveryTests.cs +++ b/test/UnitTests/ResourceHooks/DiscoveryTests.cs @@ -22,9 +22,9 @@ public DummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource entities, ResourcePipeline pipeline, bool succeeded) { } } - private IScopedServiceProvider MockProvider(object service) + private IServiceProvider MockProvider(object service) { - var mock = new Mock(); + var mock = new Mock(); mock.Setup(m => m.GetService(It.IsAny())).Returns(service); return mock.Object; } @@ -88,30 +88,5 @@ public void LoadDatabaseValues_Attribute_Not_Allowed() }); } - - //public class DoubleDummy : Identifiable { } - //public class DoubleDummyResourceDefinition1 : ResourceDefinition - //{ - // public DoubleDummyResourceDefinition1() : base(new ResourceGraphBuilder().AddResource().Build()) { } - - // public override IEnumerable BeforeDelete(IEntityHashSet affected, ResourcePipeline pipeline) { return affected; } - //} - //public class DoubleDummyResourceDefinition2 : ResourceDefinition - //{ - // public DoubleDummyResourceDefinition2() : base(new ResourceGraphBuilder().AddResource().Build()) { } - - // public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } - //} - //[Fact] - //public void Multiple_Implementations_Of_ResourceDefinitions() - //{ - // // assert - // Assert.Throws(() => - // { - // // Arrange & act - // new List<> { new DoubleDummyResourceDefinition1(), new DoubleDummyResourceDefinition2() }; - // var hookConfig = new HooksDiscovery(MockProvider( )); - // }); - //} } } From 4cc91ca9f29158900608d5cd1529c7f29affd9aa Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 11:30:13 +0100 Subject: [PATCH 6/9] fix: discovery unit test with ServiceProvider instead of ScopedServiceProvider --- .../UnitTests/ResourceHooks/DiscoveryTests.cs | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/test/UnitTests/ResourceHooks/DiscoveryTests.cs b/test/UnitTests/ResourceHooks/DiscoveryTests.cs index 165f86f5d7..3c3dab0708 100644 --- a/test/UnitTests/ResourceHooks/DiscoveryTests.cs +++ b/test/UnitTests/ResourceHooks/DiscoveryTests.cs @@ -6,8 +6,7 @@ using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Internal.Contracts; using System; -using Moq; -using JsonApiDotNetCore.Services; +using Microsoft.Extensions.DependencyInjection; namespace UnitTests.ResourceHooks { @@ -22,32 +21,27 @@ public DummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource entities, ResourcePipeline pipeline, bool succeeded) { } } - private IServiceProvider MockProvider(object service) + private IServiceProvider MockProvider(object service) where TResource : class, IIdentifiable { - var mock = new Mock(); - mock.Setup(m => m.GetService(It.IsAny())).Returns(service); - return mock.Object; + var services = new ServiceCollection(); + services.AddScoped((_) => (ResourceDefinition)service); + return services.BuildServiceProvider(); } - [Fact] public void Hook_Discovery() { // Arrange & act - var hookConfig = new HooksDiscovery(MockProvider(new DummyResourceDefinition())); + var hookConfig = new HooksDiscovery(MockProvider(new DummyResourceDefinition())); // Assert Assert.Contains(ResourceHook.BeforeDelete, hookConfig.ImplementedHooks); Assert.Contains(ResourceHook.AfterDelete, hookConfig.ImplementedHooks); - } public class AnotherDummy : Identifiable { } public abstract class ResourceDefintionBase : ResourceDefinition where T : class, IIdentifiable { - public ResourceDefintionBase(IResourceGraph resourceGraph) : base(resourceGraph) - { - } - + public ResourceDefintionBase(IResourceGraph resourceGraph) : base(resourceGraph) { } public override IEnumerable BeforeDelete(IEntityHashSet affected, ResourcePipeline pipeline) { return affected; } public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } } @@ -56,17 +50,17 @@ public class AnotherDummyResourceDefinition : ResourceDefintionBase().Build()) { } } + [Fact] public void Hook_Discovery_With_Inheritance() { // Arrange & act - var hookConfig = new HooksDiscovery(MockProvider(new AnotherDummyResourceDefinition())); + var hookConfig = new HooksDiscovery(MockProvider(new AnotherDummyResourceDefinition())); // Assert Assert.Contains(ResourceHook.BeforeDelete, hookConfig.ImplementedHooks); Assert.Contains(ResourceHook.AfterDelete, hookConfig.ImplementedHooks); } - public class YetAnotherDummy : Identifiable { } public class YetAnotherDummyResourceDefinition : ResourceDefinition { @@ -77,6 +71,7 @@ public YetAnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().Add [LoadDatabaseValues(false)] public override void AfterDelete(HashSet entities, ResourcePipeline pipeline, bool succeeded) { } } + [Fact] public void LoadDatabaseValues_Attribute_Not_Allowed() { @@ -84,7 +79,7 @@ public void LoadDatabaseValues_Attribute_Not_Allowed() Assert.Throws(() => { // Arrange & act - var hookConfig = new HooksDiscovery(MockProvider(new YetAnotherDummyResourceDefinition())); + var hookConfig = new HooksDiscovery(MockProvider(new YetAnotherDummyResourceDefinition())); }); } From 92ceff29fceb3a6b8c866f1031966c3d2a93387b Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 11:35:06 +0100 Subject: [PATCH 7/9] refactor: TResource instead of TEntity --- src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index fd3000f7c5..4510493515 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -10,9 +10,9 @@ namespace JsonApiDotNetCore.Hooks /// /// The default implementation for IHooksDiscovery /// - public class HooksDiscovery : IHooksDiscovery where TEntity : class, IIdentifiable + public class HooksDiscovery : IHooksDiscovery where TResource : class, IIdentifiable { - private readonly Type _boundResourceDefinitionType = typeof(ResourceDefinition); + private readonly Type _boundResourceDefinitionType = typeof(ResourceDefinition); private readonly ResourceHook[] _allHooks; private readonly ResourceHook[] _databaseValuesAttributeAllowed = { From 5796a640573fad3bc00311eee085e540e404b031 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 18:29:28 +0100 Subject: [PATCH 8/9] chore: process PR review --- .../Configuration/JsonApiOptions.cs | 2 +- .../Hooks/Discovery/HooksDiscovery.cs | 20 +++++++++++++------ .../Hooks/Execution/DiffableEntityHashSet.cs | 2 +- test/UnitTests/Graph/TypeLocator_Tests.cs | 1 - 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs b/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs index 88be3369af..8530cd8fa2 100644 --- a/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs +++ b/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs @@ -42,7 +42,7 @@ public class JsonApiOptions : IJsonApiOptions /// /// Whether or not ResourceHooks are enabled. /// - /// Default is set to for backward compatibility. + /// Default is set to /// public bool EnableResourceHooks { get; set; } = true; diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index 4510493515..93f31e3b1d 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -32,19 +32,27 @@ public HooksDiscovery(IServiceProvider provider) .Cast() .Where(h => h != ResourceHook.None) .ToArray(); - using var scope = provider.CreateScope(); - var containerType = scope.ServiceProvider.GetService(_boundResourceDefinitionType)?.GetType(); - if (containerType == null || containerType == _boundResourceDefinitionType) - return; - DiscoverImplementedHooksForModel(containerType); + + Type containerType; + using (var scope = provider.CreateScope()) + { + containerType = scope.ServiceProvider.GetService(_boundResourceDefinitionType)?.GetType(); + } + + DiscoverImplementedHooks(containerType); } /// /// Discovers the implemented hooks for a model. /// /// The implemented hooks for model. - void DiscoverImplementedHooksForModel(Type containerType) + void DiscoverImplementedHooks(Type containerType) { + if (containerType == null || containerType == _boundResourceDefinitionType) + { + return; + } + var implementedHooks = new List(); var databaseValuesEnabledHooks = new List { ResourceHook.BeforeImplicitUpdateRelationship }; // this hook can only be used with enabled database values var databaseValuesDisabledHooks = new List(); diff --git a/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs b/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs index 0088bf53b3..e0de8a11ff 100644 --- a/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs +++ b/src/JsonApiDotNetCore/Hooks/Execution/DiffableEntityHashSet.cs @@ -90,7 +90,7 @@ public IEnumerable> GetDiffs() private HashSet ThrowNoDbValuesError() { - throw new MemberAccessException("Cannot iterate over the diffs if the LoadDatabaseValues option is set to false"); + throw new MemberAccessException($"Cannot iterate over the diffs if the ${nameof(LoadDatabaseValues)} option is set to false"); } } diff --git a/test/UnitTests/Graph/TypeLocator_Tests.cs b/test/UnitTests/Graph/TypeLocator_Tests.cs index 502cfa1139..4c727b0253 100644 --- a/test/UnitTests/Graph/TypeLocator_Tests.cs +++ b/test/UnitTests/Graph/TypeLocator_Tests.cs @@ -1,5 +1,4 @@ using System; -using System.Reflection; using JsonApiDotNetCore.Graph; using JsonApiDotNetCore.Models; using Xunit; From 2c6796a6218fe568b8349521945ec32d6fd6667b Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Wed, 6 Nov 2019 18:30:31 +0100 Subject: [PATCH 9/9] chore: move comment to separate line (stylecop) --- src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs index 93f31e3b1d..bfade1c50d 100644 --- a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs +++ b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs @@ -54,7 +54,8 @@ void DiscoverImplementedHooks(Type containerType) } var implementedHooks = new List(); - var databaseValuesEnabledHooks = new List { ResourceHook.BeforeImplicitUpdateRelationship }; // this hook can only be used with enabled database values + // this hook can only be used with enabled database values + var databaseValuesEnabledHooks = new List { ResourceHook.BeforeImplicitUpdateRelationship }; var databaseValuesDisabledHooks = new List(); foreach (var hook in _allHooks) {