Skip to content
This repository has been archived by the owner on Jul 12, 2020. It is now read-only.

Query interception #60

Open
da1rren opened this issue Feb 18, 2019 · 10 comments
Open

Query interception #60

da1rren opened this issue Feb 18, 2019 · 10 comments
Labels
question Further information is requested

Comments

@da1rren
Copy link

da1rren commented Feb 18, 2019

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
Copy link
Owner

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 Elfocrash added the question Further information is requested label Feb 18, 2019
@da1rren
Copy link
Author

da1rren commented Feb 18, 2019

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
Copy link
Owner

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
Copy link
Author

da1rren commented Feb 18, 2019

@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
Copy link
Owner

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
Copy link
Author

da1rren commented Feb 19, 2019

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
Copy link

@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
Copy link
Author

da1rren commented Mar 7, 2019

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

@Mortana89
Copy link

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
Copy link
Author

da1rren commented Mar 7, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants