Cosmonaut icon indicating copy to clipboard operation
Cosmonaut copied to clipboard

Query interception

Open da1rren opened this issue 5 years ago • 10 comments

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?

da1rren avatar Feb 18 '19 12:02 da1rren

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.

Elfocrash avatar Feb 18 '19 13:02 Elfocrash

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.

da1rren avatar Feb 18 '19 14:02 da1rren

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?

Elfocrash avatar Feb 18 '19 14:02 Elfocrash

@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.

da1rren avatar Feb 18 '19 14:02 da1rren

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?

Elfocrash avatar Feb 18 '19 15:02 Elfocrash

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.

da1rren avatar Feb 19 '19 14:02 da1rren

@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.

Mortana89 avatar Mar 07 '19 19:03 Mortana89

@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:

  • Change feeds become unusable. This means that aggragation becomes more complex and you lose a lot of the value of cosmos db. (At least from my perspective) from the lose of change feeds
  • Each collection has a minimum of 100 RUs. This translates into around $6 cost. Which quickly scales with multiple tenants
  • Your limited to around 2k collections per cosmos database. I'm sure support could override this.
  • Its a pita to manage

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.

da1rren avatar Mar 07 '19 20:03 da1rren

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?

Mortana89 avatar Mar 07 '19 20:03 Mortana89

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

da1rren avatar Mar 07 '19 20:03 da1rren