Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ESDB container implement IDatabaseContainer #1053

Closed
wants to merge 1 commit into from

Conversation

alexeyzimarev
Copy link

What does this PR do?

EvebtStoreDbContainer class implements the GetConnectionString method so it can inherit from IDatabaseContainer.

Why is it important?

When creating an abstraction for tests, I can't currently use IDatabaseContainer and its GetConnectionString because EventStoreDbContainer doesn't implement the interface.

Copy link

netlify bot commented Nov 22, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 32dd615
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/655df3ed567913000834b090
😎 Deploy Preview https://deploy-preview-1053--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

The interface represents a database container instance that can be accessed with an ADO.NET provider. Does ESDB support ADO.NET? We test against the db client.

@alexeyzimarev
Copy link
Author

I already figured it out by looking at failing test, and I am not exactly following that decision. The interface doesn't suggest anything like that, so it's unclear from the API surface what is the intention behind that interface.

From the test perspective, and looking at other container implementations, it looks like MonoDB for example, is not a database when we all know it definitely is a database. Or Neo4j, or ESDB in that regard. I personally find it confusing.

@HofmeisterAn
Copy link
Collaborator

The interface doesn't suggest anything like that, so it's unclear from the API surface what is the intention behind that interface.

Indeed, the interface itself does not, but the description does. IIRC, the initial idea was IAdoNetDatabaseContainer or something similar.

From the test perspective, and looking at other container implementations, it looks like MonoDB for example, is not a database when we all know it definitely is a database.

No one is saying that MongoDB or ESDB are not databases, but abstraction becomes significant when commonality exists. You cannot easily swap MongoDB with ESDB as you can with ADO.NET compatible containers (AFAIK). But it is definitely ambiguous. Such as the publicly available methods to retrieve the connection string, endpoint, base address, etc. Typically, I aim to correspond to the client library.

When creating an abstraction for tests, I can't currently use IDatabaseContainer and its GetConnectionString because EventStoreDbContainer doesn't implement the interface.

Could you provide more details about what you are trying to abstract? I would like to get a better understanding of the use cases.

@alexeyzimarev
Copy link
Author

I have a base class that is used across different implementation, which are database-specific. In my case, the abstraction is on a high enough level to just need the GetConnectionString be available across all those containers so I can pass the connection string to downstream implementations without exposing much other details about the container implementation.

I ended up making the base class generic, where the generic parameter is constrained to DockerContainer, and all inheritors use their own container types, and be able to call Container.GetConnectionString(). So, I had to expose container as a protected property, which I didn't want to.

@HofmeisterAn
Copy link
Collaborator

Maybe we can add support together with this issue. Right now, I am considering a connection string provider or something similar, which is capable of resolving the correct connection strings. This would allow developers to override the default connection string and could be used for the abstraction too.

@HofmeisterAn
Copy link
Collaborator

As mentioned, the interface is exclusively for ADO.NET compatible containers. I've generated a follow-up issue that includes a proposal for implementing a connection string provider across all modules. I'll close this PR. Let's carry on the discussion in the referenced issue. Thank you for bringing this to my attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants