NHibernate.Caches.Redis
NHibernate.Caches.Redis copied to clipboard
TheCloudlessSky
Added NH5 support while retaining compatibility with previous versions of NH
Based on the discussion at https://github.com/TheCloudlessSky/NHibernate.Caches.Redis/pull/31, in order to keep compatibity with previous versions of NHibernate, the only change needed to be made is adding the new async methods to the cache.
This allows the user to reference and use:
- a previous version of NH (the new methods will not be part of the ICache interface contract and they are just... hmm... let's say "hanging in the air" ;) )
- NH5 (the new methods are now part of the ICache interface contract)
I've also marked these methods as virtual in order to let the user override them if needed.
The solution compiles fine, but I haven't tested it with a previous version of NH (as no functional changes have been made, it should work just like before). NH5 seems to be quite happy ;)
Many thanks @TheCloudlessSky for creating this lib and @gliljas for the NH5 support!
Hi @prunkster,
Interesting idea. Can you add some tests to confirm this works for NH<5 and NH5 (I'm thinking we'll need two separate projects for this)?
Unfortunately, that is not a viable solution. Compilation issues aside, the Nuget package needs the dependency to NH5.
I suggest dropping support for NH4. The existing package is stable and doesn't magically stop existing/working. Or, by all means, add a new package for NH5.
@gliljas why does it need a dependency on NH5 if this solution works? Also, you are one vote for dropping NH < 5 support out of 100s that may not be able to upgrade.
It needs the dependency or an assembly redirect, since it doesn't actually implement the new ICache interface (it should work, though, thanks to "virtual"). It's not super kosher, but quite usable.
My suggestion to drop the support is based on the fact that the reason to upgrade is NH5 support. I can understand if you want to have a bug fix path for NH4 users too, but IMHO it would be cleaner if it actually implemented the new interface. Perhaps in a separate NH5 package.
I support @gliljas here.
- You do not have to support all versions of the NH. It's like trying to sit on two (or three) chairs. Trying to do so you'll set yourself into the trap of harder pipeline and incompatibility bugs. The 5.0 is not binary compatible with the previous release (it targets different FX version, it has many interfaces changed, etc).
- The previous NuGet package is still available on NuGet.
So, what needs to be done is:
- Make a patch release to the old version of the package limiting it to NHibernate <5
- Release a new major version of the package which targets NH >5 & <6. Better if this is aligned with NH versioning.
I wouldn't believe that this solution is actually working until it is proven with tests. This solution might work with old code, but I'm more than sure that it will fail with the actual async API due to a "method not found" exceptions. To use new methods you'll need to write tests which use new async API. This has not been done in this PR.
@prunkster I've ran your tests locally and confirmed that NHibernate 5 is able to use the async methods (as was expected). We'll have to do a few things to clean this up, but overall I'm much happier to take this approach than separate packages (or dropping support all together).
@hazzik @gliljas can you please confirm this too? Also: you may have a way of doing things in NHibernate, but that's not how I choose to run my projects. I prefer high compatibility between versions as much as possible to ease developer pain (while also maintaining a balance of moving the ecosystem forward). Giving a damn about the end developer's experience (reducing surprises/friction, minimizing breaking changes, etc) is what I care about. From my ~10 years of experience with NHibernate, I find the second-level cache in NHibernate is actually more harm than good. I have found that it creates a lot of bad habits with developers and can actually hurt performance (this is experience with real-world, high usage apps). Because this project is already established and used by many people (especially NHibernate 4 usage) and I'm able to use a solution for this project that just works, it's a win-win for everyone to go this route.
It works and is functional. It's your prerogative to decide whether the implementation is the right route to take. I'm just saying that it's a bit of a hacky way to do it, since it relies on redirection and non-static behavior. I can't guarantee that it works in all scenarios.
As to the backwards compatibility aspect and breaking changes, that's what versioning and Nuget attempts to solve. If this was to be a new major version (or of course a separate package), the dependency on NH5 should be expected. Noone is forced to upgrade, and if they do they should expect breaking changes. It just happens in this case that it was possible to trick the framework into believing that there were no breaking changes. I will not object further.
Yes, second level caching is not a silver bullet. It works well in some scenarios, but requires the developer to be very careful. It could certainly be improved (e.g allow batching of both get and put), which may come as a PR for NH6 :)
This works because of this: the members implementing an interface are implicitly declared as virtual/override members on the CIL level. So, the virtual on new async methods here are not because "to let the user override them if needed", but because this is required. Without virtual it throws the "method not found" exception when you try to call these interface members. The only thing is missing is a method override table, which is, I guess, optional.
@hazzik I'd disagree with the usage of the virtual from your point of view... the virtual should be there as an optional method to implement. If something is mandatory, then an abstract method should take place, don't you think?
I don’t understand with what you disagree. I stated the fact why it works at all when it should not. The fact is that without virtual it would not work but throw, but with virtual the underlying bytecode would mimic the proper interface implementation.
With abstract it would not work because then the class would be abstract and be non instantiatable.
ah, sorry then... I misinterpreted the paragraph... The "This works because of this" got lost on my reading...
I see this is already approved. Any news of releasing this? Would be greatly appreciated.
