Hooks discovery detection of implemented hooks issue (#617)

maurei · web-flow · commit 36d76358bbe6 · 2019-11-06T18:30:54.000+01:00
* fix: discovery issue related to assemblies

* fix: LoaDatabaseValue typo

* fix: set EnableResourceHooks to true by default

* chore: add comment in code in hooksdiscovery

* fix: use IServiceProvider instead of IScopedServiceProvider

* fix: discovery unit test with ServiceProvider instead of ScopedServiceProvider

* refactor: TResource instead of TEntity

* chore: process PR review

* chore: move comment to separate line (stylecop)
diff --git a/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs b/src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs
@@ -42,9 +42,9 @@ public class JsonApiOptions : IJsonApiOptions
         /// <summary>
         /// Whether or not ResourceHooks are enabled. 
         /// 
-        /// Default is set to <see langword="false"/> for backward compatibility.
+        /// Default is set to <see langword="true"/>
         /// </summary>
-        public bool EnableResourceHooks { get; set; } = false;
+        public bool EnableResourceHooks { get; set; } = true;
 
         /// <summary>
         /// Whether or not database values should be included by default
diff --git a/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs b/src/JsonApiDotNetCore/Hooks/Discovery/HooksDiscovery.cs
@@ -1,9 +1,9 @@
 ﻿using System;
 using System.Collections.Generic;
 using System.Linq;
-using JsonApiDotNetCore.Graph;
 using JsonApiDotNetCore.Internal;
 using JsonApiDotNetCore.Models;
+using Microsoft.Extensions.DependencyInjection;
 
 namespace JsonApiDotNetCore.Hooks
 {
@@ -12,78 +12,74 @@ namespace JsonApiDotNetCore.Hooks
     /// </summary>
     public class HooksDiscovery<TResource> : IHooksDiscovery<TResource> where TResource : class, IIdentifiable
     {
+        private readonly Type _boundResourceDefinitionType = typeof(ResourceDefinition<TResource>);
         private readonly ResourceHook[] _allHooks;
         private readonly ResourceHook[] _databaseValuesAttributeAllowed =
         {
             ResourceHook.BeforeUpdate,
             ResourceHook.BeforeUpdateRelationship,
             ResourceHook.BeforeDelete
         };
+
         /// <inheritdoc/>
         public ResourceHook[] ImplementedHooks { get; private set; }
         public ResourceHook[] DatabaseValuesEnabledHooks { get; private set; }
         public ResourceHook[] DatabaseValuesDisabledHooks { get; private set; }
 
-
-        public HooksDiscovery()
+        public HooksDiscovery(IServiceProvider provider)
         {
             _allHooks = Enum.GetValues(typeof(ResourceHook))
                             .Cast<ResourceHook>()
                             .Where(h => h != ResourceHook.None)
                             .ToArray();
-            DiscoverImplementedHooksForModel();
+
+            Type containerType;
+            using (var scope = provider.CreateScope())
+            {
+                containerType = scope.ServiceProvider.GetService(_boundResourceDefinitionType)?.GetType();
+            }
+
+            DiscoverImplementedHooks(containerType);
         }
 
         /// <summary>
         /// Discovers the implemented hooks for a model.
         /// </summary>
         /// <returns>The implemented hooks for model.</returns>
-        void DiscoverImplementedHooksForModel()
+        void DiscoverImplementedHooks(Type containerType)
         {
-            Type parameterizedResourceDefinition = typeof(ResourceDefinition<TResource>);
-            var derivedTypes = TypeLocator.GetDerivedTypes(typeof(TResource).Assembly, parameterizedResourceDefinition).ToList();
-
-
-            var implementedHooks = new List<ResourceHook>();
-            var enabledHooks = new List<ResourceHook>() { ResourceHook.BeforeImplicitUpdateRelationship } ;
-            var disabledHooks = new List<ResourceHook>();
-            Type targetType = null;
-            try
+            if (containerType == null || containerType == _boundResourceDefinitionType)
             {
-                targetType = derivedTypes.SingleOrDefault(); // multiple containers is not supported
+                return;
             }
-            catch
-            {
-                throw new JsonApiSetupException($"It is currently not supported to" +
-                	"implement hooks across multiple implementations of ResourceDefinition<T>");
-            }
-            if (targetType != null)
+
+            var implementedHooks = new List<ResourceHook>();
+            // this hook can only be used with enabled database values
+            var databaseValuesEnabledHooks = new List<ResourceHook> { ResourceHook.BeforeImplicitUpdateRelationship };
+            var databaseValuesDisabledHooks = new List<ResourceHook>();
+            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<LoadDatabaseValues>().SingleOrDefault();
+                if (attr != null)
                 {
-                    var method = targetType.GetMethod(hook.ToString("G"));
-                    if (method.DeclaringType != parameterizedResourceDefinition)
+                    if (!_databaseValuesAttributeAllowed.Contains(hook))
                     {
-                        implementedHooks.Add(hook);
-                        var attr = method.GetCustomAttributes(true).OfType<LoaDatabaseValues>().SingleOrDefault();
-                        if (attr != null)
-                        {
-                            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 ? databaseValuesEnabledHooks : databaseValuesDisabledHooks;
+                    targetList.Add(hook);
                 }
-
             }
-            ImplementedHooks = implementedHooks.ToArray();
-            DatabaseValuesDisabledHooks = disabledHooks.ToArray();
-            DatabaseValuesEnabledHooks = enabledHooks.ToArray();
 
+            ImplementedHooks = implementedHooks.ToArray();
+            DatabaseValuesDisabledHooks = databaseValuesDisabledHooks.ToArray();
+            DatabaseValuesEnabledHooks = databaseValuesEnabledHooks.ToArray();
         }
     }
-}
+}
diff --git 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
@@ -90,7 +90,7 @@ public IEnumerable<EntityDiffPair<TResource>> GetDiffs()
 
         private HashSet<TResource> 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 ${nameof(LoadDatabaseValues)} option is set to false");
         }
     }
 
diff --git 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;
diff --git a/test/UnitTests/ResourceHooks/DiscoveryTests.cs b/test/UnitTests/ResourceHooks/DiscoveryTests.cs
@@ -5,6 +5,8 @@
 using JsonApiDotNetCore.Builders;
 using JsonApiDotNetCore.Internal;
 using JsonApiDotNetCore.Internal.Contracts;
+using System;
+using Microsoft.Extensions.DependencyInjection;
 
 namespace UnitTests.ResourceHooks
 {
@@ -19,85 +21,67 @@ public DummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource<D
             public override void AfterDelete(HashSet<Dummy> entities, ResourcePipeline pipeline, bool succeeded) { }
         }
 
+        private IServiceProvider MockProvider<TResource>(object service) where TResource : class, IIdentifiable
+        {
+            var services = new ServiceCollection();
+            services.AddScoped((_) => (ResourceDefinition<TResource>)service);
+            return services.BuildServiceProvider();
+        }
+
         [Fact]
         public void Hook_Discovery()
         {
             // Arrange & act
-            var hookConfig = new HooksDiscovery<Dummy>();
+            var hookConfig = new HooksDiscovery<Dummy>(MockProvider<Dummy>(new DummyResourceDefinition()));
             // Assert
             Assert.Contains(ResourceHook.BeforeDelete, hookConfig.ImplementedHooks);
             Assert.Contains(ResourceHook.AfterDelete, hookConfig.ImplementedHooks);
-
         }
 
         public class AnotherDummy : Identifiable { }
         public abstract class ResourceDefintionBase<T> : ResourceDefinition<T> where T : class, IIdentifiable
         {
-            protected ResourceDefintionBase(IResourceGraph resourceGraph) : base(resourceGraph) { }
-
+            public ResourceDefintionBase(IResourceGraph resourceGraph) : base(resourceGraph) { }
             public override IEnumerable<T> BeforeDelete(IEntityHashSet<T> affected, ResourcePipeline pipeline) { return affected; }
             public override void AfterDelete(HashSet<T> entities, ResourcePipeline pipeline, bool succeeded) { }
         }
 
         public class AnotherDummyResourceDefinition : ResourceDefintionBase<AnotherDummy>
         {
-            public AnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource<Dummy>().Build()) { }
+            public AnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource<AnotherDummy>().Build()) { }
         }
+
         [Fact]
         public void Hook_Discovery_With_Inheritance()
         {
             // Arrange & act
-            var hookConfig = new HooksDiscovery<AnotherDummy>();
+            var hookConfig = new HooksDiscovery<AnotherDummy>(MockProvider<AnotherDummy>(new AnotherDummyResourceDefinition()));
             // Assert
             Assert.Contains(ResourceHook.BeforeDelete, hookConfig.ImplementedHooks);
             Assert.Contains(ResourceHook.AfterDelete, hookConfig.ImplementedHooks);
         }
 
-
         public class YetAnotherDummy : Identifiable { }
         public class YetAnotherDummyResourceDefinition : ResourceDefinition<YetAnotherDummy>
         {
             public YetAnotherDummyResourceDefinition() : base(new ResourceGraphBuilder().AddResource<YetAnotherDummy>().Build()) { }
 
             public override IEnumerable<YetAnotherDummy> BeforeDelete(IEntityHashSet<YetAnotherDummy> affected, ResourcePipeline pipeline) { return affected; }
 
-            [LoaDatabaseValues(false)]
+            [LoadDatabaseValues(false)]
             public override void AfterDelete(HashSet<YetAnotherDummy> entities, ResourcePipeline pipeline, bool succeeded) { }
         }
-        [Fact]
-        public void LoaDatabaseValues_Attribute_Not_Allowed()
-        {
-            //  assert
-            Assert.Throws<JsonApiSetupException>(() =>
-            {
-                // Arrange & act
-                var hookConfig = new HooksDiscovery<YetAnotherDummy>();
-            });
-
-        }
-
-        public class DoubleDummy : Identifiable { }
-        public class DoubleDummyResourceDefinition1 : ResourceDefinition<DoubleDummy>
-        {
-            public DoubleDummyResourceDefinition1() : base(new ResourceGraphBuilder().AddResource<DoubleDummy>().Build()) { }
 
-            public override IEnumerable<DoubleDummy> BeforeDelete(IEntityHashSet<DoubleDummy> affected, ResourcePipeline pipeline) { return affected; }
-        }
-        public class DoubleDummyResourceDefinition2 : ResourceDefinition<DoubleDummy>
-        {
-            public DoubleDummyResourceDefinition2() : base(new ResourceGraphBuilder().AddResource<DoubleDummy>().Build()) { }
-
-            public override void AfterDelete(HashSet<DoubleDummy> entities, ResourcePipeline pipeline, bool succeeded) { }
-        }
         [Fact]
-        public void Multiple_Implementations_Of_ResourceDefinitions()
+        public void LoadDatabaseValues_Attribute_Not_Allowed()
         {
             //  assert
             Assert.Throws<JsonApiSetupException>(() =>
             {
                 // Arrange & act
-                var hookConfig = new HooksDiscovery<DoubleDummy>();
+                var hookConfig = new HooksDiscovery<YetAnotherDummy>(MockProvider<YetAnotherDummy>(new YetAnotherDummyResourceDefinition()));
             });
+
         }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -1,10 +1,10 @@`
`1`	`1`	`using System;`
`2`	`2`	`namespace JsonApiDotNetCore.Hooks`
`3`	`3`	`{`
`4`		`- public class LoaDatabaseValues : Attribute`
	`4`	`+ public class LoadDatabaseValues : Attribute`
`5`	`5`	`{`
`6`	`6`	`public readonly bool value;`
`7`		`- public LoaDatabaseValues(bool mode = true)`
	`7`	`+ public LoadDatabaseValues(bool mode = true)`
`8`	`8`	`{`
`9`	`9`	`value = mode;`
`10`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -90,7 +90,7 @@ public IEnumerable<EntityDiffPair<TResource>> GetDiffs()`
`90`	`90`
`91`	`91`	`private HashSet<TResource> ThrowNoDbValuesError()`
`92`	`92`	`{`
`93`		`- throw new MemberAccessException("Cannot iterate over the diffs if the LoaDatabaseValues option is set to false");`
	`93`	`+ throw new MemberAccessException($"Cannot iterate over the diffs if the ${nameof(LoadDatabaseValues)} option is set to false");`
`94`	`94`	`}`
`95`	`95`	`}`
`96`	`96`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`using System;`
`2`		`-using System.Reflection;`
`3`	`2`	`using JsonApiDotNetCore.Graph;`
`4`	`3`	`using JsonApiDotNetCore.Models;`
`5`	`4`	`using Xunit;`