Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Parameter filters values cached between context instances #8

Open
xabikos opened this Issue · 42 comments
@xabikos

In continuation of the discussion here (http://lostechies.com/jimmybogard/2014/05/29/missing-ef-feature-workarounds-filters/#comment-1457366765) when we set a value in a filter parameter this value cached and never change even if we create different DbContext instances. This happens probably because in the implementation of filters we use the FilterInterceptor which Implements IDbCommandTreeInterceptor and the execution of this method happens only once for query commands and then this is cached. So for example in soft delete functionality that Miller present in TechEd video this works fine as we filter against a hard coded boolean value. In case we want to filter on tenant Id then we always get back the results for the first tenant. Pay attention to the remarks in the documentation here https://entityframework.codeplex.com/SourceControl/latest#src/EntityFramework/Infrastructure/Interception/IDbCommandTreeInterceptor.cs

@jmwnoble

I think I'm having this problem as well. Works fine in local development but tenantId filter has no effect on Azure SQL/Azure Website Combo.

Jeremy

@jbogard
Owner

I wonder why my tests passed then...this would make interceptors rather useless, it seems.

@siegeon

I have a workaround. I access all of my data through a generic repository (as most of us do) Since I need to be able to change the tenant key every time a query is made against my database, and Entity Framework automatically caches composed queries, I figured we should just disable that caching.

The important thing to take away from the attached image is the first three lines of code. Every time you run your query against your object set you must disable EnablePlanCaching (I could not figure out how do to this at the context level) When disabled the results of DbScanExpression are not cached and the new values set by context.EnableFilter(*).SetParameter are used in every query composition.

screenshot_1

@leddt leddt referenced this issue from a commit in leddt/EntityFramework.Filters
@leddt leddt Add failing tests relating to issue #8 af0229c
@leddt

Hey I ran into the same problem so I added some failing tests illustrating a few ways that this issue is affecting the expected behavior (see my fork).

The workaround by @siegeon, while clever, is not easily applicable to my situation.

I hope a stable workaround can be found!

@hikalkan

Hi @jbogard,

Did you find any general solution for that? I want it to use in my ASP.NET Boilerplate project (http://www.aspnetboilerplate.com/) for soft delete and multi-tenancy at first.

@xabikos

@hikalkan as I explained in my initial comment the problem exists when querying the database so you can use it for soft delete or assign the tenant id before insert a new record in the database.

@hikalkan

@jbogard,
Assigning tenant-id is OK but I also want to use while querying. It should retrive only current tenant's entities.

@jcachat

I have come up with a solution to this problem that is working very well for me. I'm using it to enforce multi-tenancy and soft deletes globally throughout my data layer. It feels a bit hack-ish, but I thought I'd share what I did since I learned how to do it from the code in this library. Maybe it will be a viable solution to fix the issue here or will help someone else to implement this...

Since IDbCommandTreeInterceptor.TreeCreated is only called once when a query is first run, the problem we are having is that the filter values cannot be changed after that first query - they are stored in the cached query. But, what we can do is intercept the SQL before it is sent to the DB and change those values on the fly via an IDbCommandInterceptor.

The current QueryVisitor in this library results in the parameter values being stored as constants & string literals in the SQL (not as SqlParameters). So I have created a new QueryVisitor that injects a filter into the query such that a SqlParameter is added to the final query. Like this:

    public override DbExpression Visit(DbScanExpression expression)
    {
        var filterList = expression.Target.ElementType.MetadataProperties
            .Where(mp => mp.Name.Contains("customannotation:" + DynamicFilterConstants.ATTRIBUTE_NAME_PREFIX))
            .Select(m => m.Value as DynamicFilterDefinition);

        DbExpression current = base.Visit(expression);

        foreach(var filter in filterList)
        {
            var binding = DbExpressionBuilder.Bind(current);
            var columnProperty = DbExpressionBuilder.Property(DbExpressionBuilder.Variable(binding.VariableType, binding.VariableName), filter.ColumnName);
            current = DbExpressionBuilder.Filter(binding, DbExpressionBuilder.Equal(columnProperty, columnProperty.Property.TypeUsage.Parameter(filter.ParameterName)));
        }

        return current;
    }

Then I implemented my own IDbCommandInterceptor. On each of the "Executing" methods, I can inspect the SqlParameters on the DbCommand, see if any match my filters, and update the values on them.

My implementation is much simpler in terms of linq support. I only need to match a column against a value (using an equality test). But it works great and does not add any overhead to my queries.

I created a quick project for all of the relevant code and pushed it to github here: https://github.com/jcachat/EntityFramework.DynamicFilters. If there is interest, I can clean it up, add more to it, and make it a proper project. Or jbogard can use it in his project if he likes. I used quite a bit of his code & ideas to make mine. :)

@mrbaseball2usa

@jcachat I would be interested in seeing a PoC project demoing the code you've linked to...

Thanks much!

@cocowalla

Any updates on a possible solution for this? Something along the lines of jcachat's solution (i.e. using SQL parameters) seems like a good option.

@jbogard
Owner

I believe #13 fixes this, can you confirm?

@cocowalla

@jbogard no, #12 & #13 allow using database column names that don't match .NET property names and makes sure it doesn't throw for NavigationPropertyConfiguration. Nothing in there to prevent caching.

@cocowalla

@jbogard I double-checked, and the issue is indeed still there.

@siegeon

Im not somewhere to test your code, but I seem to recall running into an issue like this one with my project. Every query is cached the first time its executed. If you try to compare the effectiveness of your global filter with two different queries it will work because the second query is new and is compiled with the second tenant id as its filter. Try doing the test with the same query and only change the filter value.

@cocowalla cocowalla referenced this issue from a commit in cocowalla/EntityFramework.Filters
@cocowalla cocowalla Add failing test relating to issue #8
Values are still being cached when using DbContext.Set<>
2a44541
@cocowalla

The issue is still there when using DbContext.Set<>, rather than using DbSet<> properties on the context. I always access entities using DbContext.Set<>, so I don't need to create a property for every entity type on the DbContext (it's a pain for big projects, and I don't really get the point when we can just use DbContext.Set<>).

Not sure why there is a difference between the 2 methods, but I've added a failing test.

@cocowalla cocowalla referenced this issue from a commit in cocowalla/EntityFramework.Filters
@cocowalla cocowalla Use the same query when testing for cached values (issue #8)
The original test wasn't using the same query, and so wasn't actually
testing if values were being cached. This test is now failing, as values
are still being cached.

Also removed previous test, as it was added in error.
eca73b0
@cocowalla

@siegeon you are right, there was a 'bug' in the original test, and it's actually nothing to do with DbContext.Set<> vs DbSet<> (I'd fixed the 'bug' in my new test!).

@jbogard now the test is using the same query before and after changing the tenant, you should be able to reproduce this issue

@GasyTek

Hi,

The solution proposed by @jcachat works for me and it is the most clean solution I can think of.

The basic idea is to use a parameter when building the query tree into the Visitor, something like DbExpressionBuilder.Parameter(..., "MyParameterName").

Then intercept the command into an implementation of IDbCommandInterceptor, look for a parameter named "MyParameterName" then set its value there, as it is always called even when Entity Framework cache the query plan.

This works great for me until now :)

HTH

Riana

@cocowalla

@GasyTek could you submit a pull request? It would be nice to get this sorted in EntityFramework.Filters

@GasyTek

Hi,

Sorry, I implement the fixes on a personal project and I didn't know EntityFramework.Filters until now so I can't make a pull request.

But the idea is what I've said :

  1. Implement an interceptor (inherits from IDbCommandTreeInterceptor)
  2. Implement a visitor, modify the select query to add a where clause. In the where clause use a parameter for filter values. Parameters can be created by using DbExpressionBuilder.Parameter.
  3. Implement a command interceptor (inherits from IDbCommandInterceptor), in each implemented methods, we have a DbCommand object that represents the query command. Add the parameter value to this DbCommand object.

The IDbCommandTreeInterceptor implementation will be called just once because Entity Framework uses query plan caching by default, but the IDbCommandInterceptor implementaion is always called.

HTH

Riana

@xabikos

It's a great idea @GasyTek but if you could provide a sample then it would be easier to understand it.

@jbogard
Owner

Yeah just a gist would be good. I figured this was the right way to go, but I'm just not familiar enough with EF to know the way to go. And trying to trace through the EF codebase wasn't really helping.

@GasyTek

Do you want me to create a sample project then send it to you ?

@jbogard
Owner

Sure, whatever's easiest for you.

@jcachat

All of the code to do this is in the git hub link in my post above. Or if GasyTek improved on it, I would like to see his code too.

@cocowalla

@jcachat your code has a problem in that it requires database column names to match the C# property names, so I presume it runs in SSpace when it needs to run in CSpace. I opened an issue in your repo a while back.

@GasyTek

I have a sample project but I'm confused I don't know github enough to post it here ..
Could someone give me his email so that I can send it ? (my email : gasytek@gmail.com)

@mrbaseball2usa

I'd love to see the sample project when it gets posted as well!

Thanks!

@jcachat

@cocowalla Strange, I didn't get an email about your post over there. It is definitely using SSpace. I'll look at the changes you posted to this project to see if I can incorporate them in mine.

I'm going to try to find some time to put a sample and a readme together for my github project. I emailed GasyTek to see if what he's done might help me speed that up. I also just pushed out a fix to it for an issue with Scoped vs. Global params so if anyone's using it, you may want to get that. I just haven't put any effort into it because I didn't want to steal the project away from jbogard...

@jbogard
Owner

I honestly don't care - you could submit a PR that replaces my entire implementation. This was an exercise in exploring EF's features for a blog post series.

@jcachat

For anyone interested, this weekend I updated my project to include a readme, example project, fixed the CSpace issue, and published it to NuGet.

It didn't seem right to replace this project and I didn't want to break anyone using it.

If you decide to use it, please post any issues or feedback over there.

@xabikos

Thanks @jcachat for you example project it is very helpfull. But I would like to comment on the result of the produced SQL after filtering. Here https://github.com/xabikos/EfMultitenant I have a simplified example of a multitenant approach where we filtering on tenantId and also we assign the value of tenantId when inserting a row and filtering based on tenantId during update and delete. My question is related to filtering functionality. More analytically if we try to find based on Id an entity and apply the automatic filtering based on tenantId then we get SQL like this:
"SELECT
[Extent1].[Id] AS [Id],
[Extent1].[Category] AS [Category],
[Extent1].[TenantId] AS [TenantId]
FROM ( SELECT [Var_2].[Id] AS [Id], [Var_2].[Category] AS [Category], [Var_2].[TenantId] AS [TenantId]
FROM [dbo].[ProductCategories] AS [Var_2]
WHERE [Var_2].[TenantId] = @TenantId
) AS [Extent1]
WHERE [Extent1].[Id] = @p0',N'@TenantId nvarchar(128),@p0 bigint',@TenantId=N'f6d39662-2744-4377-b455-2cdf333d0f71',@p0=3"
which is caused from the QueryVisitor class and the filtering we apply there. A much simpler query with the same results would be this of course:
"exec sp_executesql N'SELECT
[Id] AS [Id],
[Category] AS [Category],
[TenantId] AS [TenantId]
FROM [dbo].[ProductCategories]
WHERE [Id] = @p0 AND [TenantId] = @TenantId',N'@TenantId nvarchar(128),@p0 bigint',@TenantId=N'f6d39662-2744-4377-b455-2cdf333d0f71',@p0=3"
I don't know if there is any performance hit because of the filtering and if we can somehow achieve the second query in a simplified version where no generic filtering is required.

@jcachat

My first thought is that there's probably nothing we can do about that. EF seems to generate those embedded selects whenever new DbExpressions are added (which is what's happening in that Query Visitor). But, I want to explore more about this query visitor to see what else can be done with it and if there is a better way to add these expressions. So maybe there will be an opportunity to tune that up a bit. I also know there is an issue if you create multiple filters on the same entity (maybe a tenant related table that is soft deletable) - it will generate an embedded select for each filter which could end up performing poorly and not using indexes correctly. That, I know I can fix.

