Adds Aspire Oracle EntityFrameworkCore Database component. #1295

andrevlins · 2023-12-08T16:49:34Z

No description provided.

DamianEdwards · 2023-12-08T20:26:18Z

Thanks for this! Can you please log an issue that represents this work?

andrevlins · 2023-12-08T21:24:16Z

Thanks for this! Can you please log an issue that represents this work?

Of course, I created the issue #1307

mitchdenny · 2023-12-11T01:07:07Z

Triggered package migration for the Oracle EF NuGet package to unblock the build.

mitchdenny · 2023-12-11T01:08:12Z

...Aspire.Oracle.EntityFrameworkCore.Database/Aspire.Oracle.EntityFrameworkCore.Database.csproj

@@ -0,0 +1,24 @@
+<Project Sdk="Microsoft.NET.Sdk">


Why the .Database suffix? The NuGet package from Oracle does not have this.

I tried to follow the existing pattern of other EF components where we have Aspire.X.EntityFrameworkCore.Y where:

X is the company/project
Y is the product

Oracle calls its product Oracle Database, so I followed this pattern to leave it open for other Oracle products that may support EntityFramework.

Aspire.Oracle.EntityFrameworkCore.Database
Aspire.Microsoft.EntityFrameworkCore.Cosmos
Aspire.Microsoft.EntityFrameworkCore.SqlServer
Aspire.Npgsql.EntityFrameworkCore.PostgreSQL

Fair enough. I'm happy with that as a justification as long as @eerhardt is happy as the czar of Aspire components ;)

The naming pattern we use is in

aspire/src/Components/README.md

Lines 27 to 30 in 23e168d

## Naming

- Each component's name must have the prefix `Aspire.`.

- When component is built around `ABC` client library, it should contain the client library name in its name. Example: `Aspire.ABC`. Where the technology has a particular casing we have preferred that: for example `Aspire.RabbitMQ` rather than `Aspire.RabbitMq`.

This should be called Aspire.Oracle.EntityFrameworkCore.

mitchdenny · 2023-12-11T01:09:10Z

@eerhardt PTAL for the component side of this?

mitchdenny · 2023-12-11T01:13:12Z

@andrevlins we need APIs for the AppModel side of things are you planning on adding this? There isn't much point having a component API surface without the corresponding AppModel APIs :)

Take a look at the following for inspiration:
https://github.com/dotnet/aspire/tree/main/src/Aspire.Hosting/SqlServer

andrevlins · 2023-12-12T12:01:34Z

@andrevlins we need APIs for the AppModel side of things are you planning on adding this? There isn't much point having a component API surface without the corresponding AppModel APIs :)

Take a look at the following for inspiration: https://github.com/dotnet/aspire/tree/main/src/Aspire.Hosting/SqlServer

Yes I was doing this in another PR #1004
I brought the implementation from there and I will review it as there were some changes in the latest Aspire updates.

mitchdenny · 2023-12-13T02:30:48Z

src/Aspire.Hosting/Oracle/OracleDatabaseConnectionResource.cs

+/// </summary>
+/// <param name="name">The name of the resource.</param>
+/// <param name="connectionString">The Oracle Database connection string.</param>
+public class OracleDatabaseConnectionResource(string name, string? connectionString) : Resource(name), IOracleDatabaseResource


You can remove this type and its associated extension method. We've opted not to have XYZConnection resource types for now.

mitchdenny · 2023-12-13T02:38:59Z

I brought the implementation from there and I will review it as there were some changes in the latest Aspire updates.

Yep, we made some changes to try and clarify a few things.

You don't need the Oracle connection type anymore - with have a WithReference(ConnectionString) extension method for that kind of thing.

You'll need to add OracleServerResource in addition to OracleContainerResource. OracleServerResource would emit oracle.server.v0 to the manifest and OracleContainerResource would emit as a container.v0 resource.

The IOracleResource would become IOracleParentResource and both the container and server resource types would implement it ... so that the AddDatabase(...) extension would work on both of them. Take a look at the Postgres implementation for concrete examples of how this was setup.

Apologies for the shifting API underneath you we are still shifting things around a bit. If we can get this change in though, it means that when we do future API changes we have to keep this up to date ;)

