Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EF Pain Points / Feature Discussion #20249

Closed
jwcarroll opened this issue Mar 11, 2020 · 4 comments
Closed

EF Pain Points / Feature Discussion #20249

jwcarroll opened this issue Mar 11, 2020 · 4 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Milestone

Comments

@jwcarroll
Copy link

This is an extension of a response I had to a question posed on Twitter by @JeremyLikness about pain points with data access in dotnet.

My experience has largely been with EF, so I provided some feedback in a thread. It was then suggested by @ajcvickers that I open an issue to further the discussion.

So hold onto your seats, this is long.

This is a wishlist of things I wish EF had that would make life a lot better

Unit Testing

Testing DbContext sucks. This is more about mocking IQueryable and DbSet than anything else, but it still hurts.

Throw in async / await and the ceremony just to set something up is almost unbearable.

I always end up with some pretty nasty test harness code using custom helpers like this:

protected Mock<DbSet<T>> ConfigureDataForContext<T>(Mock<ApplicationDbContext> mockContext, IList<T> data, Expression<Func<ApplicationDbContext, DbSet<T>>> dbSetSelector, Action<T> onAddCallback = null) where T : class
{
   var dataSet = data.AsQueryable();

   var mockSet = new Mock<DbSet<T>>();
   mockSet.As<IDbAsyncEnumerable<T>>()
       .Setup(m => m.GetAsyncEnumerator())
       .Returns(() => new TestDbAsyncEnumerator<T>(dataSet.GetEnumerator()));

  //... more nasty setup code

   mockSet.As<IQueryable<T>>().Setup(m => m.Expression).Returns(dataSet.Expression);
   mockSet.As<IQueryable<T>>().Setup(m => m.ElementType).Returns(dataSet.ElementType);
   mockSet.As<IQueryable<T>>().Setup(m => m.GetEnumerator()).Returns(() => dataSet.GetEnumerator());

   mockContext.Setup(dbSetSelector).Returns(mockSet.Object);

   return mockSet;
}

I can use helpers like that on a mock context that will eventually allow me to mock out specific tables with collections.

It certainly isn't perfect, but it is way better than some of the patterns that I have seen implemented in the past. Mainly, abstracting DbContext away. I already like the abstraction layer that DbContext provides, I just want an easier way of testing.

I also realize I can use in-memory SQL Lite for this, but that only solves the issue of having to mock out the provider. I still have to basically seed a bunch of data just to get a unit test.

Ideally there was a helper provider just for testing. Something that would focus on queries instead of the table sets themselves.

It would honestly be unbelievable if there was an easy way to say "When you get a query like X, return Y"

DbContext scoping

I honestly can't do a better explanation of this than Mehdi El Gueddari's excellent blog post from 2014.

It outlines the problem, and his solution is amazing.

Having this natively would be amazing.

Domain Modeling

I probably the most ignorant on this topic because I have never spent a ton of time trying to make this really work with EF. It has just never really felt natural. You can add functionality sure, but the abstractions always end up leaking out somewhere.

Basically, the only way it seems you can really make this work well is if your domain models have some knowledge of EF, and even then the issue with DbContext scoping can cause all kinds of issues.

Usually everyone just punts and ends up with an anemic domain model. DTO's and AutoMapper everywhere.

I feel like @jbogard is probably a great person to talk about what would be the most beneficial with regards to making this really seamless.

Migrations

I think I have tried every code based migration tool out there, including EF migrations and have always found them frustrating. When it works, it is nice, but being on a team that moves fast with overlapping updates ALWAYS ends in pain.

The greatest success I have had was actually a hand rolled migrator that just ran SQL scripts in order based on a unix timestamp, and saved what was run in the database. It wasn't fancy, but the friction went to near zero. If there were problems, they were easy to address because it was all just SQL scripts.

Something missing from a script? Update the SQL
Teammate push an update before you? Bump your file timestamp

Issues were easily caught on DEV/Staging, and then we just fixed the SQL scripts.

If EF had a simple migrator built in that did that, it would be awesome.

Row / Field Level Permissions

This one is huge to me because it's such a common paradigm. Data entitlements due to security is something every single application I have ever worked on needs, but there is no first class support for this in EF.

I'll use a concrete example to illustrate the point. For instance if I had a set of Orders

public DbSet<Orders> Orders { get; set; }

I want customers to be able to access only their own orders, but a manager might be able to see all orders for their department, or their entire organization.

Normally you end up doing this directly in code, by modifying the query based on the user context.

var query = context.Orders.Select();

if(isCustomer(securityContext)){
  query = query.Where(o => o.UserId == securityContext.UserId);
}

return await query
   .AsNoTracking()
   .ToListAsync();

To make this easier, I usually would refactor that code into an extension method so you end up with something like this:

return await context.Orders
   .WithSecurity(securityContext)
   .AsNoTracking()
   .ToListAsync();

The problem is that this is potentially easy to forget, or leave off, which has MASSIVE ramifications if left uncaught.

What I want, is a declarative way to specify security constraints at the model level, and have hooks for implementing what that looks like for a query.

I'd like to be able to just declare my security intent like this:

[RowSecurity(typeof(UserSecurityContext))]
public class Order {
  //...
}

//or

public class Order: IRowSecurity<UserSecurityContext> {
  //...
}

This would have two effects:

  1. Force me to pass in UserSecurityContext when making a query that included Orders (or maybe any query)

_context.WithSecurity(securityContext).Orders.Select();

  1. Force me to implement a method that would recieve the pending query, and UserSecurityContext before execution so I could modify it accordingly.
public IQueryable<Order> HandleSecurity(IQueryable<Order> query, UserSecurityContext context){
  //...
}
  1. Force me to implement some kind of check upon mutation to see if the security check passes, and throw a canonical exception if it fails.
public bool CanMutate(Order order, UserSecurityContext context){
  //...
}

Field level security could really make certain scenarios (like PII in healthcare) a lot more manageable.

An evaluation check could result in different strategies like Allow, Throw, Redact. Where field level checks might just result in the field being omitted, or redacted from the results. Attempts to mutate could result in cononical exceptions that could be handled by the calling code.

You could achieve something like this today by doing fancy reflector magic, and overriding SaveChanges and friends, or using an interceptor, but they are very low level and it certainly isn't ideal.

Conclusion

So that ended up being basically a blog post wish list, but these are the issues that have always caused the most friction for me.

Cheers,
Josh

@ajcvickers
Copy link
Member

@jwcarroll Thanks for filing this. It's been a bit crazy today, but we'll certainly get back to you soon.

@ajcvickers
Copy link
Member

@jwcarroll Thanks again. Some comments below.

Unit testing

Mocking IQueryable is very hard to do, especially once async is involved. This isn't something that is going to change--it's inherent to IQueryable. So the best we can do here is direct away from using it and provide guidance on other ways to achieve the goals.

DbContext is also not easy to mock in general because a lot of its original design is around being able to new it up and then use services of EF. Attempting to mock those services in a meaningful way is hard.

This is why going into EF Core we said not to mock the context or IQueryable, but instead to:

  • Define a more specific interface (could be a repository but don't have to be) and mock at that level. This gives you full control over what your mock does, while still allowing the real instances to use DbContext underneath.
  • Use the in-memory database if you want a low-overhead, functioning DbContext that you can use in tests, understanding that it still may not behave like a real database, just like a mock doesn't.

I think the first part of this guidance still stands. The second part is also still valid, but it turns out people had strong expectations that the in-memory database would behave exactly like their target database. See #18457 for an in-depth discussion. So I think we need better guidance overall about the pros/cons of testing at different levels.

With regard to, "When you get a query like X, return Y", we'll discuss it. It's interesting, but I'm not sure exactly where it fits.

DbContext scoping

We've been discussing this quite a lot in the last year. #18575 is the result, coupled with guidance on how to use it in different platforms.

Domain Modeling

This is certainly a place where there are a lot of opinions. :-) EF Core is already much better at this than EF6 since it can handle many more entity shapes, field mappings, entity type constructors, value conversions, and so on. We're also building up the support for aggregates and value objects although I think we made some wrong turns on the way. Issue #240 covers some of this, although it's an old issue that needs some cleanup.

Migrations

