Be open to extension

[tomkerkhove]

We provide a lot of great stuff, but when you want to extend or tweak something to your needs it becomes harder because we are not always open to extension.

We should ensure that we allow people to derive and extend things by using protected virtual instead of private, for example, or use public virtual.

[stijnmoreels]

I agree, though there may be places were it's recommended to privatize stuff. Maybe this is a bit subjective. Useful places that could be extended, For sure!

[fgheysels]

I agree with @stijnmoreels : you don't want to change all private's to protected virtual by default. Same is true for public members: you don't want to change them all to virtual by default. Moreover, I used to never specify public members as virtual. If I want to allow a consumer to change the behavior of a method, I mostly design it like this:

public void DoSomething( Foo bar )
{
    if( bar == null )
    {
        throw new ArgumentNullException(nameof(bar));
    }

   DoSomethingCore(bar);
}

protected virtual void DoSomething(Foo bar)
{
    ... 
}

see also this article for design guidelines on using virtual.

Do we have specific cases where we're lacking extensibility ?

.edit:

Just a thought, but you could provide extensibility-points in certain methods, for instance:

public async Task<Secret>  GetSecretAsync( string name )
{
    await OnBeforeGetSecretAsync(name);

    // Retrieve secret from secretstore ...
   ...

   await OnAfterGetSecretAsync(name, retrievedSecret);
}

protected virtual Task OnBeforeGetSecretAsync(string name) {}

protected virtual Task OnAfterGetSecretAsync(string name, Secret secret) {}

(ps: should this be a discussion ? )

[tomkerkhove]

It's not a discussion because it's a must :D Today this is a pain.

I agree that we shouldn't do it for all privates, that was not really my intend indeed. However, I would personally keep it simple and make the protected methods virtual itself rather than different sub pieces since you can still manage that through base.MethodName, no?

Otherwise it becomes too complex imo.

[fgheysels]

My intention was rather: should it be a 'discussion' so that we can agree on how to do it. :)

When looking at the KeyVaultCachedSecretProvider class, you'll see that this class has one method which is public virtual: StoreSecretAsync. In this specific case, I actually wonder why this method is virtual. As we're in the KeyVaultCachedSecretProvider, you'll certainly want to store the secret in KeyVault, so why would anyone really want to override the functionality ?
If you want to tweak something, it'll probably be some additional logic that must be executed before you store the secret, or after you've stored the secret. Hence my suggestion to have something like this:

public async Task StoreSecretAsync( ... )
{
    await OnBeforeStoreSecretAsync(...);

    await _secretProvider.StoreSecretAsync( ... );
   MemoryCache.Set(... );

    await OnAfterStoreSecretAsync(...);
}

protected virtual Task OnBeforeStoreSecretAsync() {}

Instead of providing protected virtual methods as extension points, you could also implement this via events. However, that might be a bit of a pain when working with async code.

Anyhow, I think it's to blunt to just say that all protected methods must be virtual. Take a look at the KeyVaultSecretProvider class. One protected method: GetSecretClient which -imho- doesn't make sense to override. Two static protected methods: You can't override static, and IMHO it also doesn't make sense to override them:

• ThrottleTooManyRequestsAsync : You can allow users to change the throttling behavior by havving a settable ThrottlingPolicy property f.i.
• (The other protected static is the same method with a different signature, so that would mean that if you override the first one, you must also override the 2nd one. Having an adaptable 'policy' which the consumer can specify, makes more sense imo.

Edit:

Although I like the OnBefore... , OnAfter... pattern as it provides some controlled extension points, I think it might not be a good idea to implement it in Arcus, as I see that there are already public virtual methods defined.
Should we remove the virtual keyword from those methods, we'll introduce a breaking change (unless this is something that hasn't been released yet). By having those public methods declared as virtual, you actually have the same behavior as a consumer can do this:

public override StoreSecretAsync( .... )
{
    // Do something before storing the secret
   Foo();

   await base.StoreSecretAsync( ... );

   // Do something after the secret has been stored
  Bar();
}

But ... this construct allows a consumer to completely change the implementation of this method as well. And that's why I'm actually not a fan of public virtual methods: they're not conform to the adagio 'be open for extension but closed for change': It is allowed to change the complete implementation, so it's open for extension and open for change. Therefore, I much more like the OnBefore../OnAfter... pattern as this complies with 'be open for extension but closed for change'.

[stijnmoreels]

The KeyVaultCachedSecretProvider is maybe not a good example to use here. The reason btw the store method is virtual, is because when using KeyVault with caching, there's some extra methods with the boolean ignoreCache which are only available in the cached variantes. But again, I think not a good example to use here.

The KeyVaultSecretProvider itself or even this open PR https://github.com/arcus-azure/arcus.security/pull/282 may be better ones. I think the goal here is to make sure that virtualize public members (coming from interface or otherwise) and provide access (by making it protected) to otherwise private members. In the https://github.com/arcus-azure/arcus.security/pull/282, you'll see the consumer having access to the user-configurable information and the actual secret-retrieval functionality. Making it open for extension (public virtual from interface) but closed to modification (private members with the hard-lifting becomes protected but not virtual).

[stijnmoreels]

As stated by @fgheysels here https://github.com/arcus-azure/arcus.security/pull/282#discussion_r671077238 : we may not be ready to know what the consumers want. On the other hand, if we continue the approach we're using, they are free to override the public methods and set their own limitations instead of us trying to restrict something we can't predict.