Azure / azure-cosmos-dotnet-v3

.NET SDK for Azure Cosmos DB for the core SQL API
MIT License
736 stars 491 forks source link

Allow to pass types derived from CosmosContainer and CosmosDatabase to corresponding methods #1396

Closed xperiandri closed 3 years ago

xperiandri commented 4 years ago

Consuming Cosmos DB from F# I have a need to pass a container to a function. It is possible to pass not the right container to a function. The way you usually solve this issue is restricting received container by type. it is possible to derive from CosmosContainer but it is not possible to get an instance of it.

Please, add the ability to pass type derived from CosmosContainer to methods that produce a container from CosmosDatabase. And the same for CosmosDatabase from CosmosClient.

The most perfect solution would be to define a derived type with default constructor passing ContainerProperties to CosmosContainer constructor.

In C# you could create methods to work with that container on that derived class.

j82w commented 4 years ago

Composition is normally a better model over inheritance. Is there any reason you can't use composition to solve this issue?

public class MicrosoftContainer
{
     public CosmosContainer Instance {get;}
}
public class MicrosoftContainer : CosmosContainer
{
     private CosmosContainer container {get;}

     public override Task<ItemResponse<T>> CreateItemAsync<T>(
            T item,
            PartitionKey? partitionKey = null,
            ItemRequestOptions requestOptions = null,
            CancellationToken cancellationToken = default(CancellationToken))
    {
        return this.container.CreateItemAsync<T>(item, partitionKey, requestOptions, cancellationToken);
    }
}
xperiandri commented 4 years ago

Again, I can't produce MicrosoftContainer from Database. But what I want is to produce type guarded instance that can be statically checked on a build.

ealsur commented 4 years ago

I'm not fluent on F# so I might make interpretation mistakes but this sounds like a Repository Pattern? Where you want to have a Container for a particular schema and you want to be able to identify them at compile?

So a Repository<T> can be the contract you use, and each Repository<T> has a reference to the Container and a method to obtain it. Each Repository<T> might have a naming rule to know which is the id/name of the container it needs to reference.

j82w commented 4 years ago

@xperiandri any chance you can provide an example of what the current code is and what you would like it to be if @ealsur suggestion didn't help? I'm also not fluent in F#, and having an example would make it easier to understand.

xperiandri commented 4 years ago

Option 1

type BooksDatabase () = inherit CosmosDatabase("Books")
// booksDatabase becomes of type BooksDatabase
let booksDatabase = client.CreateDatabaseIfNotExistsAsync<BooksDatabase>()

type UsersContainer () = inherit CosmosContainer("Users")
// usersContainer becomes of type UsersContainer
let usersContainer = booksDatabase.CreateContainerIfNotExistsAsync<UsersContainer>()

let asyncGetUsers (container : UsersContainer) = ...
xperiandri commented 4 years ago

Option 2

// Books would be a phantom type/interface
// booksDatabase becomes of type CosmosDatabase<Books>
let booksDatabase = client.CreateDatabaseIfNotExistsAsync<Books>("Books")

// User can be a real entity used for that container of phantom type/interface
// usersContainer becomes of type CosmosContainer<User>
let usersContainer = booksDatabase.CreateContainerIfNotExistsAsync<User>(containerProperties)

let asyncGetUsers (container : CosmosContainer<User>) = ...
xperiandri commented 4 years ago

The main point of this is to know at compile time that you will never put a container with "books" where a container with "cars" expected.

ealsur commented 4 years ago

The thing is that by definition, Cosmos DB is schemaless, so a container can contain items with different schemas. Adding a direct overload that takes a type would hint users that there is some sort of schema verification/validation, and sounds like it would set the wrong expectation. And for databases the conceptual gap is even larger (it does not even apply on a SQL world). What do you think @j82w ?

Build time validation can be again achieved by a Repository wrapping right? That repository can have it's on Container instance:

public class Repository<T>
{
    private readonly CosmosClient client;
    public Container Container {get;private set;}
    public Repository(CosmosClient client)
    {
        this.client = client;
        this.Container = client.GetContainer("database", typeof(T).Name);
    }

    public async Task CreateIfNotExists()
    {
        Database database = await client.CreateDatabaseIfNotExists("database");
        await database.CreateContainerIfNotExists(this.Container.Id);
    }
}

var repository = new Repository<Book>(client);

// Then you can use Repository<T> as input through the code

await repository.CreateIfNotExistsAsync();

Container container = repository.Container;
j82w commented 4 years ago

I agree with @ealsur that Cosmos DB doesn't have any schema validation. Is there any reason @ealsur sample won't work?

xperiandri commented 4 years ago

Why do you relate my request to schema existence or non-existence? It is about compile-time guard against container and database mix up.

ealsur commented 4 years ago

A Container related to a <Type> conceptually translates to the Container for the Type, where the Type sounds in C# as the schema.

This conceptually aligns with other data handling libraries like Entity Framework (DbSet<T>).

That is why I mention that adding Container<T> would be interpreted that way by a C# user, and not as simply a compiler time guard. Which is something I wouldn't agree with, it gives the wrong expectations.

If your goal is to have a guard on compile time that a particular container is related to a particular type regardless of what is stored inside, it can be achieved as I described through a Repository<T>. It achieves what you need and there is no type polluting needed on the SDK classes that could lead to confusion for users used to data libraries in C#.

xperiandri commented 3 years ago

OK, but why do you seal containers?

ealsur commented 3 years ago

@xperiandri What do you mean? Container is not sealed https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/Resource/Container/Container.cs#L28

I still don't understand why the proposed code doesn't achieve what you need (build time checking). As explained before, Cosmos DB is schema-less, so adding any sort of <T> to the Container types would not make much sense, at least for me.

xperiandri commented 3 years ago

So the main flow is that you get Container from Database https://github.com/Azure/azure-cosmos-dotnet-v3/blob/94914c41aad9b1996599a515168fee3e710eb7fc/Microsoft.Azure.Cosmos/src/Resource/Database/Database.cs#L392

How do I need to achieve that in my case?

xperiandri commented 3 years ago

Imagine that I have a CosmosClient instance what code do I need to write to instantiate my derived Database and my derived Container classes?

ealsur commented 3 years ago

You'd have to inherit from Database, make your GetContainer call return an instance of your class instead I guess.

You can read the container information as a Stream and deserialize it to a custom class, which could be inheriting from ContainerProperties and have the extra methods you need.

I still don't understand why you need a custom Container type, if it's the reason explained before, the Repository pattern solves it.

xperiandri commented 3 years ago

Because I already have an issue where someone put one container where another is expected

xperiandri commented 3 years ago

I want to prevent that on compile time

bartelink commented 3 years ago

@xperiandri I wonder can you use FSharp.UMX and/or the techniques therein to type-tag the containers the way you are seeking?

ealsur commented 3 years ago

I want to prevent that on compile time

The Repository approach mentioned on the thread can address that without needing to override the Container or anything else, doesn't it?

xperiandri commented 3 years ago

Do you mean creating a decorator on top of the Container with the same API? I don't use Repository pattern, it is not a functional way of doing things. I have a function that takes a Container. I can only wrap a Container into a decorator with the same API.

Container has 46 abstract methods. This means that I need to write 93 lines of useless code just to create a decorator if possible.

bartelink commented 3 years ago

I mean that your logic will request a Container<users>, which uses units of measure to make it different from Container<images>

It's a compile time strong type alias using F#'s unit of measure feature. There is no decorator of any kind - you are just wrapping the actual CosmosContainer in a tagged type alias that makes it not be interchangeable without you doing an explicit mapping.

Then you have

type CosmosDatabase with
    member Users : UsersContainer = %this.Container("Users")
    member Images : UsersContainer = %this.Container("Images")

(obv you want to cache the container instance creation.

bartelink commented 3 years ago

@xperiandri you'll need a line like this to make Containers type-taggable in this way.