dotnet / aspire

An opinionated, cloud ready stack for building observable, production ready, distributed applications in .NET
https://learn.microsoft.com/dotnet/aspire
MIT License
3.49k stars 376 forks source link

Aspire.Npgsql component does not create database #1170

Open alexwolfmsft opened 8 months ago

alexwolfmsft commented 8 months ago

When providing a custom name for a database, the builder.AddPostgresContainer("pg").AddDatabase("postgresdb") methods do not seem to create the database in the Postgres container. For example, with the following code:

var postgresdb = builder.AddPostgresContainer("catalog").AddDatabase("catalogdb");

var postgres = builder.AddProject<Projects.AspireSQL>("aspirepostgres")
                       .WithReference(postgresdb);

And:

builder.AddNpgsqlDataSource("catalogdb");

I receive this error: PostgresException: 3D000: database "catalogdb" does not exist

When I use psql commands in the container to view the list of databases, it looks like this:

image

Should I expect to see a "catalogdb" database listed here?

The connection and query does work if I switch to AddDatabase("postgres") to use the default database.

DamianEdwards commented 8 months ago

This is by design. The database name is to ensure the appropriate connection information is provided but it's not intended that the database will be auto-created by the hosting APIs given all the variables that can impact database creation.

eerhardt commented 8 months ago

The database name is to ensure the appropriate connection information is provided but it's not intended that the database will be auto-created by the hosting APIs given all the variables that can impact database creation.

I think this is confusing, and not consistent with our other hosting APIs. The first call is "AddPostgresContainer", which creates a Postgres container. The second call is "AddDatabase", which doesn't create a database? That seems wrong that the same verb doesn't do the same thing.

alexwolfmsft commented 8 months ago

@eerhardt It's interesting you mention this because that's the exact reason I assumed it auto generated the db - it autogenerates a redis setup for you using similar methods/naming.

eerhardt commented 8 months ago

Re-opening to discuss this a bit more. I'm not sure the current behavior is the best we can do.

alexwolfmsft commented 8 months ago

Re-opening to discuss this a bit more. I'm not sure the current behavior is the best we can do.

I know you've seen https://github.com/dotnet/aspire/issues/1168 as well, but just wanted to point out the other db components don't autogenerate the database either - this seems to be a Redis vs Databases inconsistency.

Some .NET tools have historically autogenerated databases for you locally, though I think that was only when using EF.

DamianEdwards commented 8 months ago

Redis is more akin to a service than a database, in that you get a Redis instance and connect to that. With databases, you typically get a server which hosts multiple databases, and the database is part of the connection detail. All that said, I'm more than happy to discuss further whether we think the Aspire.Hosting layer should do more to ensure the configured database is actually available.

/Cc @davidfowl @roji for their thoughts.

mitchdenny commented 8 months ago

The moment you start auto creating the database you need to start specifying things like collations and that kind of stuff.

The other part of the problem is that you have to figure out where you want to provision the database. If you say you want to do it in the AppHost then suddenly the app host needs to have client drivers for each database which could be dependency hell.

Then you get into lifetime considerations such as the AppHost not being around during deployment so you still need something else to create the database anyway.

Better off to face these considerations up front.

It is possible for a deployment tool like AZD to be enhanced to support creating the database since in the Azure ARM world a database is actually a resource in the resource provider. But this is certainly cloud platform dependent.

IEvangelist commented 8 months ago

Perhaps, we should consider renaming the AddDatabase API. It make more sense as AddDatabaseConnection to make it clearer, that it's not in fact adding a database, but rather a connection to one?

mitchdenny commented 8 months ago

LOL ... it is funny how we keep recycling nouns :) We just removed AddPostgesConnection(...) ... and now looking to add AddPostgres(...).AddDatabaseConnection(...) which does something different.

I've never been super enthused about AddDatabase(...) myself so I'm more than open to renaming it. I'm not sure AddDatabaseConnection(...) is any better. Perhaps we need something like SelectDatabase(...).

DamianEdwards commented 7 months ago

There's a PR out that adds a sample on how to initialize databases when using database containers now: https://github.com/dotnet/aspire-samples/pull/61

mitchdenny commented 6 months ago

Flagging for offsite because this has come up a bit.

mitchdenny commented 5 months ago

I think we are comfortable with where this is for now. When provisioning you can get the database created for you because the permission set for deployed resources is usually a bit stricter.

For local development a worker app to manage database creation is our current guidance.

We may revisit this when we look at commanding/tasking.

jakoss commented 3 months ago

I agree that AddDatabase is confusing in this sense. I just wasted too much time getting this information. Renaming it would be nice, but i think more important would be to add information about that to the documentation. It's not mentioned anywhere here https://learn.microsoft.com/en-us/dotnet/aspire/database/sql-server-entity-framework-component?tabs=dotnet-cli . It's only briefly mentioned in the seed data https://learn.microsoft.com/en-us/dotnet/aspire/database/seed-database-data?tabs=sql-server . I didn't got to this part of tutorial since i didn't succeed in connecting in the first place.

This is worse in the case of SqlServer i believe, since i got an exception when trying to connect to the database. SqlServer is verifying the Database=name part of connection string. If the database does not exist it throws with authentication exception which is misleading too. So even if i wanted to create the database from code - i can't, the connection string will not work.

I think it would be even better to create those databases as part of the AppHost configuration though

DamianEdwards commented 3 months ago

This is worse in the case of SqlServer i believe

I'm fairly certain this happens for other database engines too like PostgreSQL.

So even if i wanted to create the database from code - i can't, the connection string will not work

Entity Framework Core actually handles this specifically in its database creation routines and reconnects to the database with a modified connection string (by stripping off the database name). There's no reason your apps can't do something similar (assuming you're not using EF Core).

I think it would be even better to create those databases as part of the AppHost configuration though

The resistance to this is that that requires having the database client referenced by the AppHost project and the logic to check for and if not present create the database needs to be specifically implemented for every different database hosting extension (as it's not universal). This is exactly what provider-based ORM libraries like Entity Framework Core exist to do and we're hesitant to reimplement that in the Aspire hosting layer. All that said, I acknowledge that it still might be the right thing to do as this has been a consistent piece of feedback we've had since the first preview.

jakoss commented 3 months ago

Entity Framework Core actually handles this specifically in its database creation routines and reconnects to the database with a modified connection string (by stripping off the database name). There's no reason your apps can't do something similar (assuming you're not using EF Core).

That does explain my issue. I have one service with normal DbContext using postgres and it's working fine. Other service is using SqlServer and it's a Identity Server. I'm trying to set persistence to Identity Server and it's not using DbContext, hence the error i guess.

The resistance to this is that that requires having the database client referenced by the AppHost project and the logic to check for and if not present create the database needs to be specifically implemented for every different database hosting extension (as it's not universal). This is exactly what provider-based ORM libraries like Entity Framework Core exist to do and we're hesitant to reimplement that in the Aspire hosting layer. All that said, I acknowledge that it still might be the right thing to do as this has been a consistent piece of feedback we've had since the first preview.

Understandable, I guess it's not really that bad. But I'm quite sure it should be documented better, both in the learn.microsoft.com and in the code docs :)

holytshirt commented 2 months ago

I just hit this and though I was doing something wrong for hours. I did not realise the database was not created. In docker compose

    image: postgres:16-alpine
    environment:
      - POSTGRES_USER=postgres
      - POSTGRES_PASSWORD=postgres
      - POSTGRES_DB=zitadel

And database is there ,,,

DamianEdwards commented 2 months ago

@holytshirt yes the PostgreSQL container supports changing the name of the default database via the POSTGRES_DB environment variable, as does the MySQL container. The DatabaseContainers sample shows this. However not all database containers have this mechanism, including the SQL Server container.

We're still considering what we might do to have databases specified via AddDatabase be automatically created if they're not present but haven't settled on a plan as yet.

ozzioma commented 2 months ago

Is there a scenario for connecting to an existing database? That is without provisioning a container for one?

DamianEdwards commented 2 months ago

Yes, you can use builder.AddConnectionString to model an existing database instance

atrauzzi commented 3 weeks ago

@mitchdenny -- is #4160 an evolution to this conversation? :pray:

mitchdenny commented 3 weeks ago

No we provably won't go that direction. We are considering allowing the hosting packages to reference client libraries to support doing things like creating databases for local development.

atrauzzi commented 3 weeks ago

Sounds good, I'm sure you guys will sort out the best approach.

Any good options in the interim? I'd really like to champion the gradual adoption of Aspire in my group, but I hope I'm not jumping into another hurry-up-and-wait. :cold_sweat:

manojlds commented 3 weeks ago

So how am I supposed to handle this? This is trivial with existing docker-compose setups.

eerhardt commented 3 weeks ago

This is trivial with existing docker-compose setups.

How do you do it with docker-compose?

Matthewsre commented 1 week ago

I encountered the same confusion with .AddDatabase(...) not actually creating the database. I see the value in supporting both creating the database and just referencing it.

Here are a few options that make sense to me:

Option 1: Prefix Conventions (Add, With, ...)

Option 2: Suffix Conventions (Reference, ReferenceOnly, ...)

Option 3: Optional Argument (create = true)

Additionally, I would like to see these same considerations made for containers/collections for resources such as CosmosDB and MongoDB.

For context, I use Terraform to build environments (dev, test, ppe, prod), which creates databases and sometimes containers/collections. I'm transitioning to Aspire to simplify dev environment configurations and reduce/remove Terraform. Beyond the dev environment, I plan to use Aspire for generating manifests compatible with Terraform, Pulumi, Bicep, etc. (something similar to one of my examples here: terraform-azurerm-microservices/examples/exampleB/main.tf at master · Matthewsre/terraform-azurerm-microservices (github.com))