v2.0.0 #94
v2.0.0 #94
Conversation
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
…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
this should be cleaned up by #91
failing with duplicate key value violates unique constraint "pg_database_datname_index"
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
…d custom route controllers and links
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.
De-Couple DAL from Entity Framework
Use TypeHelper.ConvertType to keep consistency.
Fix use spaces instead of tabs.
Allow PATCH objects with nullable attribute (e.g. DateTime?)
|
I'm going to let this sit for another day or two and then release. As of right now I have no additional changes planned. |
| if (IsDasherizedJsonApiController(controller)) | ||
| template = $"{_namespace}/{controller.ControllerName.Dasherize()}"; | ||
| else | ||
| template = GetTemplate(controller); |
There was a problem hiding this comment.
Why should anything be changed if the dasherized routing convention does not apply? Why not just leave it as is? E.g. imagine a controller with several stacked routes:
[Route("/api/v1/MyController")]
[Route("/api/v2/MyController")]
public class MyController(...) { ... }This is a valid use case: I want the same controller to be hosted for various routes. (not even restricted to API controllers, this also applies to static content or MVC controllers for documentation, login, ...)
Currently in this case, line 47 var routeAttr = type.GetCustomAttribute<RouteAttribute>(); throws an exception that multiple attributes of the same type are found. Why not just leaving everything as is if the dasherized convention is disabled or the controller base type does not match? Of course then the user is responsible to actually define the routing and this is not done by the dasherized routing convention. Actually, if you think about it: even if you explicitly disable the dasherized routing convention, it still does apply a routing convention i.e. just the name of the controller. As a developer, I don't expect that component to do anything if I disable it.
There was a problem hiding this comment.
Hey thanks for bringing this to my attention and you're absolutely right. I hadn't considered multiple route attributes which is totally valid.
The ideal scenario would be to not apply the convention at all for a particular controller. But, I'm not sure if this is possible (I'll be digging deeper this week). The line you commented on is more of a hack to try and determine what the intended route should be. In my limited experimentation, the simple fact that a convention is added seems to alter the behavior. Even if I detect that the convention should not be applied and don't alter the AttributeRouteModel it still doesn't seem to recognize attribute defined routes.
I'll explore this further this week and hopefully come up with a better solution. I would expect this PR to be on hold till next weekend.
There was a problem hiding this comment.
disregard ^^ i had bad tests.
opening PR to fix it now
Fix/routing convention
currently, I do not have enough time to keep the video up to date
i do not believe there is enough added value to continue maintaining the generators
Closes #84
Closes #93
Closes #95