Unnecessary type guard in QueryableExtensions.cs?

[heinrich-ulbricht]

Category

• Bug - but not really

Describe the bug

The issue might be a copy and paste oversight in QueryableExtensions.cs. I'm not sure where to put this, so I'm opening a ticket.

Here's the method in question:

pnpcore/src/sdk/PnP.Core/QueryModel/Linq/QueryableExtensions.cs

Lines 600 to 614 in d2adbd6

	public static IEnumerable<TSource> AsRequested<TSource>(
	this IDataModelCollection<TSource> source)
	{
	if (source == null)
	{
	throw new ArgumentNullException(nameof(source));
	}
	if (source is BaseQueryableDataModelCollection<TSource> enumerable)
	{
	return (IEnumerable<TSource>)enumerable.RequestedItems;
	}
	throw new InvalidOperationException(PnPCoreResources.Exception_InvalidOperation_NotAsyncQueryableSource);
	}

Is this type guard here really needed?

pnpcore/src/sdk/PnP.Core/QueryModel/Linq/QueryableExtensions.cs

Lines 608 to 611 in d2adbd6

	if (source is BaseQueryableDataModelCollection<TSource> enumerable)
	{
	return (IEnumerable<TSource>)enumerable.RequestedItems;
	}

Shouldn't this be sufficient instead (which compiles fine):

return (IEnumerable<TSource>)source.RequestedItems;

Also, the exception thrown on a failed cast is strange. It says Exception_InvalidOperation_NotAsyncQueryableSource - but there is no async queryable involved in this method. There are other methods higher up in the file where a lot of async stuff is going on. But not this one. Thus my suspicion that this might be a copy and paste thing.

So I'm wondering if I can replace this:

if (source is BaseQueryableDataModelCollection<TSource> enumerable)
{
    return (IEnumerable<TSource>)enumerable.RequestedItems;
}

with this:

return (IEnumerable<TSource>)source.RequestedItems;

But I am hesitant since this is deep in the core and hasn't been touched for years. And there might be expectations further down the road that need this type check, although this is not visible here.

Additional context

Why am I bothering? I'm currently trying to mock enough PnP types via their interfaces to accomplish something. But those hard type guards (here the check for BaseQueryableDataModelCollection) sabotage this. There are other places like this and they might make my approach fail. But I have not given up, yet.

[wizofaus]

Mocking PnP types is not straightforward - at least not using a library like Moq that can only do interfaces. I've already raised a ticket for that: #1373

[heinrich-ulbricht]

@wizofaus Yes, I can confirm that. I have something working, but it is not pretty. Appreciate the ticket - seems like we had the same pain at the same time :D

[jansenbe]

@heinrich-ulbricht : did you test above approach or it's only from "observation"?

[heinrich-ulbricht]

I implemented that and for me it's running fine.

[jansenbe]

@heinrich-ulbricht : thanks for the test confirmation. I've committed this change, closing this now.