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

3.x Release: net8.0 and Microsoft.Data.SqlClient support #222

Merged
merged 10 commits into from
Nov 30, 2024

Conversation

NatMarchand
Copy link
Contributor

Hey there!
I've made a PR to upgrade dependencies mainly to switch to Microsoft.Data.SqlClient (solves #193) and took the opportunity to upgrade other dependencies.
Among other stuffs I've also used centralized nuget packages versions.
Please note that, although I've made my best to be non breaking, in order to do this I've changed the required framework version from net461 to net462 and removed the target net461 from aspnetcore projet (as aspnetcore on net framework has not been supported for a long time).
I've thought about supporting Microsoft.Data.SqlClient and System.Data.SqlClient side by side by moving the mssql store to another assembly and using reflection for catching SqlException (like it's done for the redis exception) but I'm afraid of the perf penalty which could occur.
I'd be happy to talk to you if you need :)

@NatMarchand NatMarchand force-pushed the chore/dependencies-update branch 2 times, most recently from 6713db7 to cd4f6c6 Compare May 22, 2023 07:56
Copy link
Owner

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Thanks for this - it's overdue and this is great! There's a few things we'd need to do to make this happen with such major version bumps though:

  1. Some people are still using System.Data.SqlClient, and by changing it we make IntelliSense everywhere say "...which SqlConnection?" and fun like that.
  2. We should probably bump the version to signify this, I guess a 3.x (version.json in root).

For the first, I just never got around to it but do not want to break people. I think what needs to happen is .Shared depends on neither SQL client, and instead we have 2 new packages: StackExchange.Exceptional.SqlClient and StackExcahnge.Exceptional.SystemDataSqlClient (naming suggestions welcome). IIRC the split only got tricky because there's one place we hook up SQL exception handlers to get more information automatically on SQL errors and we wouldn't want to lose that. Perhaps it happens in some static initializer that registers a global default or...something. I don't think we need reflection (and it'd be hosting to AOT), but need some thoughts.

Any chance you thought about the above already? Suggestions?

@NatMarchand
Copy link
Contributor Author

NatMarchand commented May 22, 2023

Ok, I tried to extract store + default handler method to two packages (one for each SqlClient).
Now, I need to find out how to call the method.

Btw, I noticed that the packages were not shipped with deterministic build. I've tuned a little build props. Is it ok for you ?

Marking the PR draft meanwhile.

@NatMarchand NatMarchand marked this pull request as draft May 22, 2023 13:42
@NatMarchand NatMarchand force-pushed the chore/dependencies-update branch from 699524a to cbc077d Compare May 22, 2023 13:45
@NickCraver
Copy link
Owner

@NatMarchand last visiting this simply predates deterministic being a thing - absolutely welcome that improvement <3

@NatMarchand NatMarchand marked this pull request as ready for review May 22, 2023 14:31
@NatMarchand
Copy link
Contributor Author

Okay, quite naive implementation but seems to work : I've made a static ctor in both SQLErrorStore which adds the sqlexception action

@NickCraver
Copy link
Owner

NickCraver commented May 22, 2023

@NatMarchand I gotta switchover to work stuff but can try to poke at this likely tomorrow (kiddo birthday!). The current problems looking at this local are:

  1. We've inadvertently dropped the StackRedis.CacheException handler.
  2. Libraries have Client on naming...but these aren't clients, we should shave that off.
  3. Users could have both libraries, so we'll have namespace and typename conflicts. We'll probably need to go with SystemDataSQLErrorStore and MicrosoftDataSQLErrorStore (this also makes it a cleaner breaking change that causes people to gut check when upgrading, but not an onerous one). Same prefix for Extensions class...though I think these go away, see below - they share type/namespace with base so conflict anyway.
  4. The extensions appear to work but are only on the static instance, people may have instances of settings anywhere, created at any time.

I think what I'm imagining here is an actual static dictionary for defaults that we register to, and that collection is used in the existing AddDefault() method (loop and add). I think we can use C#9 module initializers for hookup here. This way any module that's loaded can register some default handlers...but users can also override and kill those. It'd be a new property on ExceptionalSettingsBase, e.g. public static Dictionary<string, Action<Error>> DefaultExceptionActions { get; } = new(); - thoughts?

@NatMarchand NatMarchand force-pushed the chore/dependencies-update branch from bbc180a to 8c583ad Compare May 22, 2023 15:17
@NatMarchand
Copy link
Contributor Author

Ok, meanwhile, I've fixed the github action build.

  1. will add it back
  2. okay, will try another naming
  3. agreed

For the module initializer thing, I tried it but couldn't make it work on netstandard target (nor netframework) maybe only working on net ?
I'll try to see what I can do.

@NickCraver
Copy link
Owner

@NatMarchand I think calling the add to the default dictionary in the static constructor is fine too, can tweak after if needed - no need for extension class though, can just inline it there IMO.

@NatMarchand NatMarchand force-pushed the chore/dependencies-update branch 2 times, most recently from 5f37f82 to 0013e26 Compare May 23, 2023 07:40
@NatMarchand
Copy link
Contributor Author

NatMarchand commented May 23, 2023

As agreed yesterday, I made some changes : added back the redis action, changed and moved the assemblies to a path without "Client"
I've also fixed a %CD% that was hiding in the github action script which had a weird effect.
However, I'm still confused about default actions. If I add a static dictionary to the Statics static class it could do the trick however there's now a problem of order : static ctor of SQLStore is executed only when type is "discovered" which happens only lazily in the DefaultStore property and so there's already an instance of ExceptionalSettings.

@NatMarchand NatMarchand force-pushed the chore/dependencies-update branch from 0013e26 to 63b69f4 Compare September 7, 2023 13:55
@NatMarchand NatMarchand force-pushed the chore/dependencies-update branch from 63b69f4 to 93b3f2d Compare September 7, 2023 14:50
@NatMarchand
Copy link
Contributor Author

Hi there!
I've pushed a new version of my code with some updates.
I still have trouble for adding sqlexception handler as default and no idea on how to fix.

@Poltuu
Copy link

Poltuu commented Nov 7, 2023

any update on this :) ?

@Doxoh
Copy link

Doxoh commented Jan 17, 2024

@NickCraver any changes here?

have some vulnerable warning for the latest version of StackExchange.Exceptional.AspNetCore
image

@tysonstolarski
Copy link

Any updates on this?
The System.Data.SqlClient dependency now throws an exception on .net 8:
dotnet/runtime#86969
And it looks like it won't be fixed given the move to Microsoft.Data.SqlClient.
We've cleaned our projects of all references to System.Data.SqlClient, and Exceptional is the last remaining package to pull in a reference.
As it stands now, it looks like we'll have to remove Exceptional to proceed with our .net 8 upgrades. :(

@nsmithdev
Copy link

We have been using the lib in dotnet 8 successfully for a while. Based on the blog post below system.data.sqlclient is supported in dotnet 8 but will be deprecated and no longer supported starting with dotnet 9.

https://techcommunity.microsoft.com/t5/sql-server-blog/announcement-system-data-sqlclient-package-is-now-deprecated/ba-p/4227205

@NickCraver NickCraver changed the title chore: dependencies upgrade 3.x Release: net8.0 and Microsoft.Data.SqlClient support Nov 30, 2024
Copy link
Owner

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort here - I've been heads down on some other stuff for far too long here, trying to catch up during a few off days here and get releases out.

@NickCraver NickCraver merged commit c956059 into NickCraver:main Nov 30, 2024
2 checks passed
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.

6 participants