search

AliBazzi / IdentityServer4.Contrib.RedisStore

A persistence layer using Redis DB for operational data and for caching capability for Identity Server 4
https://www.nuget.org/packages/IdentityServer4.Contrib.RedisStore
MIT License
137 stars 48 forks source link

Add option to re-throw exception #11

Closed dpix closed 5 years ago

dpix commented 5 years ago

Currently exceptions are getting swallowed, I'd like to be able to let them bubble up. Without this, a refresh token could get issued to a client that can never actually be used

Set the option to false by default so should be backwards compatible

Also changed log levels to error inside catch statements.

Updated from netstandard1.6 to 2.0

AliBazzi commented 5 years ago

Hi @dpix

Thank you for opening this PR, although I think it's my mistake to swallow the exceptions as the current state, let's not add it as an option, and re-throw the exceptions, since the consuming components expect those exceptions. So please modify the code to propagate the exceptions no matter what.

The second concerns is that I need to understand what you think is the purpose of upgrading the .net standard version ? what is the benefit ? I would recommend to not do that otherwise you have a valid point about this change.

Thanks again !

dpix commented 5 years ago

Ok, I only added it as an option for backwards compatibility I guess but happy to remove the option entirely.

The reason I made this PR at all was actually to update the .net version in the hope it might resolve a tricky bug/performance issue we are currently facing. Happy to undo but seems like it doesn't do any harm to upgrade, all tests continue to pass. Otherwise I could just add it as another PR?

Cheers, Dan

AliBazzi commented 5 years ago

Great, thanks Dan for the explanation.

As far as I know the upgrade of the .net standard is for the standard, not for the version, so if we have a bug let's say in .net core 1.1, and it's good to proceed to upgrade to .net core 2.1 that's totally fine for the library, it's still relying on the contract presented in .net standard 1.6, which is included by any later version beyond .net core 1.1, so .net core 2.x satisfies the requirement by .net standard 1.6 out of the box, because actually they satisfies .net standard 2.0.

I'm not sure about the bug you are facing, maybe you can open an issue and we can discuss it there.

Anyway, I will not be able to merge the PR because of upgrade of the .net standard version, since identity server 4 v 1.0 for example, relies on .net standard 1.6, and upgrading the library to .net standard 2.0 will force customers who are still investing in a framework running on .net standard 1.6, and not willing to upgrade to .net standard 2 (which is implemented the least by .net core 2) to upgrade.

Hope the point is clear about .net standard upgrade concerns.

Regards.

AliBazzi commented 5 years ago

Any feedback @dpix ?

dpix commented 5 years ago

Sorry for the late reply, we've had a long weekend in Australia.

Totally fair point, I'll just include the updated logging and remove the framework version update.

dpix commented 5 years ago

Updated to always throw, also logs the error first.

Turns out my other issue was resolved by updating aspnetcore2.1 packages 👍

AliBazzi commented 5 years ago

merged, thank you !

AliBazzi commented 5 years ago

@dpix Released to Nuget as 1.4.5