App-vNext
Allow policy registries to be defined for keys of any type
Is your feature request related to a specific problem? Or an existing feature? Please describe.
Currently the PolicyRegistry class is tied to string keys, as opposed to being generic on the key. The result is that it's not possible to create a PolicyRegistry with a different type as the key.
For example, if I want to key my policies by System.Uri, the only option I have is to implement IConcurrentPolicyRegistry<Uri> - which makes no sense because I don't want or need any different semantics from what PolicyRegistry does, I just want it to allow me to use a different key type. I also don't see anything on PolicyRegistry that is string-specific.
Describe your proposed or preferred solution:
- Rename the current
PolicyRegistrytoKeyedPolicyRegistry<TKey>. Create a "new"PolicyRegistryclass that inherits it, i.e.PolicyRegistry : KeyedPolicyRegistry<string>.
(Ideally we'd change PolicyRegistry to PolicyRegistry<TKey> and create a StringPolicyRegistry : PolicyRegistry<string>, except that would be a breaking change.)
-
PollyServiceCollectionExtensionsshould get 2 new generic overloads ofAddPolicyRegistrythat allow specifying the key type of the policy implementation. These will useKeyedPolicyRegistry<TKey>. -
PollyHttpClientBuilderExtensionsshould get 3 new generic overloads ofAddPolicyHandlerFromRegistrythat handle the generic key scenario.
Describe any alternative options you've considered:
Unending pain and torment, AKA implementing IConcurrentPolicyRegistry myself, registering a generic version of it myself, et cetera.
Any additional info? Nope.
I'm not sure if it definitely is a breaking change, but adding a new generic type as the base class of PolicyRegistry feels it on a binary level, even if it isn't at the source level (guidelines suggest it's a maybe).
I don't know the history of why it wasn't implemented as a generic class in the first place. Going through the history only takes me as far back as it being renamed from DefaultPolicyRegistry to PolicyRegistry around Polly v5.
It feels to me that this is how it should have been implemented, but without any further context as to why it is the way it is I can't really comment on whether it's just a mistake or a considered decision.
If it did change to allow this use case out-of-the-box then this wouldn't happen until at least v8.0.0 (whenever that happens). Then depending on whether it is breaking or not, we'd need to coordinate it with a new major ASP.NET Core release (it's quite late in their release cycle for new functionality in 6.0 now) to update their minimum version from 7.2.2 and for PRs to add the overloads you've suggested.
A potentially less breaking way of addressing the same (at least on the Polly side, not ASP.NET Core's) would be to add an additional PolicyRegistry<T> type that is generic and leave the non-generic version alone. Then it's totally opt-in and doesn't break any existing code, while also not requiring the consumer to write their own implementation. The non-generic version could then be made obsolete and/or inherit from the generic version in a future release (with the breaking changes caveats I've already mentioned).
As vexing as this seems to be for you, this isn't blocking as all the extensibility you need to implement this yourself do exist, it's just a bit more code to write/copy-paste-edit.
I was curious as to how much/little code it was with all the comments removed and it comes to 45 lines ignoring whitespace, which I've included below.
using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Concurrent;
namespace Polly.Registry
{
public class PolicyRegistry<T> : IConcurrentPolicyRegistry<T>
{
private readonly ConcurrentDictionary<T, IsPolicy> _registry = new ConcurrentDictionary<T, IsPolicy>();
public int Count => _registry.Count;
public void Add<TPolicy>(T key, TPolicy policy) where TPolicy : IsPolicy =>
_registry.Add(key, policy);
public bool TryAdd<TPolicy>(T key, TPolicy policy) where TPolicy : IsPolicy
{
return registry.TryAdd(key, policy);
}
public IsPolicy this[T key]
{
get => _registry[key];
set => _registry[key] = value;
}
public TPolicy Get<TPolicy>(T key) where TPolicy : IsPolicy =>
(TPolicy) _registry[key];
public bool TryGet<TPolicy>(T key, out TPolicy policy) where TPolicy : IsPolicy
{
bool got = _registry.TryGetValue(key, out IsPolicy value);
policy = got ? (TPolicy)value : default;
return got;
}
public void Clear() =>
_registry.Clear();
public bool ContainsKey(T key) =>
_registry.ContainsKey(key);
public bool Remove(T key) =>
_registry.Remove(key);
public bool TryRemove<TPolicy>(T key, out TPolicy policy) where TPolicy : IsPolicy
{
bool got = registry.TryRemove(key, out IsPolicy value);
policy = got ? (TPolicy) value : default;
return got;
}
public bool TryUpdate<TPolicy>(T key, TPolicy newPolicy, TPolicy comparisonPolicy) where TPolicy : IsPolicy
{
return registry.TryUpdate(key, newPolicy, comparisonPolicy);
}
public TPolicy GetOrAdd<TPolicy>(T key, Func<T, TPolicy> policyFactory) where TPolicy : IsPolicy
{
return (TPolicy) registry.GetOrAdd(key, k => policyFactory(k));
}
public TPolicy GetOrAdd<TPolicy>(T key, TPolicy policy) where TPolicy : IsPolicy
{
return (TPolicy) registry.GetOrAdd(key, policy);
}
public TPolicy AddOrUpdate<TPolicy>(T key, Func<T, TPolicy> addPolicyFactory, Func<T, TPolicy, TPolicy> updatePolicyFactory) where TPolicy : IsPolicy
{
return (TPolicy) registry.AddOrUpdate(key, k => addPolicyFactory(k), (k, e) => updatePolicyFactory(k, (TPolicy)e));
}
public TPolicy AddOrUpdate<TPolicy>(T key, TPolicy addPolicy, Func<T, TPolicy, TPolicy> updatePolicyFactory) where TPolicy : IsPolicy
{
return (TPolicy)registry.AddOrUpdate(key, addPolicy, (k, e) => updatePolicyFactory(k, (TPolicy)e));
}
public IEnumerator<KeyValuePair<T, IsPolicy>> GetEnumerator() => _registry.GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();
}
}
It's so nice to interact with someone who is passionate about the project and willing to take time to address/answer questions, no matter how daft they may be. @martincostello you are an exemplar of how interactions on GitHub should be - really, thank you. I wish I could say that my interactions with actual MS employees had generally been this helpful but alas...
(I also totally didn't forget that generic types are not considered the same as non-generic types with the same name, despite doing .NET for over 15 years at this point. I swear I'm not getting old and forgetful...)
Anyhow, it sounds like you broadly agree with my proposal, so what're the next steps? Do I need to write up a formal doc like dotnet/runtime requires, or does the Polly team need to discuss it further, or...? The reason I ask is that I'd be happy to contrib the code for this (to both Polly and ASP.NET Core) as it doesn't appear to be a huge change in terms of effort, but obviously I'm not gonna do anything until you guys give the go-ahead.
Off the top of my head, I think a rough plan if we went down the generic class route (rather than a rename) would be:
- Raise a PR here to add the
PolicyRegistry<T>class. As that would be an additive change, we could ship that relatively quickly (if there's no dissenting opinions to it) as a minor release for 7.3.0. - Once/if that update is published to NuGet, open a PR to ASP.NET Core for the 6.0 release to bump the dependency version to 7.3.0. I would have thought this would have a good chance of making it in before they branch from
mainfor the final release. I've made this a separate step as it doesn't tie making the dependency version available out-of-the-box dependent on also adding the extension methods. Here's the PR where I updated it from 7.1.0 earlier this year: https://github.com/dotnet/aspnetcore/pull/31708. - Open an API proposal issue in the ASP.NET Core repo for the new extension methods you propose that use the new generic key implementation. Given the timelines I'm less optimisitic this would make it in for 6.0 and would end up in 7.0 if they agreed to the proposed additions.
Then in the future for 8.0.0 we could either change the existing non-generic class to be a subclass of the new generic one, or we could just mark it as obsolete instead to err on the side of backwards-compatibility.
Also, thanks for the kind words Ian. Much appreciated. 😃
I just found myself in need of the same thing. Much appreciated if this would be implemented :) By the way, why not make the non-generic class a subclass of the new generic one in step 1 (maybe even marking it obsolete without making it an error). This would make it possible for v 8.0.0 to completely remove the non-generic class :)
