Added PolicyRegistry Feature

[ankitbko]

Implementation of PolicyRegistry Proposal #226

[dnfclas]

Hi @ankitbko, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[reisenberger]

Hi @ankitbko Huge thanks for this contribution around the PolicyRegistry (and all your initial ideas, which really helped shape it!). This is looking great. I've made some comments, mainly around the specs - what do you think?

[reisenberger]

Namespace should be Polly.Specs.Registry, to match others. (Visual Studio will have inserted that .SharedSpecs. on file creation, due to the name of the shared specs library...)

[ankitbko]

Completely missed this one out. Fixed.

[reisenberger]

Because the test assigns the same instance policy to r[key] twice, it doesn't prove that the first value at r[key] has been overwritten by the second assignment. But it looks like this test has been superseded anyway by Should_Overwrite_Existing_Policy_If_Key_Exists_When_Inserting_Using_Idexer(), which does check the overwrite 👍 . So, perhaps this test can simply be deleted?

[ankitbko]

Ah.. Missed to delete it. Done now.

[reisenberger]

Should the indexer section of this test be removed, now that we have the test Should_Allow_Adding_Policy_Using_Indexer() below?

(Or: If intent is to show/test that we can mix adding policies using .Add(...) and indexer, maybe state that explicitly in the // comment, or pull out into separate test with title stating that, eg: Should_be_able_to_mix_adding_policies_with_Add_and_indexer.)

[ankitbko]

Removed.

[reisenberger]

We've tended to use lower case for words in the spec name, except if that word is a class / property / method etc normally capitalised in C#. We've also tended to use Should_be_able_to instead of Should_allow (apart from where we mean allow only one instead of allow two)

So: Should_be_able_to_add_Policy_using_Add()

(No biggie, but it would be nice to make consistent.) (Similarly, in other specs.)

[ankitbko]

Done. Didn't knew about this. I also tend to keep things consistent and follow the practice of the project. Will keep this in mind in my future contributions. 😁

[reisenberger]

Where spec is inserting two different policies against two different keys, it may be slightly clearer (quicker to understand intent at a glance), if we use different variables for the second policy and key (eg key2, policy2), rather than re-using existing variables policy and key. What do you think?

(Similarly, in other specs.)

[ankitbko]

Wouldn't make any difference in behavior as I was re-initializing the variables but I see your point. I actually had done the same before but removed it later. 😞 Will make the changes.

[reisenberger]

? The property is retrieved ?

[ankitbko]

Haha!!! Nice catch. To start with I had copied the comments from IDictionary since we had earlier inherited from it. Seems even IDictionary has a lot of mistakes. 😁 Fixed.

[reisenberger]

To follow the standard Microsoft style for generic type parameters, I suggest we call the type parameters TKey and TPolicy.

[ankitbko]

Done

[reisenberger]

otherwise, null.

[ankitbko]

Done. Default value for reference types would have returned null. Making it explicit makes it more clear.

[reisenberger]

Maybe .ShouldNotThrow() is redundant (can be removed) throughout this test. .ShouldNotThrow() is perfect where it adds semantic value to the test, to state "this call shouldn't throw" - to make a contrast with elsewhere in the test (or other circumstances perhaps in different tests) where the same call might throw. In this case, we never expect ContainsKey(...) to throw with a non-null key, so maybe _registry.ContainsKey(key).Should().BeTrue(); is just more readable?

What do you think? Simplicity better? See if you think the same applies elsewhere?

[ankitbko]

I have removed .ShouldNotThrow() which were redundant unless, as you mentioned, to make a contrast with elsewhere in the test (Should_not_be_able_to_add_Policy_with_duplicate_key_using_Add) and where the test explicitly says Should_not_throw (Should_not_throw_while_retrieving_when_key_does_not_exist_using_TryGetValue)

PS: I hadn't worked with FluentAssertion much before. Thanks for helping me out with best practices. Really appreciate it.

[reisenberger]

Would be nice also to have the complementary test: Should_throw_while_retrieving_using_indexer_when_key_does_not_exist

[ankitbko]

Added. I also added more test cases to check ArgumentNullException is thrown when key is null while adding, removing and retrieving policy. Plus since the number of tests has now increased, I grouped them intro different region.

[reisenberger]

@ankitbko Apologies for my delay replying (due to volume of paid work!). Many thanks for the revisions! This is perfect for PolicyRegistry for policies of non-generic type Policy!

When travelling two weeks ago, I realised we hadn't provided a PolicyRegistry<TResult> for policies of type Policy<TResult>.

(Background: In Polly, Policy<TResult>. does not extend Policy, because the original non-generic Policy class also has a generic method .Execute<TResult>(...). During discussion around v4.3.0/v5.0, people preferred to keep that overload (for its flexibility; and removing would be a big breaking change). But its existence necessarily means Policy<TResult> cannot extend Policy. Policy<TResult> otoh is essential for strongly-typed .HandleResult<TResult>(...), and strongly-typed composition of PolicyWraps.)

What we need to do

To complete the PolicyRegistry offering, I think we need to add a PolicyRegistry<TResult> : IPolicyRegistry<TKey, TResult>, for policies of type Policy<TResult>. Could I suggest you:

• Make a PR of your branch onto new v5.0.7 branch I created rather than master
• Then either you or I can add the PolicyRegistry<TResult> concept there, add doco etc.

I think the need for IPolicyRegistry<TKey, TResult> means we should probably simplify public interface IPolicyRegistry<TKey, TPolicy> where TPolicy: Polly.Policy, replace it simply with public interface IPolicyRegistry<TKey, Policy>. Otherwise, the new public interface IPolicyRegistry<TKey, TResult> has a signature-clash with IPolicyRegistry<TKey, TPolicy>.

Views?

[ankitbko]

@reisenberger No need to apologies. Its been great working with you. Thanks for such detailed discussion on all topic. And excellent spot that we missed Policy<TResult>.

I did not fully understand what you mentioned here of PolicyRegistry<TResult> (maybe due to lack of background on Policy vs Policy<TResult>). From what I understand, IPolicyRegistry<Key, Policy> provides a mechanism of storing (any) policies (anywhere). The default implementation stores it in-memory (which is fine).

Are you suggesting to create another class PolicyRegistry<TResult> which can store Policy<TResult>? But then I will have two registry, one for Policy and another for Policy<TResult> (feels weird).
But then what happens if I have Policy<int> and Policy<string>? Do I now have PolicyRegistry<int> and PolicyRegistry<string> to store individual type of policy? (may be I am missing something here)

Just thinking out loud, what if we have another kind of Policy tomorrow, say Policy<TIn, TOut> (highly unlikely) so we will create another registry?

Ok so the requirement is that to register Policy<TResult> also, which is currently not possible due where Policy: Polly.Policy generic type constraint . Let's suppose we remove the constraint. Removing it effectively makes it a generic dictionary IPolicyRegistry<Key, Value>, allowing user to store anything.

Then the next challenge is that concrete implementation also depends upon Policy.

public class DefaultPolicyRegistry : IPolicyRegistry<string, Policy>

How about we create an empty interface IPolicy, derive both Policy and Policy<TResult> from it, and then implement DefaultPolicyRegistry : IPolicyRegistry<string, IPolicy> and re-introduce the where type constraint with IPolicy.
This should in general allow me to use same registry to store both Policy and Policy<TResult>. After retrieving, the user can then typecast it to whatever they want. (We can go a step further and change TryGetValue(string key, out Policy value) to TryGetValue<TPolicy>(string key, out TPolicy value) and typecast for them).

For the user, registry just stores policy(can be anywhere) and gives ability to retrieve them based on Key. Doesn't it make more sense that we have one registry which can store all kind of policies, and if I have the Key I can get whatever I stored.

Apologies if I misunderstood you and you meant something different (I had a long, tiring day).

PS: I did not create a new PR instead I just switched the target branch in this pull request to v5.0.7 (so the discussions are not split in multiple places). I hope it works for you.

[reisenberger]

@ankitbko Perfect to just re-target the PR to the new branch!

The evolution of Polly has led to Policy<TResult> does not extend Policy. Past #129 describes further. Also, non-generic policies can execute Actions returning void, while generic Policy<TResult>, can only execute Funcs returning TResult. So, unlike IEnumerable<T> : IEnumerable, we do not have Policy<TResult> : Policy.

I was also uncomfortable about multiple registries: one registry would probably be better, as you say. Creating an empty marker interface, as you suggest, could be a way to do this. I can think of one other approach. I will aim to spike out - or describe in more detail - shortly.

[reisenberger]

Huge thanks @ankitbko for all the great thinking towards PolicyRegistry!

Empty marker interface definitely a possible way to bridge between Policy<TResult> and Policy which are (necessarily) not in an inheritance hierarchy. Two thoughts followed from the empty interface:

(a) if the registry returns instances fulfilling empty IsPolicy (say), users would have to cast the received policy every time to the relevant Policy or Policy<TResult>. To avoid that, the Registry c/should provide helper getter overloads to reduce the burden, maybe something like:

public Policy IPolicyRegistry.Policy(TKey) // or Get(TKey)
public Policy<TResult> IPolicyRegistry.PolicyFor<TResult>(TKey) // or Get<TResult>(TKey)

(b) given an empty interface, users might soon ask why it is empty, and ask for populated interfaces that in fact provide the relevant .Execute(...) overloads they can use.

Which led me to consider whether:

(i) we can/could/should now introduce interfaces, say, IPolicy, IPolicy<TResult>, IPolicyAsync, IPolicyAsync<TResult>, defining the relevant available .Execute/AndCapture/Async(...) overloads. Alongside the possible empty marker interface IsPolicy.

(ii) whether either the Policy<TResult>/Policy split, or the sync/async split, can be reduced/mitigated.

Re (ii), I think not - intend to post later on this - but now is definitely the time to consider, before we introduce interfaces. If we go for interfaces now, the helper methods at (a) above also would return those rather than the concrete Policy<TResult>/Policy .

Thoughts on any aspect of this?

[ankitbko]

@reisenberger Very inline with my thoughts. Next steps could very well be populating the relevant interfaces.
We have a method TryGetValue in the registry. How about having a TryGetValue<TResult> extension method as the helper method? It would typecast and return the Policy.

[reisenberger]

Hi @ankitbko ! Here reisenberger/Polly/policy-registry-TResult-nointerfaces is a prototype branch (light amend from yours) providing a single policy registry class offering overloads for both Policy and Policy<TResult>, without using interfaces.

this[...] operators have been replaced by Get/Set methods. Add, Set, Get, and TryGetValue exist in overloads for both generic and non-generic policies.

Comments welcome!

We could go this way with PolicyRegistry if we don't introduce interfaces. (Not necessarily preferring this: just the first prototype which is ready.) Working separately towards a branch of Polly including a full set of interfaces (empty) IsPolicy and (populated) IPolicy, IPolicyAsync, IPolicy<TResult>, IPolicyAsync<TResult>. Then we can compare the two.

[reisenberger]

Hi @ankitbko ! Here is a Registry based on yours, but allowing us to pull policies out of the Registry correctly typed, based on generic methods. The generic methods become particularly useful because of adding interfaces.

This version again omits the this[...] operators, because the getter on that, registry[key], could never return policies of an immediately usable type: users would always have to cast first.

EDIT: Do you think it is better to exclude the this[...] operators? (because the getter can only return something that would always have to be cast). Or retain the this[...] operator (because people are used to it), and just use the doco to explain the existence of the more helpful .Get<TPolicy>(...) ?

Please let me know what you think! (particularly between the two options of this comment, and the previous one)

[ankitbko]

@reisenberger Sorry for delay. This is excellent. Interfaces looks amazing. Couple of thoughts on the Interface implementation of registry -

• There are two ways to add to registry Add<TPolicy>(string key, TPolicy policy) where TPolicy : IsPolicy and Set<TPolicy>(string key, TPolicy policy) where TPolicy : IsPolicy. Any differences between two or can we remove Set and just keep Add (or vice versa)?

• If using Interfaces, is there any need of making Add method generic? Alternative would be to just have Add(string key, IsPolicy policy).

• Since now we have interface, does it make sense to have dictionary value of type object in IDictionary<string, object> or should we make it IsPolicy like IDictionary<string, IsPolicy>?

• WithPolicyKey(String policyKey) method in ISyncPolicy has returns type of Policy (Line 22). Any particular reason of having reference of derived class in interface? (Similar for other sibling interfaces)

I am leaning more towards interfaces side. It leaves registry class clean (rather than having different get set for Policy and Policy). What do you think?

As for this[key], I don't see any benefit of having it (or not having it) as long as it is well documented.

But if we change IDictionary to store type IsPolicy, it may be useful if we add methods to IsPolicy rather than having it empty. Do you think it is possible to move common methods from ISyncPolicy and IAsyncSyncPolicy (and their generic counterpart) to IsPolicy?
If we are able to move common methods to IsPolicy (say Execute), people can use this[key] without having to explicitly typecast it. Does it make sense?

[reisenberger]

@ankitbko Huge thanks for your thoughtful feedback on the interface-based approach I overlaid on your PR!

Pushed changes in response to your comments, again to this branch: https://github.com/reisenberger/Polly/tree/feature/interfacesplusregistry

Responses to your comments:

Since now we have interface, does it make sense to have dictionary value of type object in IDictionary<string, object> or should we make it IsPolicy like IDictionary<string, IsPolicy>?

Agree. Done!

WithPolicyKey(String policyKey) method in ISyncPolicy has returns type of Policy (Line 22). Any particular reason of having reference of derived class in interface? (Similar for other sibling interfaces)

Great catch! Done!

There are two ways to add to registry Add(string key, TPolicy policy) where TPolicy : IsPolicy and Set(string key, TPolicy policy) where TPolicy : IsPolicy. Any differences between two or can we remove Set and just keep Add (or vice versa)?

The intention was that Set() was like this[], so Add() and Set() differed in that Add() would throw ArgumentException on duplicate key, while Set() would not. The very fact that you asked the q though, shows that sticking with API precedent probably would engender less confusion. I've therefore switched Set() back to this[].

The original reason for not using this[] was the thought that users might find it frustrating that registry[key] necessarily returns only an IsPolicy, which would require the further cast (as you observed), in order to be useful. I have addressed that now by giving explicit guidance right in the intellisense, highlighting the Get<TPolicy> alternative. And can ditto in online doco.

If using Interfaces, is there any need of making Add method generic? Alternative would be to just have Add(string key, IsPolicy policy).

Agree could reduce it to IsPolicy. Kept it generic just for symmetry with Get<>(). Encourages users into thinking about which policy type they are dealing with, for when they later Get<>() it back out of registry.

I am leaning more towards interfaces side. It leaves registry class clean (rather than having different get set for Policy and Policy). What do you think?

👍

But if we change IDictionary to store type IsPolicy, it may be useful if we add methods to IsPolicy rather than having it empty. Do you think it is possible to move common methods from ISyncPolicy and IAsyncSyncPolicy (and their generic counterpart) to IsPolicy?

Completely get the suggestion. I'm (starting out) genuinely ambivalent, per following discussion:

(a) The history is that the sync/async split in Polly was, long pre-AppvNext, implemented (unfortunately imo) as a runtime exception on the Policy class (instances of Policy are usable only for either sync or async, but enforced only at runtime rather than compile time), instead of defining separate Policy and PolicyAsync classes at that point. That historical decision means that users get both sync .Execute(...) and async .ExecuteAsync(...) overloads available (eg in intellisense) on every concrete Policy instance, even though only half the overloads are valid (on any one instance), and the other half will throw.

My introducing SyncPolicy and AsyncPolicy as interfaces then is an attempt to move users away from this - to separate out the sync and async functionality currently joint on Policy, by that well-known interface segregation principle. If users get into the habit of using ISyncPolicy/AsyncPolicy , they will get only the set of execution overloads that apply 😁

As SyncPolicy and AsyncPolicy are interface segregation, there are no common overloads to move on to IsPolicy. We essentially have two options:

(b-i) Keep IsPolicy empty, a marker-interface only (and encourage users towards SyncPolicy and AsyncPolicy)
(b-ii) Make IsPolicy a union of SyncPolicy and AsyncPolicy
(and similar for generic versions)

Having started out ambivalent, I'm leaning to (b-i). Doing (b-ii) just places us back to the "false promise" that a single Policy instance can be used for both sync or async, when in fact it can only be used for one or other. (b-i) at least begins to push the solution architecture in the right direction.

For further background, this recent blog post sets out why I think we're stuck (without a major change of syntax) with separate Policy instances for sync and async executions, in Polly.

Sorry for that long explanation. The sync/async split in Polly is an unhappy legacy part of the Polly architecture I've been tussling with, hence the long explanation - and interested in people's views.

😅

[reisenberger]

Hi @ankitbko Per discussion on slack, I have:

• drafted some wiki documentation ( PolicyRegistry; Polly and interfaces ).
• also readme doco in my working branch on this feature.

Anything you think we should add/change?

I'll aim to sort out the PRs between code branches in next few days

[reisenberger]

@ankitbko Merged this to the App-vNext:v5.0.7registry branch. Decided it would be easier to merge there, then add my interfaces on top.

Thank you for your great contribution to this feature! 💯