De-Couple DAL from Entity Framework #89

jaredcnance · 2017-04-08T03:07:20Z

Closes #84

As written, this will break all controllers inheriting from JsonApiController. You would need to update your controller constructor parameters. One approach (as lightly discussed in #84) would be to provide a deprecated constructor method and possibly a LegacyJsonApiController that would contain the current logic. JsonApiController would then pass calls into the legacy controller if it were constructed using the IEntityRepository constructor instead of the IResourceService param. This would be removed at a later date. I would like to hear any thoughts on this and whether it is necessary, or if we should accept the breaking change. Also, does this constitute a major version change?

Please feel free to tear this apart or raise any concerns/suggestions/complaints

TODO

Replace IEntityRepository with IResourceService in JsonApiController
Determine how best to maintain backwards compatibility, if at all
Acceptance tests to validate extensibility at the service layer
Custom ContextGraphBuilder API
Service Collection Extension (services.AddJsonApi()) without the DbContext generic
Complete documentation
Bump project version
Add ResourceAttribute to allow custom type names for DbSets

this is reprensentative of the kind of breaking change this introduces

also affects the service collection extensions

Since AddJsonApiInternals<T> was previously exposed, it needs to remain. Also, this is where the DbContext service should be added to the service collection since users could call this instead of UseJsonApi. It would be unreasonable to expect users to inject their dbContexts as the DbContext base type

as far as I can tell, .Net's native DI will check all pathways for resolution even if they shouldn't be resolved. Specifically, the Internal.Generics classes require DbContext to be injected. However, they would never be resolved if the app does not actually use EF and implements their own IResourceService

jaredcnance · 2017-04-13T03:20:35Z

src/NoEntityFrameworkExample/Startup.cs

@@ -34,7 +34,7 @@ public void ConfigureServices(IServiceCollection services)
            services.AddJsonApi(options => {
                options.Namespace = "api/v1";
                options.BuildContextGraph((builder) => {
-                    builder.AddResource<TodoItem>("todo-items");
+                    builder.AddResource<TodoItem>("TodoItems");


Currently, this simulates the DbSet name. I don't really like the way this works, especially since the format provided is not even the public format. But, it will currently break in the de-serialization. Need to do some more work on this.

For the DbSets, the name is dasherized and there is no opt-out. Why are you not dasherizing here as well?

jaredcnance · 2017-04-13T03:22:18Z

src/JsonApiDotNetCore/Extensions/IServiceCollectionExtensions.cs

+            {
+                services.AddScoped<DbContext>();
+                services.AddSingleton<DbContextOptions>(new DbContextOptionsBuilder().Options);
+            }


I really really don't like this, but as stated in the commit message:

as far as I can tell, .Net's native DI will check all pathways for resolution even if they shouldn't be resolved. Specifically, the Internal.Generics classes require DbContext to be injected. However, they would never be resolved if the app does not actually use EF and implements their own IResourceService

Would love to see if anyone has any extra insight into this.

Tracking: aspnet/DependencyInjection#514

…once there is really no reason to store the proper cased entity name this also lays the ground work so that later users can specify different cases for entity type names

jaredcnance · 2017-04-14T02:25:01Z

@JanMattner @jfhs this is mostly ready. I'll spend some time this weekend adding extra tests around it. Wanted to get your opinions on it so far. I've updated the Readme, and there is a sample project that does not use EF.

JanMattner · 2017-04-18T12:55:54Z

Thanks, great work! However, what first comes to my mind: If I want to use the IResourceService as an interface to some HTTP service, I won't be able to fulfill Task<IEnumerable<T>> GetAsync(); simply because of the huge amount of items.

I need at least some kind of paging mechanism with pageSize and pageNumber.

jaredcnance · 2017-04-18T13:02:36Z

@JanMattner you should be able to implement paging however you like. You can get the IJsonApiContext from your service constructor. From that, you can get the page query like so:

var pageNumber = _jsonApiContext.PageManager.CurrentPage;
var route = $"{EXTERNAL_API_HOST}/items?page={pageNumber}";
var request = new HttpRequestMessage(new HttpMethod("GET"), route);
var response = await client.SendAsync(request);

Is this what you're talking about?

JanMattner · 2017-04-18T13:18:34Z

My bad! Yes, that was what I was looking for. I'll have a closer look at the rest later and give feedback.

this should be cleaned up by #91

failing with duplicate key value violates unique constraint "pg_database_datname_index"

JanMattner

I think you accidentally added the "bin" and "obj" folder of the new "NoEntityFrameworkExample" project.

I think it's great that you clean up the magic dasherization. However, currently I cannot capture the whole impact of that and how the default behavior is changed by removing the dasherization. In any case the easiest way would be to just introduce a new major version and leave it to the users to figure out the impact on their existing applications. The possible breaking change about the controller constructor encourages that solution.

However, we should then at least ensure that all the magic dasherization is addressed.

JanMattner · 2017-04-19T05:45:51Z

src/JsonApiDotNetCore/Builders/ContextGraphBuilder.cs

+            var entityType = typeof(TResource);
+            Entities.Add(new ContextEntity
+            {
+                EntityName = pluralizedTypeName,


This name is not dasherized. Why?

See comment below. I'd like to avoid mutating any manual input so we can avoid inconsistencies as described in #93

JanMattner · 2017-04-19T05:47:50Z

src/JsonApiDotNetCore/Builders/ContextGraphBuilder.cs

+                    var entityType = dbSetType.GetGenericArguments()[0];
+                    entities.Add(new ContextEntity
+                    {
+                        EntityName = property.Name.Dasherize(),


Either there is an attribute for the DbSet to define a custom non-dasherized name or the custom resource name above (when directly adding a resource instead of using the DbContext) must be dasherized as well.

I'm inclined to agree with allowing for an optional attribute per DbSet that would allow for a custom name.

When defining names manually (either through the ContextGraphBuilder or using a ResourceAttribute), whatever input is provided will be accepted as the resource name

If no [Resource("my-resource")] is specified on the DbSet, then it will be dasherized by default

That sounds good! And yes you are right, any manual explicit input should not be altered.

JanMattner · 2017-04-19T06:05:10Z

src/JsonApiDotNetCore/Configuration/JsonApiOptions.cs

+        public void BuildContextGraph(Action<IContextGraphBuilder> builder)
+        {
+            if(builder == null)
+                throw new ArgumentException("Cannot build non-EF context graph without a IContextGraphBuilder action", nameof(builder));


typo: "without an IContextGraphBuilder"

there should be very few places .Dasherize is actually used. most of the time we should stick to using either the Internal or Public attr/relationship names

jaredcnance · 2017-04-19T21:37:05Z

I think this is ready to go (other than the goofy DI stuff mentioned above -- but i dont think I want that to hold up a pre-release).

If there are no other comments, I'll merge this tomorrow afternoon. Thanks @JanMattner for your help!

…d custom route controllers and links

JanMattner · 2017-04-20T20:39:22Z

I have found still a buggy usage of the dasherization when creating links. I have added a couple of tests that currently fail.

jaredcnance · 2017-04-21T01:39:14Z

@JanMattner thanks for the tests. So, the main issue revolves around this line (which can be improved by removing the .Dasherize() and assume the routes will be in the same format as the entity name).

I made the assumption that APIs should use semantic endpoints (maybe the wrong name for it?). That is, a request to "/todoitems" will return "todoitems" and a request to "/todo-items" should return "todo-items". This breaks the link builder as it assumes, the base route structure will end with the resource name. I'd be curious to hear if you currently need this support and what kind of constraints create this kind of requirement.

I've added a fix that removes hyphenation from being performed in the link builder.

JanMattner · 2017-04-21T16:54:07Z

Yes, you are right, in general a server implementation should follow the guidelines and provide semantic endpoints. However, the user is not bound to that and the current implementation is just another hidden dasherization. Furthermore, if I interpret the code correctly, all path segments will be dasherized. But what if I want to host (even my proper semantic) endpoint at:
https://host.com/my/customPath/todo-items

It would dasherize the whole path. I just think that this is not really intuitive if done hidden / implicitly.

jaredcnance · 2017-04-21T18:08:30Z

I agree that there should not be any dasherization in the LinkBuilder. My last commit should have fixed this.

The way the logic is intended to work is to identify the "base route/namespace" from the current route and build links from there:

                 resource-name |--------|
https://host.com/my/customPath/todo-items
|-----base-route-------------|

You're right that a user can define routes however they want and nothing prevents that. If someone defines a controller as:

[Route("custom/route/blueberries")]
public class TodoItemsController : JsonApiController<TodoItem>
{
   // ...
}

There is no great way (that I am aware of) to determine "TodoItems" links should actually point to "Blueberries". In this scenario, the link builder assumes /blueberries is actually part of the base route and would build links like:

https://host.com/custom/route/blueberries/todo-items

I'm unsure how to proceed with this. Perhaps the link builder should throw if the resource cannot be found in the path? But I'm not convinced that we should support routing like this.

jaredcnance · 2017-04-23T02:23:21Z

There is one other issue that we need to address and that's the DasherizedRoutingConvention that assumes all JsonApiControllers should have dasherized names.

var template = $"{_namespace}/{controller.ControllerName.Dasherize()}";

I propose a few options:

introduce a flag on JsonApiOptions that would allow users to disable the DasherizedRoutingConvention globally
provide attributes that could decorate the controller to disable the routing convention
check if the user has supplied a specific route through the RouteAttribute and if so, then disable the convention.

However, I think this is more about routing choices and would like to close this PR and punt routing into another. Although, to be clear, I'd like to see both of these go out in the 2.0.0 release.

these tests are not currently setup correctly. they assume that a resource can be routed based on a different name (todo-items-test -> todo-items). this is not currently supported and is out of scope for this PR. I am open to discussion, but don't believe this is going to be on the roadmap. Other changes (todoitems -> todo-items) is for the dame reason. TodoItems have been mapped to todo-items in this example so the tests would fail.

jaredcnance · 2017-04-25T15:08:47Z

@JanMattner i'm going to merge this into develop and move any additional routing concerns into a new PR

jaredcnance added 5 commits April 7, 2017 21:50

feat(services): define IResourceService

4ed46c8

refactor(controller): move bulk of logic into the resource service

fa9364f

feat(service-collection-ext): add new service

b62227d

chore(example-controllers): update for new JsonApiController ctor

6d6c3e4

this is reprensentative of the kind of breaking change this introduces

docs(readme): stub out documentation updates

e17cad2

jaredcnance added the feedback wanted label Apr 8, 2017

jaredcnance requested a review from JanMattner April 8, 2017 12:44

jaredcnance added 6 commits April 12, 2017 04:56

feat(context-graph): first pass at refactoring the graph API

550e00a

also affects the service collection extensions

fix(IContextGraphBuilder): expose the AddDbContext method

ba4719d

example: add "no entity framework" example

8457b22

test(extensibility): add initial test around fetching non-ef data

99e8b4d

jaredcnance commented Apr 13, 2017

View reviewed changes

jaredcnance added 6 commits April 12, 2017 22:25

Merge remote-tracking branch 'origin/develop' into feat/decouple-dal

194d31d

example(no-ef): rename models, controllers seem to cause conflict

09cc32a

refactor(context-graph): the stored entity name should be dasherized …

7c20d77

…once there is really no reason to store the proper cased entity name this also lays the ground work so that later users can specify different cases for entity type names

test(no-ef): validate the response body

2fd9eb3

docs(readme): document custom data implementation

94e5dee

docs(readme): document custom context mapping

e6ba54f

jaredcnance requested a review from jfhs April 14, 2017 02:23

jaredcnance mentioned this pull request Apr 14, 2017

Inconsistency for dasherized attributes #93

Closed

jaredcnance added the breaking-change label Apr 14, 2017

docs(readme): document new controller constructor

0264ea8

feat(example): use Dapper and Npgsql in NoEF example

84cf741

jaredcnance mentioned this pull request Apr 18, 2017

Cleanup Test Project #91

Closed

6 tasks

jaredcnance added 4 commits April 18, 2017 14:45

test(acceptance/no-ef): ensure the database is created

93edba5

this should be cleaned up by #91

test(acceptance/no-ef): ensure database is migrated

2c164fa

fix(example/startups): try using EnsureCreated to fix travis-ci build

edc4626

failing with duplicate key value violates unique constraint "pg_database_datname_index"

refactor(tests): split test projects

7c14d49

JanMattner suggested changes Apr 19, 2017

View reviewed changes

jaredcnance added 8 commits April 19, 2017 07:47

fix(ci): restore new test project

36b55d8

chore(example): remove build files from source control

62183f3

chore(csproj): bump package major version

629169f

feat(context-graph-builder): intro resource attr

fd5e04a

refactor(*): further reduction of Dasherize usage

b0146c4

there should be very few places .Dasherize is actually used. most of the time we should stick to using either the Internal or Public attr/relationship names

docs(readme): show ResourceAttribute usage

75ae1b5

fix(de-serializer): do not dasherize a public name

3d95c01

test(no-ef): additional tests / examples

7cff6d0

jaredcnance changed the title ~~[WIP] De-Couple DAL from Entity Framework~~ De-Couple DAL from Entity Framework Apr 19, 2017

fix(example): missing attribute

517367b

jaredcnance force-pushed the feat/decouple-dal branch from 400a088 to 517367b Compare April 20, 2017 12:48

test(extensibility): add tests for dasherized routes for inherited an…

aec7e1c

…d custom route controllers and links

fix(link-builder): do not dasherize routes by default

4480e6b

jaredcnance added 2 commits April 25, 2017 09:55

chore(csproj): add package information

8d134f3

jaredcnance merged commit d4cb99d into develop Apr 25, 2017

jaredcnance deleted the feat/decouple-dal branch April 25, 2017 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-Couple DAL from Entity Framework #89

De-Couple DAL from Entity Framework #89

jaredcnance commented Apr 8, 2017 •

edited

Loading

jaredcnance Apr 13, 2017

JanMattner Apr 19, 2017

jaredcnance Apr 13, 2017 •

edited

Loading

jaredcnance commented Apr 14, 2017

JanMattner commented Apr 18, 2017 •

edited

Loading

jaredcnance commented Apr 18, 2017

JanMattner commented Apr 18, 2017

JanMattner left a comment

JanMattner Apr 19, 2017

jaredcnance Apr 19, 2017

JanMattner Apr 19, 2017

jaredcnance Apr 19, 2017

JanMattner Apr 19, 2017

JanMattner Apr 19, 2017

jaredcnance commented Apr 19, 2017 •

edited

Loading

JanMattner commented Apr 20, 2017

jaredcnance commented Apr 21, 2017 •

edited

Loading

JanMattner commented Apr 21, 2017

jaredcnance commented Apr 21, 2017 •

edited

Loading

jaredcnance commented Apr 23, 2017

jaredcnance commented Apr 25, 2017

De-Couple DAL from Entity Framework #89

De-Couple DAL from Entity Framework #89

Conversation

jaredcnance commented Apr 8, 2017 • edited Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredcnance Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

jaredcnance commented Apr 14, 2017

JanMattner commented Apr 18, 2017 • edited Loading

jaredcnance commented Apr 18, 2017

JanMattner commented Apr 18, 2017

JanMattner left a 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

jaredcnance commented Apr 19, 2017 • edited Loading

JanMattner commented Apr 20, 2017

jaredcnance commented Apr 21, 2017 • edited Loading

JanMattner commented Apr 21, 2017

jaredcnance commented Apr 21, 2017 • edited Loading

jaredcnance commented Apr 23, 2017

jaredcnance commented Apr 25, 2017

jaredcnance commented Apr 8, 2017 •

edited

Loading

jaredcnance Apr 13, 2017 •

edited

Loading

JanMattner commented Apr 18, 2017 •

edited

Loading

jaredcnance commented Apr 19, 2017 •

edited

Loading

jaredcnance commented Apr 21, 2017 •

edited

Loading

jaredcnance commented Apr 21, 2017 •

edited

Loading