andrevlins · 2023-12-14T00:34:55Z

You don't need the Oracle connection type anymore - with have a WithReference(ConnectionString) extension method for that kind of thing.

You'll need to add OracleServerResource in addition to OracleContainerResource. OracleServerResource would emit oracle.server.v0 to the manifest and OracleContainerResource would emit as a container.v0 resource.

The IOracleResource would become IOracleParentResource and both the container and server resource types would implement it ... so that the AddDatabase(...) extension would work on both of them. Take a look at the Postgres implementation for concrete examples of how this was setup.

Thank you very much for the explanations, I believe I made all the adaptations mentioned.

Apologies for the shifting API underneath you we are still shifting things around a bit. If we can get this change in though, it means that when we do future API changes we have to keep this up to date ;)

No problem! It's been great to follow the evolution of this project.

...Aspire.Oracle.EntityFrameworkCore.Database/Aspire.Oracle.EntityFrameworkCore.Database.csproj

...omponents/Aspire.Oracle.EntityFrameworkCore.Database/AspireOracleEFCoreDatabaseExtensions.cs

src/Components/Aspire_Components_Progress.md

src/Components/Aspire.Oracle.EntityFrameworkCore.Database/README.md

tests/testproject/TestProject.IntegrationServiceA/TestProject.IntegrationServiceA.csproj

DamianEdwards · 2023-12-15T17:57:04Z

src/Aspire.Hosting/Oracle/OracleDatabaseBuilderExtensions.cs

@@ -24,7 +24,7 @@ public static class OracleDatabaseBuilderExtensions
    /// <returns>A reference to the <see cref="IResourceBuilder{OracleDatabaseContainerResource}"/>.</returns>
    public static IResourceBuilder<OracleDatabaseContainerResource> AddOracleDatabaseContainer(this IDistributedApplicationBuilder builder, string name, int? port = null, string? password = null)
    {
-        password = password ?? Guid.NewGuid().ToString("N");
+        password = password ?? Guid.NewGuid().ToString("N").Substring(0, 30);


We should centralize this generated password logic into PasswordUtils @mitchdenny

Note that it is different than SqlServer.

aspire/src/Aspire.Hosting/SqlServer/SqlServerBuilderExtensions.cs

Line 25 in bc220e3

password = password ?? Guid.NewGuid().ToString("N") + Guid.NewGuid().ToString("N").ToUpper();

and MySql

aspire/src/Aspire.Hosting/MySql/MySqlBuilderExtensions.cs

Line 27 in bc220e3

password ??= Guid.NewGuid().ToString("N");

We should probably just come up with a decent password generator that we can use. What I did for SQL Server was half baked and really only appropriate for inner loop (which luckily is where it is used). I'd be happy to see this go in without change this password generation logic and instead see an issue created where we properly develop out PasswordUtils than can meet our various use cases (needs to be flexible enough to side step various limitations around what the containerized tool can used, and what we can escape in the connection string.

I know an issue hasn't been created yet, but I saw this comment and opened a PR that might address it: #1469.

src/Components/Aspire.Oracle.EntityFrameworkCore/ConfigurationSchema.json

eerhardt · 2023-12-15T23:07:53Z

tests/Aspire.Oracle.EntityFrameworkCore.Tests/ConformanceTests_Pooling.cs

+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "EF1001:Internal EF Core API usage.", Justification = "Required to verify pooling without touching DB")]


Can you move this to a #pragma only around the code that needs it?

eerhardt

This is getting pretty close. Thanks for this work @andrevlins.

eerhardt · 2023-12-15T23:14:31Z

src/Components/Telemetry.md

@@ -248,3 +248,30 @@ Aspire.StackExchange.Redis.OutputCaching:
 - Everything from `Aspire.StackExchange.Redis` plus:
 - Log categories:
  - "Microsoft.AspNetCore.OutputCaching.StackExchangeRedis"
+
+Aspire.Oracle.EntityFrameworkCore:


(nit) Can you keep these are alphabetically ordered?

Of course, I didn't understand the ordering, sorry.

eerhardt · 2023-12-15T23:15:50Z

src/Components/Aspire.Oracle.EntityFrameworkCore/README.md

+    builder.AddOracleDatabaseDbContext<MyDbContext>("orcl", settings => settings.HealthChecks = false);
+```
+
+## AppHost extensions 


This should also have a code snippet for the code to call AddOracleDatabaseDbContext in the service project.

eerhardt · 2023-12-15T23:18:18Z

src/Components/Aspire.Oracle.EntityFrameworkCore/OracleEntityFrameworkCoreSettings.cs

+    /// <summary>
+    /// <para>Gets or sets the maximum number of retry attempts.</para>
+    /// <value>
+    /// The default is 6.
+    /// Set it to 0 to disable the retry mechanism.
+    /// </value>
+    /// </summary>


Suggested change

/// <summary>

/// <para>Gets or sets the maximum number of retry attempts.</para>

/// <value>

/// The default is 6.

/// Set it to 0 to disable the retry mechanism.

/// </value>

/// </summary>

/// <summary>

/// <para>Gets or sets the maximum number of retry attempts.</para>

/// <para>Default value is 6, set it to 0 to disable the retry mechanism.</para>

/// </summary>

value isn't a valid element in summary. Repeated elsewhere. See

aspire/src/Components/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL/NpgsqlEntityFrameworkCorePostgreSQLSettings.cs

Lines 25 to 37 in bc220e3

/// <summary>

/// <para>Gets or sets the maximum number of retry attempts.</para>

/// <para>Default value is 6, set it to 0 to disable the retry mechanism.</para>

/// </summary>

public int MaxRetryCount { get; set; } = 6;

/// <summary>

/// Gets or sets a boolean value that indicates whether the database health check is enabled or not.

/// </summary>

/// <value>

/// The default value is <see langword="true"/>.

/// </value>

public bool HealthChecks { get; set; } = true;

for the latest format.

eerhardt · 2023-12-15T23:20:19Z

src/Components/Aspire.Oracle.EntityFrameworkCore/README.md

+  "Aspire": {
+    "Oracle": {
+      "EntityFrameworkCore": {
+        "Database": {


This is out-dated.

eerhardt · 2023-12-15T23:30:14Z

tests/testproject/TestProject.IntegrationServiceA/Program.cs

@@ -8,13 +8,15 @@
 builder.AddNpgsqlDataSource("postgresdb");
 builder.AddRabbitMQ("rabbitmqcontainer");
 builder.AddMongoDBClient("mymongodb");
+builder.AddOracleClient("freepdb1");


This test program should be using the Oracle EF Component you are adding, not a workaround method and talking to an OracleConnection directly.

Done. It makes perfect sense, I got a little lost here. Thank you for the explanation.

eerhardt · 2023-12-15T23:30:51Z

tests/testproject/TestProject.IntegrationServiceA/Oracle/OracleDatabaseExtensions.cs

+        app.MapGet("/oracledatabase/verify", VerifyOracleDatabaseAsync);
+    }
+
+    private static async Task<IResult> VerifyOracleDatabaseAsync(OracleConnection connection)


This should be

Suggested change

private static async Task<IResult> VerifyOracleDatabaseAsync(OracleConnection connection)

private static async Task<IResult> VerifyOracleDatabaseAsync(MyDbContext dbContext)

And then using the EF Component you are adding in this PR.

mitchdenny · 2023-12-20T08:28:18Z

I think we should merge this once preview 2 is out the door.

eerhardt

Just a few more comments to fix up, and then I think this will be good to merge.

Thanks again for the contribution!

Aspire.sln

src/Aspire.Hosting/Oracle/IOracleDatabaseResource.cs

src/Aspire.Hosting/Oracle/OracleDatabaseContainerResource.cs

src/Components/Aspire.Oracle.EntityFrameworkCore/README.md

tests/Aspire.Hosting.Tests/Oracle/OracleDatabaseFunctionalTests.cs

andrevlins · 2024-01-02T23:15:47Z

@eerhardt I will update the pr with the new updates to solve the build problems

andrevlins · 2024-01-03T00:12:05Z

@eerhardt I will update the pr with the new updates to solve the build problems

Ready

mitchdenny

LGTM!

eerhardt

This looks great. Thanks @andrevlins for the contribution!

andrevlins added 2 commits December 8, 2023 01:57

prepare directory

92a7922

Adds Aspire Oracle EntityFrameworkCore Database component.

0f2f3f8

dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Dec 8, 2023

DamianEdwards linked an issue Dec 8, 2023 that may be closed by this pull request

Add Aspire Oracle EntityFrameworkCore Database component #1307

Closed

mitchdenny reviewed Dec 11, 2023

View reviewed changes

mitchdenny added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Dec 11, 2023

AppModel apis

12ee396

joperezr added the community-contribution Indicates that the PR has been added by a community member label Dec 11, 2023

Merge branch 'main' into aspire-oracle-ef

0f384e8

mitchdenny reviewed Dec 13, 2023

View reviewed changes

andrevlins added 2 commits December 13, 2023 21:23

appmodel apis

773b457

tests for oracle hosting

06d7a4c

eerhardt reviewed Dec 14, 2023

View reviewed changes

andrevlins and others added 4 commits December 15, 2023 11:40

adjusting to reviews

4db7868

Merge branch 'dotnet:main' into aspire-oracle-ef

824ed28

Oracle has a 30 character password limit

91e0b7b

functional test

74a10a0

DamianEdwards reviewed Dec 15, 2023

View reviewed changes

Merge branch 'main' into aspire-oracle-ef

ae61db3

eerhardt reviewed Dec 15, 2023

View reviewed changes

src/Components/Aspire.Oracle.EntityFrameworkCore/ConfigurationSchema.json Show resolved Hide resolved

eerhardt reviewed Dec 15, 2023

View reviewed changes

Merge branch 'dotnet:main' into aspire-oracle-ef

9ac8eab

eerhardt reviewed Dec 15, 2023

View reviewed changes

andrevlins and others added 3 commits December 15, 2023 22:55

adjusting to reviews

e04cee1

Merge branch 'main' into aspire-oracle-ef

8b84294

configurationschema was outdated

5d99c70

andrevlins requested review from mitchdenny and eerhardt December 27, 2023 00:32

eerhardt reviewed Jan 2, 2024

View reviewed changes

adjusting to reviews

7c09cbd

andrevlins and others added 2 commits January 2, 2024 20:21

Merge branch 'dotnet:main' into aspire-oracle-ef

9abcdd8

updated with the latest bits

5f0b689

mitchdenny approved these changes Jan 3, 2024

View reviewed changes

eerhardt approved these changes Jan 3, 2024

View reviewed changes

eerhardt merged commit 398dd70 into dotnet:main Jan 3, 2024
8 checks passed

andrevlins deleted the aspire-oracle-ef branch January 5, 2024 00:19

github-actions bot locked and limited conversation to collaborators Apr 27, 2024

	## Naming

	- Each component's name must have the prefix `Aspire.`.
	- When component is built around `ABC` client library, it should contain the client library name in its name. Example: `Aspire.ABC`. Where the technology has a particular casing we have preferred that: for example `Aspire.RabbitMQ` rather than `Aspire.RabbitMq`.

	/// <summary>
	/// <para>Gets or sets the maximum number of retry attempts.</para>
	/// <para>Default value is 6, set it to 0 to disable the retry mechanism.</para>
	/// </summary>
	public int MaxRetryCount { get; set; } = 6;

	/// <summary>
	/// Gets or sets a boolean value that indicates whether the database health check is enabled or not.
	/// </summary>
	/// <value>
	/// The default value is <see langword="true"/>.
	/// </value>
	public bool HealthChecks { get; set; } = true;

	private static async Task<IResult> VerifyOracleDatabaseAsync(OracleConnection connection)
	private static async Task<IResult> VerifyOracleDatabaseAsync(MyDbContext dbContext)

Adds Aspire Oracle EntityFrameworkCore Database component. #1295

Adds Aspire Oracle EntityFrameworkCore Database component. #1295

Conversation

andrevlins commented Dec 8, 2023

DamianEdwards commented Dec 8, 2023

andrevlins commented Dec 8, 2023

mitchdenny commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Dec 11, 2023

mitchdenny commented Dec 11, 2023

andrevlins commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Dec 13, 2023

andrevlins commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Dec 20, 2023

eerhardt left a comment

Choose a reason for hiding this comment

andrevlins commented Jan 2, 2024

andrevlins commented Jan 3, 2024

mitchdenny left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment