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

WIP. Adds query interception, resolves #60 #63

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

WIP. Adds query interception, resolves #60 #63

wants to merge 4 commits into from

Conversation

da1rren
Copy link

@da1rren da1rren commented Feb 20, 2019

This pull request allows the developer to specify typed predicate which will then be applied to allow LINQ -> SQL query requests.

An example of how to configure a global filter:

var cosmosStoreSettings = new CosmosStoreSettings(_databaseId, _emulatorUri, _emulatorKey,
    settings =>
    {
        settings.AddInterceptor<Cat>(x => x.Name == "Nick");
    });

I've added integration tests covering most of the basic scenarios.

todo

  • Add docs
  • Pull in QueryInterceptor.Core
  • Resolve feedback
  • Fix tests
  • Fix history

@da1rren
Copy link
Author

da1rren commented Feb 20, 2019

Looks like my history rewrite went a little wrong, just fixing it now.

Copy link
Owner

@Elfocrash Elfocrash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments on the code. There is also a sample missing, and documentation of the feature as both a brief mention of it in the main README file. If you wanna go the extra mile and also add it to the site documentation it's up to you.

I'm still a but iffy about where the interceptors list should sit. CosmosStoreSettings are meant to be reusable. I know that you made it so that interceptor won't have any effect if the entity is not the CosmosStore's entity but I'm still a bit concerned about it. I will play around with it later today and see how it feels from a developer experience.

src/Cosmonaut/Cosmonaut.csproj Outdated Show resolved Hide resolved
src/Cosmonaut/Cosmonaut.csproj Outdated Show resolved Hide resolved
src/Cosmonaut/CosmosStore.cs Outdated Show resolved Hide resolved
src/Cosmonaut/CosmosStoreSettings.cs Outdated Show resolved Hide resolved
src/Cosmonaut/CosmosStoreSettings.cs Show resolved Hide resolved
tests/Cosmonaut.System/CosmosQueryInterceptionTests.cs Outdated Show resolved Hide resolved
@Elfocrash
Copy link
Owner

Elfocrash commented Feb 20, 2019

I think, moving it to the CosmosStore constructor as it's own thing would be more appropriate. The reason is that there are two ways to initialise the CosmosStore.

  1. Using the CosmosStoreSettings
  2. Passing your own ICosmonautClient

If you do add it in the settings then you are limiting the feature to a specific initialisation method. I's rather have it in both.

Wouldn't it be better if we have a separate class that both constructors take where you store things like query interceptors, or operations before and after like a middle-ware?
See #58 for example. Mats needs an Action that's invoked before the operation happens. Interceptors and request delegates could sit together on a new class.

It would also allows us to control the TEntity and validate it in compile time.

What do you think about this idea?

EDIT: Also this whole feature is more of a Query Filter than a Query Interceptor. I know it's using query interception behind the scenes but the CosmosStore Query Filter is what will end up being exposed to the user.

@Elfocrash Elfocrash added the enhancement New feature or request label Feb 20, 2019
@da1rren da1rren changed the title Adds query interception, resolves #60 WIP. Adds query interception, resolves #60 Feb 20, 2019
@da1rren
Copy link
Author

da1rren commented Feb 20, 2019

@Elfocrash I agree it makes far more sense for it to exist on the CosmosStore constructor. I'll add some overrides to the constructors. It also saves a filter operation and resolves the issue with the IReadOnly collection.

@da1rren
Copy link
Author

da1rren commented Feb 20, 2019

@Elfocrash The integration tests appear to be failing on Az Devops. I cannot replicate locally and the error looks suspiciously similar to some issues with the emulator I've been having recently. Would you be able to investigate?

@Elfocrash
Copy link
Owner

Don't worry about integration tests. the Cosmos DB team has been doing some stuff with the emulator and it's failing on some agents. I will deal with that problem once the feature is done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants