OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
853 stars 476 forks source link

EF Core Many to Many join tables #1924

Open TehWardy opened 4 years ago

TehWardy commented 4 years ago

So in short ... How do we expose many to many join table entity sets in odata for full crud usage just like any other entity?

And here's the longer version of the question ...

Now that EF Core is forcing exposure of the "Many to Many" join tables as entities in our models is there some advice for exposing controllers so that we can CRUD on this new entity type with a complex / multi column key directly?

Example ...

EntityTypes

class Role { 
    public Guid Id { get; set; }
     ... 
}
class User { 
    public Guid Id { get; set; }
     ... 
}
class RoleUser 
{
    // keys
     public Guid RoleId { get; set; }
     public Guid UserId { get; set; }

     // Relationships
     public Virtual Role Role { get; set; }
     public Virtual User User { get; set; }
}

Then in the Odata Model construction ..

Builder.EntitySet<Role>("Roles");
Builder.EntitySet<User>("Users");
Builder.EntitySet<RoleUser>("RoleUsers");

... Having tried this I can't figure out quite how to make this work as I don't know how to first define and reference the "primary keys" from the join table since the key is more than one value.

I know there are other approaches (e.g. coming from one side or the other of the join and calling in to those endpoints instead but this then represents an inconsistency in the graph where some parts are to be treated as entities and others as "implied entities" even though they actually have to be declared as proper entities.

KanishManuja-MS commented 4 years ago

@TehWardy You can use [key] attribute on the model to annotate them as part of the key in a composite key.

TehWardy commented 4 years ago

@kanishmanuja-MS I think you've misunderstood the problem ... Odata routes require THE key ... How does a pair of values go in to the single value expected in say a delete call for a join table row?

Danielku15 commented 4 years ago

@TehWardy In OData you can specify multiple keys either as path segments or via the explicit key names in the URL: http://docs.oasis-open.org/odata/odata/v4.01/csprd05/part2-url-conventions/odata-v4.01-csprd05-part2-url-conventions.html#sec_URLsforRelatedEntitieswithReferentia

class RoleUsersController
{
    // http://localhost/odata/RoleUsers(RoleId=1,UserId=2)
    [EnableQuery]
    public virtual SingleResult<RoleUser> Get(long keyRoleId, long keyUserId)
    {
        return SingleResult.Create(...);
    }
}

The only special thing of the OData WebApi library is that you must prefix the method parameters with key. Some docs related to this are here: https://docs.microsoft.com/en-us/odata/webapi/key-value-binding

TehWardy commented 4 years ago

omg .... that's awesome @Danielku15 Defintely something to try!!! Does this work for all of CRUD using the normal conventions?

Danielku15 commented 4 years ago

Does this work for all of CRUD using the normal conventions?

Yes, we also use this key-handling for all our operations on Many-to-Many entities.

TehWardy commented 4 years ago

Confirmed as working ... thanks guys (sorry about the delay, busy in other areas)

TehWardy commented 4 years ago

Ok further testing is showing this not working as intended. Can't seem to figure out what needs to be where.

TehWardy commented 4 years ago

The issue seems to be around the key names ... Using the types above I would do (as per the documentation referenced (also above)

Builder.EntityType<RoleUser>().HasKey(i => new { i.UserId, i.RoleId });

To map to controller method defined as ....

public virtual async Task<IActionResult> Delete([FromODataUri]string keyUserId, [FromODataUri]Guid keyRoleId)
{
    await Service.DeleteAsync(keyUserId, keyRoleId);
    return Ok();
}

The parameter names on the dynamically defined key object MUST match the original object and thus the controller action must also have those property names.

This removes the possibility of having a base controller (generic) that I can pass the key types in to with a "standard" left key and right key parameter on them like this ...

        [HttpDelete]
        public virtual async Task<IActionResult> Delete([FromODataUri]TLeft keyLeft, [FromODataUri]TRight keyRight)
        {
            await Service.DeleteAsync(keyLeft, keyRight);
            return Ok();
        }

With an updated key def in the model as ...

Builder.EntityType<RoleUser>().HasKey(i => new { Left = i.UserId, Right = i.RoleId });

I would expect this to still work but with a URL like ... http://localhost/odata/RoleUsers(Left=1,Right=2) ... since i'm surely telling the framework here what I want it to map in terms of param values to the controller method.

My point if anything therefore is ... why ask me to define a mapping for the key if it's ignored anyway?

Danielku15 commented 4 years ago

This is indeed an interesting finding. I couldn't see on a quick test where this issue is coming from. I guess it is something on the OData URL parser which does not support named properties on composite keys. From a model strictness this makes sense, you are still on a entity set of RoleUser objects, they do not have Left or Right properties. Aliasing on the Model Builder is likely an unsupported feature/concept.

In our case we override in all subclasses the base implementation and adjust the parameter names:

class BaseController : ODataController
{
    public virtual async Task<IActionResult> Delete(
        int keyLeft,
        int keyRight,
        CancellationToken cancellationToken)
    {
        await Service.DeleteAsync(keyLeft, keyRight, cancellationToken);
        return NoContent();
    }
}
public class RoleUsersController : BaseController 
{
    public override Task<IActionResult> Delete(
        [FromODataUri] int keyUserId,
        [FromODataUri] int keyRoleId,
        CancellationToken cancellationToken)
    {
        return base.Delete(keyUserId, keyRoleId, cancellationToken);
    }    
}

For us this mechanism adresses 2 aspects:

  1. The delete logic is properly shared and not duplicated
  2. From an API design perspective it is clear to clients what keys are there for what. Having Left and Right needs additional manual documentation that Left is now the UserId and not the RoleId.

You could write a Roslyn Analyzer to ensure that nobody forgets this override.

TehWardy commented 4 years ago

Exactly my situation and how I came across this. I have something like ...

public abstract class EntityController<T> : ODataController
{
         public EntityController(IService<T> service) {  }
}

public class RoleUsersController : EntityController<RoleUsers>
{

}

Essentially the controller deals with the http layer stuff and the passed in (by DI) service layer object handles the business logic as you might expect from a standard N-Tier modelled architecture.

Under .Net Framework 4.x I didn't even need to define the concrete implementation in many cases since the base implementation could handle everything, I was using ninject for my DI stuff though and resolving the exact type when the request came in.

Under .Net Core i switched to using the built in IoC / DI framework which didn't appear to be able (at least initially anyway) ... resolving a type from a request with a custom factory / Func<Request, object> definition.

So I resolved everything ahead of time by simply defining what was essentially a bunch of empty controller classes that inherited from my base. This now goes a step further and forces some code duplication on me unless there's a way round this.

My worry is that Microsoft's resource management internally lately has seen stuff like this left behind because they would rather work on a new version of .Net Core than fix the stuff that sits on top of it.

I'm already dreading having to move to .Net Core 3, the core 2 upgrade was brutal!

It would be nice to get at least some reassurance from someone at Microsoft.