NickCraver / StackExchange.Exceptional

Error handler used for the Stack Exchange network
https://nickcraver.com/StackExchange.Exceptional/
Apache License 2.0
862 stars 170 forks source link

chore: dependencies upgrade #222

Open NatMarchand opened 1 year ago

NatMarchand commented 1 year ago

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 commented 1 year ago

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.

NickCraver commented 1 year ago

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

NatMarchand commented 1 year ago

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

NickCraver commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

any update on this :) ?

Doxoh commented 10 months ago

@NickCraver any changes here?

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

tysonstolarski commented 2 months ago

Any updates on this? The System.Data.SqlClient dependency now throws an exception on .net 8: https://github.com/dotnet/runtime/issues/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 commented 2 months ago

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