I will say though that I have checked performance on those embedded selects many times. While they are ugly to look at, they do perform just fine (based on query stats from profiler & query analyzer). But I agree with you that if there's a way to avoid them, I would like to do that.

Would you mind posting this issue here so we can track it there?

@jbogard
Owner

@jcachat As long as you don't mind if it doesn't seem right to "borrow" your implementation for this project :)

@Januschan

@jbogard I was wondering if you ever updated your NuGet package with the outcome / integration of the @jcachat multi-tenancy solution. I think both packages are pretty excellent, but would like to see the best of both worlds.

@jbogard
Owner

I checked the implementation, but unfortunately it's pretty much limited to one single parameter. And when I tried to do more advanced scenarios, I was stuck. And it's really hard to understand the internals of the EF query generation, so I'm more or less waiting for EF7 to retarget. No point in doing a ton of work if the internals get modified anyway.

@Januschan
@jcachat

I am working on adding linq/lambda support to my solution. It may be somewhat limited in what can be done, but it will provide much more than just single column equality conditions. Keep tabs on jcachat/EntityFramework.DynamicFilters#7 and jcachat/EntityFramework.DynamicFilters#9 for updates.

I have it mostly working right now and I'm hoping to have something release-able in the next couple weeks. The 'in' scenario is one I am going to try to support.

@Januschan
@jbogard
Owner

@jcachat @Januschan Reviewing my implementation - honestly I don't think it's good long-term. It's accessing internal methods using reflection.

Unfortunately, my needs do require arbitrary lambdas, with NHibernate you can do some really complex things and we use filters to manage securing data on some rather gnarly access rules. Like "you can only access data if you're assigned to, or own, this case" or "you can access this data if you've been assigned to a special group", permission-based access and the like. NHibernate's solution is great in that it allows you to specify any arbitrary HQL, and that's the base query that all queries in NH flow to. I had wanted arbitrary lambda support, because it's a lot harder to take a simple solution and grow it to a generic one than just start on the generic one.

Unfortunately, the EF API at this level is not really envisioned for this sort of scenario, so you can't easily throw arbitrary expressions at it.

@Januschan
@erichexter

@jbogard @jcachat So where does this net out? Does https://github.com/jcachat/EntityFramework.DynamicFilters replace this library? If so, how about updating the readme, to say this was a prototype and link to DynamicFilters?

@Korijn Korijn referenced this issue in jcachat/EntityFramework.DynamicFilters
Closed

What about query plan cache? #25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.