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

Fix database issues #630

Merged
merged 3 commits into from
Nov 1, 2023
Merged

Fix database issues #630

merged 3 commits into from
Nov 1, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Nov 1, 2023

@eerhardt eerhardt enabled auto-merge (squash) November 1, 2023 22:11
@eerhardt eerhardt merged commit a617a54 into main Nov 1, 2023
5 checks passed
@eerhardt eerhardt deleted the FixDatabaseIssues branch November 1, 2023 22:19
eerhardt added a commit that referenced this pull request Nov 1, 2023
* Allow for configuring DbContextOptions in EF Components

Fix #438

* Enable Npgsql Meter instead of using EventCounters

Need to disable the prepared_ratio instrument until #629 is fixed.

Fix #442

* Update Telemetry doc for updated Metrics
joperezr pushed a commit that referenced this pull request Nov 1, 2023
* Allow for configuring DbContextOptions in EF Components

Fix #438

* Enable Npgsql Meter instead of using EventCounters

Need to disable the prepared_ratio instrument until #629 is fixed.

Fix #442

* Update Telemetry doc for updated Metrics
@@ -115,6 +117,8 @@ void ConfigureDbContext(DbContextOptionsBuilder dbContextOptionsBuilder)
builder.CommandTimeout(settings.Timeout);
}
});

configureDbContextOptions?.Invoke(dbContextOptionsBuilder);
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually right? Invoking this after the call to UseSqlServer seems wrong doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The users options/intentions should win over our default implementation. They always get the last say. So we invoke them at the end, after we are done configuring the options, so their options win.

Note that if we touch 2 properties, and they touch 2 other properties, all values "merge" together.

Copy link
Member

Choose a reason for hiding this comment

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

But EF has already looked at the options by the time you use it here. Also note that in your test, you end up with two calls to UseSqlServer one in the extension method, and one in your lambda to get to the SqlServerDbContextOptionsBuilder which doesn't seem right either.

Copy link
Member Author

Choose a reason for hiding this comment

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

But EF has already looked at the options by the time you use it here.

That isn't correct. EF won't look at the options until we return from our ConfigureDbContext method here. So the caller can still modify the options.

Also note that in your test, you end up with two calls to UseSqlServer one in the extension method, and one in your lambda to get to the SqlServerDbContextOptionsBuilder which doesn't seem right either.

But it works - as the test shows. I believe this is expected behavior, you can call UseXXX multiple times on the same DbContextOptionsBuilder instance and the mutations aggregate with the last one winning. That's at least what @roji told me this morning.

The alternative would be for Aspire to take 2 actions - one for the DbContextOptions and another for the Db specific options - ex. SqlServerDbContextOptionsBuilder. This doesn't seem desirable and is probably confusing.

cc @ajcvickers @bricelam

Copy link
Member

Choose a reason for hiding this comment

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

It works in this case, but its a) ugly and b) there is no guarantee that executing the delegate twice for a given context instance will work. It depends what code the user is running in the delegate.

Maybe what Shay was referring to is that both the delegate that builds the options for D.I. and the override of OnConfiguring can be used together. This is useful when some options are set using D.I. and some options are always set on the context and don't change based on D.I.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what's the recommendation here? If users need to be able to configure both, what should Aspire do?

cc @davidfowl

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I have not been included in any of the Aspire work, so I don't have any answer for that.

Copy link
Member Author

@eerhardt eerhardt Nov 2, 2023

Choose a reason for hiding this comment

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

Consider this me including you in Aspire work. 😉

Copy link
Member

Choose a reason for hiding this comment

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

I was going to file an issue on EF about DbContextOptions<T> not being composable (it doesn't use options under the covers) but I was waiting to see how this was going to pan out.

@JamesNK
Copy link
Member

JamesNK commented Nov 2, 2023

Metrics needs some improvement. I created an issue: #641

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for adding the tests!

joperezr added a commit that referenced this pull request Nov 14, 2023
@danmoseley danmoseley added the area-integrations Issues pertaining to Aspire Integrations packages label Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
8 participants