Entity and Resource separation #344

roblankey · 2018-07-18T19:21:26Z

Closes #112

write tests that address the requirements outlined in the issue
fulfill the feature requirements
bump package version

A couple of specific notes RE: the implementation.

I didn't think it was necessary to add mapping support to the contextgraph, instead opting for a much simpler DI of automapper profiles.
To simplify relationship mapping and limit the amount of reflection necessary, there is an assumed convention of naming the mapped properties the same. Mapping between them is accomplished by use of automapper profiles

Finally, an additional note. Initially I did not want to include resource representations of the many-to-many entities that entity framework requires, but the additional complexity for mapping these out to relationships the context graph would know about does not seem to provide real value, since you can accomplish the same via a resource declaration.

Add new MappingResourceService that can be used to separate and map between Entity Framework objects and JSON-API data transfer objects Added new example project, ResourceEntitySeparationExample, showing how to configure an entity/resource split and corresponding tests.

…zation

…ying the internal name instead of only inferring it from the dto variable name, as that may not match the entity relationship name

…-many entity required by ef core

jaredcnance

Thanks for getting this started! It's a much needed feature and I can tell you've put a lot of work into it and really appreciate the help here. I still need some more time to review everything, but I'm interested in your thoughts on the comments I've made so far. I've discussed the design proposed by #112 with a few others but could definitely use some extra feedback on it. Nothing is set in stone, so feel free to dispute any of the points I've made.

One quick question too. You said:

assumed convention of naming the mapped properties the same

You're referring to your specific AutoMapper implementation in the example app? If not I'm not sure that this should be a framework requirement. I think mapping from differently named resource/entity property names should definitely be allowed.

jaredcnance · 2018-07-19T03:36:42Z

src/Examples/JsonApiDotNetCoreExample/Migrations/20180717134637_ResourceSeparation.cs

+
+namespace JsonApiDotNetCoreExample.Migrations
+{
+    public partial class ResourceSeparation : Migration


migrations shouldn't be necessary since we don't actually use them to build the db (we use the EnsureCreated api). I thought I had removed all the migrations earlier, but apparently not, so I completely understand the confusion. I've added a note to #290 to improve our docs around this.

JsonApiDotNetCore/src/Examples/JsonApiDotNetCoreExample/Startup.cs

Line 62 in 404231b

context.Database.EnsureCreated();

jaredcnance · 2018-07-19T03:38:49Z

src/Examples/JsonApiDotNetCoreExample/Models/Person.cs

@@ -21,7 +21,7 @@ public class Person : Identifiable, IHasMeta
        [HasMany("todo-collections")]
        public virtual List<TodoItemCollection> TodoItemCollections { get; set; }

-        [HasOne("unincludeable-item", Link.All, canInclude: false)]
+        [HasOne("unincludeable-item", documentLinks: Link.All, canInclude: false)]


jaredcnance · 2018-07-19T03:41:36Z

src/Examples/ResourceEntitySeparationExample/.gitignore

@@ -0,0 +1,234 @@
+## Ignore Visual Studio temporary files, build results, and


this shouldn't be necessary. it should be inherited from the root .gitignore

jaredcnance · 2018-07-19T03:42:12Z

src/Examples/ResourceEntitySeparationExample/Profiles/CourseProfile.cs

+                ;
+
+            CreateMap<CourseResource, CourseEntity>()
+                // .ForMember(e => e.Registrations, opt => opt.MapFrom(r => RegistrationsFromResource(r)))


☝️ forgotten comment line ?

jaredcnance · 2018-07-19T03:43:55Z

src/Examples/ResourceEntitySeparationExample/Profiles/StudentProfile.cs

+                    Number = e.Number,
+                    Title = e.Title,
+                    Description = e.Description
+                });


Why the manual mapping? Seems like if we're already using AutoMapper, this shouldn't be necessary.

This bit here is for mapping the underlying many to many 3 entities to a more logical 2 resource setup. This really just provides the direct link from Student to Courses without having to go through the CourseStudent artificial resource.

jaredcnance · 2018-07-19T03:58:20Z

src/JsonApiDotNetCore/Models/HasOneAttribute.cs

        : base(publicName, documentLinks, canInclude)
        {
-            _explicitIdentifiablePropertyName = withForiegnKey;


jaredcnance · 2018-07-19T04:04:12Z

src/JsonApiDotNetCore/Services/EntityResourceService.cs

@@ -133,7 +116,7 @@ private bool ShouldIncludeRelationships()
            var relationship = _jsonApiContext.ContextGraph
                .GetContextEntity(typeof(T))
                .Relationships
-                .FirstOrDefault(r => r.InternalRelationshipName == relationshipName);
+                .FirstOrDefault(r => r.PublicRelationshipName == relationshipName);


Awesome catch! Should actually use: r.Is(relationshipName)

JsonApiDotNetCore/src/JsonApiDotNetCore/Models/RelationshipAttribute.cs

Lines 74 to 75 in 404231b

public virtual bool Is(string publicRelationshipName)

=> string.Equals(publicRelationshipName, PublicRelationshipName, StringComparison.OrdinalIgnoreCase);

jaredcnance · 2018-07-19T04:13:24Z

src/JsonApiDotNetCore/Services/MappingResourceService.cs

+
+namespace JsonApiDotNetCore.Services
+{
+    public class MappingResourceService<TResource, TEntity> 


I would prefer to not duplicate the actual implementation details. My thought was that we would have a single, generalized implementation that would look like:

public ResourceService<TResource> : ResourceService<TResource, int>, IResourceService<TResource> { /* ... */ } public ResourceService<TResource, TId> : ResourceService<TResource, TResource, TId>, IResourceService<TResource, TId> { /* ... */ } public ResourceService<TResource, TEntity, TId> : IResourceService<TResource, TEntity, TId> { public ResourceService( IJsonApiContext jsonApiContext, IEntityRepository<TEntity, TId> entityRepository, ILoggerFactory loggerFactory) { // no mapper provided, TResource & TEntity must be the same type if(typeof(TResource) != typeof(TEntity)) throw new InvalidOperationException("..."); } public ResourceService( IJsonApiContext jsonApiContext, IEntityRepository<TEntity, TId> entityRepository, ILoggerFactory loggerFactory, IMapper mapper) { /* ... */ } }

Now obviously this would allow for runtime errors, so #326 could include checking to verify at startup.

jaredcnance · 2018-07-19T04:29:36Z

src/JsonApiDotNetCore/Services/MappingResourceService.cs

+          IJsonApiContext jsonApiContext,
+          IEntityRepository<TEntity> entityRepository,
+          ILoggerFactory loggerFactory,
+          IMapper mapper)


As already stated, we don't want a hard dependency on AutoMapper. As outlined in #112, it should take an internally defined interface such as a IResourceMap<TResource, TEntity> and not make any assumptions about a single general map instance. This also enables mapper agnostic startup validation as stated in the previous comment.

Example:

services.AddScoped<IResourceService<FooResource, FooEntity, int>>(); services.AddSingleton<IResourceMap<FooResource, FooEntity>>( new AutoMapperAdapter<FooResource, FooEntity>() ); // OR this could be bundled into a JsonApiDotNetCore.AutoMapper extension method // on IServiceCollection like: services.AddAutoMappedResource<FooResource, FooEntity>();

By doing this we can do a few things at startup:

For every resource, check to see if an IResourceService<TResource, TEntity, int> implementation exists and has been registered on the container

If the implementation exists, check to see if an IResourceMap<FooResource, FooEntity> exists and has been registered on the container

Also, the AutoMapperAdapter (or whatever it is called) can verify the mapping is actually defined.

If these checks fail, we can provide quick feedback that a configuration error has occurred:

A mapper has not been registered (TResource != TEntity && MapperIsRegistered() == false)

The AutoMapper.Mapper does not define a required mapping

maybe others ?

Ah. Okay, yeah.i misunderstood that bit. Makes sense.

jaredcnance · 2018-07-19T04:32:12Z

test/ResourceEntitySeparationExampleTests/.gitignore

@@ -0,0 +1,234 @@
+## Ignore Visual Studio temporary files, build results, and


shouldn't be necessary

jaredcnance · 2018-07-19T04:39:30Z

src/Examples/JsonApiDotNetCoreExample/Models/Resources/CourseStudentResource.cs

+    /// for that relationship, due to the need to make the underlying 
+    /// framework and mapping understand the explicit navigation entity, 
+    /// a mirroring DTO resource is also required.
+    /// </summary>


🤔 I think we can find a way to get around this...
Maybe not in v1, but it would be a pretty cool feature if we could allow applications to hide this implementation detail.

Definitely. This bit was really only needed for patching the many to many relationship due to the framework not immediately knowing anything about the entity and the genericprocessor not knowing what to do with it.

I really wanted to avoid creating this but thought the rest might be valuable enough to postpone this bit.

roblankey · 2018-07-19T07:10:57Z

Thanks for the feedback and catches. Next step is generalizing out the mapping resource.

…resourceservice

…it untyped for now to match how automapper works

…apper injection

roblankey · 2018-07-19T09:56:41Z

the last question i have here is around the 112 conversation with configuration checking. for the moment, i've left IResourceMapper untyped really only because that's how IMapper works and i wanted to be able to inject an AutoMapper configuration pretty directly on to it like in

public class AutoMapperAdapter : IResourceMapper
    {
        private readonly IMapper _mapper;

        public AutoMapperAdapter(IMapper mapper)
        {
            _mapper = mapper;
        }

        public TDestination Map<TDestination>(object source)
        {
            return _mapper.Map<TDestination>(source);
        }

        public TDestination Map<TSource, TDestination>(TSource source)
        {
            return _mapper.Map<TSource, TDestination>(source);
        }
    }

and Startup

public ConfigureServices(...)
{
    ....
    services.AddAutoMapper();
    services.AddScoped<IResourceMapper, AutoMapperAdapter>();
    ....
}

What are your thoughts on that? Ideas on how to bridge that gap from AutoMapper's overall approach to type specific IResourceMapper implementations?

jaredcnance · 2018-07-20T00:10:58Z

🤔 so, from a discovery/validation perspective IResourceMapper would just need to expose a ContainsMapping(Type in, Type out) method so that we can check if the mapping exists (this doesn't have to be done right now). This is arguably a better experience than having to lookup on the container to see if a particular type (e.g. IMapper<TIn, TOut>) has been registered.

Also, if you need type specific overrides, you can just create a different implementation or interface and depend on that instead. So, let's say I use AutoMapper for 99% of my mapping needs, but there is this one resource/entity pair where I need something else. I would achieve that by doing something like:

// CustomMapper.cs
public class FooMapper : IResourceMapper { /* ... */ }

// Startup.cs
services.AddSingleton(new FooMapper());

// FooService.cs
public class FooService : DefaultResourceService {
  public FooService(
    IJsonApiContext jsonApiContext,
    IEntityRepository<Bar> entityRepository,
    ILoggerFactory loggerFactory,
    FooMapper mapper // <----- need to inject concrete type (or custom abstraction)
  ) : base(jsonApiContext, entityRepository,  loggerFactory, mapper) { /* ... */ }
}

So, I think that should be fine. I'll need to think about other scenarios as well.

jaredcnance · 2018-07-20T00:13:16Z

src/JsonApiDotNetCore/Services/DefaultResourceService.cs

+
+namespace JsonApiDotNetCore.Services
+{
+    public class DefaultResourceService<TResource> : DefaultResourceService<TResource, int>, 


I'd prefer not to rename this class just yet. This would result in a wide-spread breaking change (i think we should be able to roll out the first version of this in a minor version). I'd prefer to hold off until #317. I can add this as a suggestion to the open issue.

Good deal. Undoing the rename.

jaredcnance · 2018-07-23T05:41:52Z

I've retargeted to the develop branch so I can publish a release to our MyGet feed (https://www.myget.org/F/research-institute/api/v3/index.json). I'll be integrating this into a large application tomorrow and if everything goes smoothly, I'll merge into master early this week. Thanks again for all your hard work! 🍻

roblankey · 2018-07-23T12:02:35Z

Awesome! Thanks for all the feedback along the way!

roblankey added 11 commits July 11, 2018 13:59

Add TestFixture to account for otherwise repeated AutoMapper initiali…

afee0fe

…zation

add additional parameter to relationshipattribute to allow for specif…

b6df7c6

…ying the internal name instead of only inferring it from the dto variable name, as that may not match the entity relationship name

update model for resource separation example in base example src

a10c528

add department for a nice hasone and other model code

0146709

update resource entity separation tests with relationships

8f877cd

add fixture to noentity tests

1b8404e

use public name instead of reflected property name

bed4e9d

revert the exposing of internal name

a208460

wrapping up basic testing and functionality

664d35e

add coursestudent resource corresponding to the coursestudent many-to…

7c104c2

…-many entity required by ef core

jaredcnance suggested changes Jul 19, 2018

View reviewed changes

jaredcnance reviewed Jul 19, 2018

View reviewed changes

roblankey added 4 commits July 19, 2018 05:00

combined entityresourceservice and mapperresourceservice into default…

f438d5c

…resourceservice

clean up

6e9776b

remove automapper and create resource mapper generic interface. kept …

49127e5

…it untyped for now to match how automapper works

added automapperadapter implementation of iresourcemapper using autom…

5b18304

…apper injection

jaredcnance reviewed Jul 20, 2018

View reviewed changes

revert rename of entity resource service

dd0cae4

jaredcnance changed the base branch from master to develop July 23, 2018 05:39

jaredcnance approved these changes Jul 23, 2018

View reviewed changes

jaredcnance merged commit 536ea4e into json-api-dotnet:develop Jul 23, 2018

roblankey deleted the feat/#112 branch July 23, 2018 12:04

This was referenced Jul 23, 2018

Deeply Nested Relationship Inclusion #39

Closed

Entity Resource Separation (2.4.0-beta3) #352

Merged

jaredcnance mentioned this pull request Sep 9, 2018

Attribute Naming Conventions #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity and Resource separation #344

Entity and Resource separation #344

roblankey commented Jul 18, 2018 •

edited by jaredcnance

Loading

jaredcnance left a comment •

edited

Loading

jaredcnance Jul 19, 2018

roblankey Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

roblankey Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

roblankey Jul 19, 2018

jaredcnance Jul 19, 2018

jaredcnance Jul 19, 2018

roblankey Jul 19, 2018

roblankey commented Jul 19, 2018

roblankey commented Jul 19, 2018 •

edited

Loading

jaredcnance commented Jul 20, 2018 •

edited

Loading

jaredcnance Jul 20, 2018 •

edited

Loading

roblankey Jul 20, 2018

jaredcnance commented Jul 23, 2018 •

edited

Loading

roblankey commented Jul 23, 2018

		@@ -0,0 +1,234 @@
		## Ignore Visual Studio temporary files, build results, and

	public virtual bool Is(string publicRelationshipName)
	=> string.Equals(publicRelationshipName, PublicRelationshipName, StringComparison.OrdinalIgnoreCase);

Entity and Resource separation #344

Entity and Resource separation #344

Conversation

roblankey commented Jul 18, 2018 • edited by jaredcnance Loading

jaredcnance left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblankey commented Jul 19, 2018

roblankey commented Jul 19, 2018 • edited Loading

jaredcnance commented Jul 20, 2018 • edited Loading

jaredcnance Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredcnance commented Jul 23, 2018 • edited Loading

roblankey commented Jul 23, 2018

roblankey commented Jul 18, 2018 •

edited by jaredcnance

Loading

jaredcnance left a comment •

edited

Loading

roblankey commented Jul 19, 2018 •

edited

Loading

jaredcnance commented Jul 20, 2018 •

edited

Loading

jaredcnance Jul 20, 2018 •

edited

Loading

jaredcnance commented Jul 23, 2018 •

edited

Loading