We're working on the deployment experience for migrations in EF Core 5.0.

being on a team that moves fast with overlapping updates ALWAYS ends in pain.

Is this with EF Core or EF6? If EF Core, then could you provide some more details on the types of issues that you run into? One of the goals for EF Core was to make this better--allowing conflicts to be resolved using source control in the same way as code conflicts.

Row / Field Level Permissions

It seems like query filters should work for this, or am I missing something important?

Thanks again for taking the time to give this feedback. :-)

@jwcarroll
Copy link
Author

@ajcvickers Thanks so much for the detailed reply.

Unit Testing

Use the in-memory database if you want a low-overhead, functioning DbContext that you can use in tests, understanding that it still may not behave like a real database, just like a mock doesn't.

This is the route I have gone when unit testing EF Core in the past. I'm well aware of the limitations, and those don't really bother me. It still requires a lot more setup than I would like. I don't think that's an issue that the in-memory solution needs to solve though.

Define a more specific interface (could be a repository but don't have to be) and mock at that level. This gives you full control over what your mock does, while still allowing the real instances to use DbContext underneath.

So I might be on a limb by myself here, but I actually avoid this for a lot of reasons. I have written my fair share of abstractions on top of DbContext over the years, but they always ended up introducing a lot more friction than they were worth.

DbContext is already a pretty glorious abstraction in itself, and any time I put something over top of it, I am basically stripping it of what makes it awesome. I have seen too many teams where you end up with 100 GetByXYZ methods in the abstraction. Worse, you end up effectively recreating all the same functionality all over again that already exists in DbContext in your abstraction.

For complex interactions I will 100% put that behind a service layer, but usually it's the work the service does consuming DbContext that I want to test.

With regard to, "When you get a query like X, return Y", we'll discuss it. It's interesting, but I'm not sure exactly where it fits.

I've been a full stack dev for a while, and there is some really interesting stuff going on with mocking HTTP endpoints in order to make unit testing in the front end easier. The work for mocking GraphQL is probably the closest analogy to what this would be like in EF.

See: https://www.apollographql.com/docs/graphql-tools/mocking/

It may be this is just a problem that is beyond the scope of EF team, but hopefully you can at least provide enough hooks, for someone to build tooling to achieve the above.

DbContext Scoping

This sounds awesome, and should help a lot!

Domain Modeling

Most of my experience was with EF6, and admittedly haven't done a ton in this particular space with EF Core. As I mentioned, I probably have the least to say here.

Migrations

EF6 and many, many other migrations solutions. These comments have more to do with my realization after many years and attempts to use tooling to solve this problem that all I really wanted was a canonical way to execute scripts as part of the deployment process.

This may be a more personal opinion, but I have typically encouraged everyone to ditch code based migrations in favor of simpler script based migrations. I have yet to find a team that wasn't happy making that kind of switch.

This may not jive with the goals of the people working on Migrations.

I will say that if there was a way to use the EF Migrations without ever inspecting code... but rather just creating and executing SQL scripts... that would be wonderful. This is especially helpful for people who are working in a DB-First environment.

Row / Field Level Permissions

I think you could probably do some of this using query filters, but in my experience these types of filters are so highly contextual, that they depend on runtime information.

You can sort of do this with query filters, but it is a little less dynamic.

I guess from my perspective, this is such a common scenario, that it feels like it should have first class support. Much the same way data annotations are given first class support.

Of everything in this list, this would be my first choice by a million percent!

As far as quality of life for most enterprise devs, I think this would be the biggest one. I could be wrong, but in my many years of consulting, how to correctly handle security and data entitlements is a question that comes up a lot. The solutions are always home grown as well, and can sometimes vary from team to team within the same organization.

Having a standardized way of declaring security like this would be an unbelievable boon to knowledge portability, and potentially lead to a bumper crop of new open source solutions to make this even more plug-n-play.

@ajcvickers ajcvickers added this to the Discussions milestone Mar 23, 2020
@ajcvickers ajcvickers added closed-no-further-action The issue is closed and no further action is planned. and removed type-enhancement labels Mar 23, 2020
@ajcvickers
Copy link
Member

Filed #20376 for row/field permissions.

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

2 participants