Substituted method call returns a null in Azure.Storage.Blobs.BlobContainerClient

[siewers]

This is a weird one and very specific to the Azure.Storage.Blobs.BlobContainerClient.

When using version 12.6.0, the following works:

// Using Azure.Storage.Blobs version 12.6.0
interface IClientWrapper
{
    public BlobContainerClient GetBlobContainerClient();
}

var clientWrapper = Substitute.For<IClientWrapper>();
clientWrapper.GetBlobContainerClient().CreateIfNotExists();
clientWrapper.Received(1).GetBlobContainerClient().CreateIfNotExists(); // This one works on 12.6.0

When I update Azure.Storage.Blobs to version 12.7.0, the substituted method call throws a NullReferenceException.

// Using Azure.Storage.Blobs version 12.7.0
interface IClientWrapper
{
    public BlobContainerClient GetBlobContainerClient();
}

var clientWrapper = Substitute.For<IClientWrapper>();
clientWrapper.GetBlobContainerClient().CreateIfNotExists(); // This throws a NullReferenceException (GetBlobContainerClient returns null)

The weird part is that this works:

var clientSubstitute = Substitute.for<BlobContainerClient>();
clientSubstitute.CreateIfNotExists();
clientSubstitute.Received(1).CreateIfNotExists();

So NSubstitute is perfectly capable of substituting the type, but no longer able to do it with the method call 🤯 I can't understand what or how this behavior can have changed, but it's really strange.

Our workaround is of course just to substitute the method call as well, like so:

interface IClientWrapper
{
    public BlobContainerClient GetBlobContainerClient();
}

var clientWrapper = Substitute.For<IClientWrapper>();
var blobContainerSubstitute = Substitute.For<BlobContainerClient>();
clientWrapper.GetBlobContainerClient().Returns(blobContainerSubstitute);
// other assertions...
clientWrapper.Received(1).GetBlobContainerClient().CreateIfNotExists();

What could have caused this change and/or is there something I'm doing wrong?

[dtchepak]

Hi @siewers ,

Sorry for the long delay in replying.

I've been having a look through the source repo and I'm not sure what change to BlobContainer has caused this. (I think it is BlobContainer, not BlobContainerClient causing the issue?)

NSubstitute will only automatically substitute for classes that have all public methods and properties defined as virtual or abstract and with a default, parameterless constructor defined as public or protected (as described here), so I am guessing a change has been made so that this no longer holds for this type.

[siewers]

Sorry about that, I mistyped the return type. I've updated the sample. It is in fact the BlobContainerClient that is returned, which doesn't seem to have changed in a long time :/ In the source code (https://github.com/Azure/azure-sdk-for-net/blob/99d4b8633be422fd1236afc704f7c5e22a783e43/sdk/storage/Azure.Storage.Blobs/src/BlobContainerClient.cs#L174) there is also a protected constructor, which should be the one used for mocking purposes.

I can't understand why it won't work.

[siewers]

I should probably also remind you that I can perfectly fine substitute the BlobContainerClient class and invoke methods on that substitute. It's only when the BlobContainerClient is used as a return type for a method on an interface it fails.

[dtchepak]

It might be the addition of the non-virtual CanGenerateSasUri that has changed this behaviour.

It should be fine to manually substitute for the type (be careful not to try to stub the non-virtual member; NSubstitute.Analyzers is great to help avoid this). NSub just won't automatically substitute for it to avoid people accidentally running real code in their tests.

[siewers]

But if I can write Substitute.for<BlobContainerClient>() which works, but a substituted interface with a method that returns the same type fails. I find it a bit strange that the addition of a non-virtual property would break this behavior.

[dtchepak]

I find it a bit strange that the addition of a non-virtual property would break this behavior.

I understand. It's a trade-off we had to make -- we decided on erring on the side of safety. If a non-virtual member was added that randomly deleted files and got called on construction, we didn't want to unexpected execute that code by auto-created a sub of that type. If you write Substitute.For<BlobContainerClient>() then you are opt-ing in; you are acknowledging that you are creating this class knowing that non-virtual members will execute.

Bit of an aside, but for me this also ties in to the idea of avoiding mocking types we don't own. I know it's not always feasible, but might be worth considering as I feel it has some strong advantages.

[siewers]

Okay, I get it. That makes sense. It's of course not possible for you to know or detect these kinds of changes, but could it be possible to maybe add some sort of diagnostics that can reveal such issues? In this case it took us quite some time to figure out and rewrite our tests.

It's of course an annoying addition by the developers of the Azure library, and they should probably reconsidered whether this should have been added in the first place 🙂

[dtchepak]

@tpodolak I'm guessing Roslyn analyzers just hook into compilation and not on package update?

@siewers:

they should probably reconsidered whether this should have been added in the first place 🙂

I understand this was frustrating to get bitten by this. It's really NSub's fault for making this confusing (but, as discussed, for what we feel is a good reason of erring for safety). This generally isn't a breaking change for a library to make, and it is good to prioritise library design of those that will be used in production over a test library. For a test library like NSub, if you find something like this that suddenly returns null, then you'll get alerted by your test suite straight away.

In terms of avoiding problems like this, I recommend sticking to mocking interfaces as much as possible, and considering using patterns like facade to avoid mocking types that we don't control.

[tpodolak]

Hi @dtchepak Sorry for the long delay in replying. I must have missed GitHub notification.

@tpodolak I'm guessing Roslyn analyzers just hook into compilation and not on package update?

Yes, analyzers are hooked into compilation, however package update affects compilation as well. So if we had analyzer for https://nsubstitute.github.io/help/auto-and-recursive-mocks/, @siewers case would be detected on package upgrade.

It's of course not possible for you to know or detect these kinds of changes, but could it be possible to maybe add some sort of diagnostics that can reveal such issues?

Writing analyzer for that particular case will not be easy. Analyzer itself also will be perf expensive, as we would need to analyze quite a lot of context around actual method call. I would refrain from adding diagnostic for this case(at least for now), unless @dtchepak you can see some clever trick/NSub usage pattern to make analysis not that expensive(as at the moment, IMO we would need to check if given method was not manually substituted in order to produce diagnostic)