Elfocrash / Cosmonaut

🌐 A supercharged Azure CosmosDB .NET SDK with ORM support
https://cosmonaut.readthedocs.io
MIT License
342 stars 44 forks source link

Query interception #60

Open da1rren opened 5 years ago

da1rren commented 5 years ago

Hey Guys,

I was investigating Cosmonaut for a multi-tenanted system. Basically I was looking to override the CosmosStore to use the visitor pattern to ensure that queries always include a tenant id. However I see that CosmosStore is sealed. Is there any particular reason for sealing it?

Elfocrash commented 5 years ago

Hey @da1rren,

Can you please specify if you are using the fluent LINQ toSQL conversion or if you are using raw SQL queries?

The main reason why the class is sealed is because it wasn't designed to be inherited.

For example, in your case, you can attach this part of business logic in your predicate by simply adding this logic there. Ideally CosmosStore shouldn't be directly referenced in your Controller or top level layer but have some sort of Service or Repository that uses CosmosStore to provide appropriate behaviour.

da1rren commented 5 years ago

Thanks for the response @Elfocrash. We are using the LINQ to SQL conversion and while I appreciate where your coming from I'm looking to access the expression just before it's executed against the database. In order to implement, as you would describe I would have to create a facade over the top of the ICosmos store or build the expression then remember to pre-validate it before execution. However the reason I am willing to go to such extremes as rewriting the expression tree is to guarantee that only information for that tenant is returned.

For an example implementation see entity frameworks global filters.

I do believe it would be a worthwhile edition & am happy to fork and provide an implementation if its something you would consider useful, but if not no worries.

Elfocrash commented 5 years ago

See, I really really like the idea of having a Global Query Filter like EF and I would choose that over wrapping the CosmosStore. I don't really have the time currently to design it and implement it but if you are willing to fork and look into it, you are more than welcome to.

You could however go about it another way as well, by simply adding an extension method on your CosmosStore Queryables that dynamically adds the tenant predicate.

An example would be how I go about limiting the shared collections feature by adding a where clause with the following expression:

  internal static Expression<Func<TEntity, bool>> SharedCollectionExpression<TEntity>() where TEntity : class
{
    var parameter = Expression.Parameter(typeof(ISharedCosmosEntity));
    var member = Expression.Property(parameter, nameof(ISharedCosmosEntity.CosmosEntityName));
    var contant = Expression.Constant(typeof(TEntity).GetSharedCollectionEntityName());
    var body = Expression.Equal(member, contant);
    var extra = Expression.Lambda<Func<TEntity, bool>>(body, parameter);
    return extra;
}

I believe you can do something similar without creating a facade by adding an extension method on IQueryable called .TenantSpecific() which appends a where clause with this dynamic expression.

What are your thoughts on that?

da1rren commented 5 years ago

@Elfocrash I totally understand where your coming from with the extension method. However I would come at it the other way allowing a developer to make a specific request to not execute the filter logic e.g. .WithoutInterception() as I'd rather fail safe than fail open.

I suppose historically I've used query interception as a security method so I cannot allow a junior or distracted senior developer to accidentally display information from another tenant.

Elfocrash commented 5 years ago

That's all good on paper, but realistically, even if you were able to extend the CosmosStore, you would end up in the same situation as with having a facade wrapping the CosmosStore in the first place. I don't see the reason why you can't have the facade and inject the CosmosStore into it and wrapping the call with your interception logic.

It's outcome is exactly the same as extending the CosmosStore.

For example:

image

You would have to implement all the methods to add your tenant specific predicate anyway.

image

I know it's not pretty but it's probably the only way to satisfy your needs and it's the closest thing to what you requested.

Does that make sense?

da1rren commented 5 years ago

Yeah I see where your coming from. I have implemented a small query interception prototype on my fork gonna add a bunch of tests today and have a play.

So far it looks very straight forward.

Mortana89 commented 5 years ago

@da1rren, out of interest, do you store your tenant data in the same collection? Because we're having a collection per tenant, and was also looking at options to solve the singleton cosmosstore with fixed collection name. Was thinking about a factory as singleton that caches cosmosstores per tenant (based on requests), but am still digging in the code for other potential solutions.

da1rren commented 5 years ago

@Mortana89 We originally where untaking that approach but we are intending to have hundreds of seperate collections. We decided against it for the following reasons:

I've been doing quite a lot recently and have this working locally minus a few tweaks. But I am reconsidering using Cosmos for the larger project we have lined up as I do not want to have to constantly worry about if I have correctly filtered my data. I would rather have configured it & tested it in one central place.

Mortana89 commented 5 years ago

That's interesting, thanks!

In our case we won't have more than 1000 tenants. It's a niche B2B application, not just publically available to anyone so am less worried about that. Didn't know about the minimum RU though, thought that didn't apply if you have DB level RU?

da1rren commented 5 years ago

It does apply you will require 100 RUs per collection. Or at least you did last week when I investigated this.