IQueryable - powerful but sometimes dangerous?

[s-KaiNet]

The problem I would like to discuss here is the power (and the danger) of IQueryable interface used for almost all *Collection model objects.

The power

The power is very straightforward. You can use LINQ to dynamically filter, iterate or fetch SharePoint data. All your LINQ will be translated to the corresponding REST API queries:

// REST API: _api/web/lists?$filter=substringof('s',Title)
var lists = context.Web.Lists.Where(l => l.Title.Contains("s")); 

// REST API: _api/web/lists?$filter=Id+eq+(guid'a141bb4b-c2ec-4465-9ff5-9401afdcef55')&$top=1
var list = context.Web.Lists.FirstOrDefault(l => l.Id == new Guid("a141bb4b-c2ec-4465-9ff5-9401afdcef55"));

// REST API: _api/web/lists?$top=100 
var lists = context.Web.Lists.ToList();

//etc

The danger

If you don't fully understand how IQueryable works under the hood, you might have performance problems, because in some cases you might experience additional (totally unnecessary) server roundtrips. I saw quite a lot of such problems many years ago when working with old good (or bad?) LINQ to SQL.

Let me explain the problem on the real PnP SDK cases (I use Lists, but actually any *Collection has the issue):

// get lists with some filter, looks good
var lists = context.Web.Lists.Where(l => l.Title.Contains("s"));

// now lets iterate over results using "for" cycle:
for (int i = 0; i < lists.ToList().Count; i++) //  oops! the issue here - on every cycle we send a server request to get lists
{
}

Or:

// lets load lists, looks good
await context.Web.LoadAsync(w => w.Lists);

// lists are loaded, let iterate over them
foreach (var list in context.Web.Lists) // oops! the issue here - additional HTTP request to get all lists
{
}

// or even worse
for(var i = 0; i < count; i++)
{
	foreach (var list in context.Web.Lists) // oops! here we have "count" of additional server roundtrips to get lists
	{
	}
}

It happens because LINQ queries are not fired when they are declared, but when they are enumerated over (inside foreach cycle or when calling ToList()).

You can also take a look at the code sample from one of the latest issues:

To my mind, such a problem will be very common.
These are simple and easy to track examples, but in the complicated methods chains, it might be a challenge to find out where the problem is.

From my POV the biggest problem is, that it's not clear how it works, especially for those, who only start exploring the library, or even for experienced developers. As a result, a bigger and more complicated codebase might suffer from performance problems in the future. As a result, a developer might be disappointed, why it's slow If I do everything right? Will they be disappointed with the library then?

How to solve "the danger"

Well, we don't have an easy answer here. In my opinion, there are two options here, one is just a docs update, the other one is more drastic with code changes.

1. Update documentation (somewhere in the basics section) to clearly indicate how it works. It was a problem for me quite a while until I understood how and when to use AsRequested(). Also, provide examples where multiple unnecessary requests might be sent and how to avoid such situations.
2. Alternatively, change the code, especially GetEnumerator so that if the collection was requested previously, it does not send additional HTTP requests, but uses loaded items instead. If the collection was not requested, it either throws an exception (like in CSOM - "Collection should be initialized or explicitly requested") or iterate over empty results.

In this case in the below code will be:

// lets load lists, looks good
await context.Web.LoadAsync(w => w.Lists);

// lists are loaded, let iterate over them
foreach (var list in context.Web.Lists) // <-- use the result of LoadAsync(w => w.Lists) here
// so under the hood it will be the same as "context.Web.Lists.AsRequested()"
{
}

Will be happy to see other opinions on this topic.

[PaoloPia]

Hi @s-KaiNet,
Thanks for sharing your POV. Let me try to explain the reasoning we applied behind the scenes when we designed the *Collections and the IQueryable support in PnP Core SDK.

At the very beginning, in early alphas/betas, the PnP Core SDK was not using the LINQ query model. We changed our minds because we were experiencing usability issues related to when and how to load data in memory, considering that we have a PnPContext instance with a hierarchy of in-memory objects (context.Web.Lists for example). Let me try to explain! Without a LINQ query provider, what should we do when a developer executes a request for a list of objects (like lists, for example) filtered by specific criteria? Should we execute a new query and load the result in the PnPContext? Or should we rely on the in-memory data only? And what if the list of objects got updated in the meantime on the SharePoint side? And what if the in-memory list of objects got updated in memory? How can we synchronize in-memory data with new data coming from SharePoint? How can a developer tell to the PnP Core SDK when to load fresh data and when to rely on in-memory data only? And there could be many more questions like these.

That's why, in the last beta release, we decided to introduce support for IQueryable and to create our own LINQ query provider. When you work with IQueryable, as you said, you execute the query every single time you enumerate it, This is a well-known pattern for a LINQ query provider. In fact, the object you are browsing (with a for-each for example) is not an actual collection, but just a query expression that will be "visited" by the LINQ provider and that will generate the actual query upon request. That's the intended behavior of an IQueryable collection. And the benefits of this behavior are tangible in many scenarios:

• you define the query once and you reuse it multiple times, you will always get updated results
• you don't care about Graph or SPO REST, you simply write the query and the LINQ provider will create the right query for you
• if you query a huge list of objects, you will get transparent paging (what we call "implicit paging" in PnP Core SDK docs) and optimized queries
• etc.

So, whenever you will query data with the LINQ model, you will always get back a fresh new list of objects and the *Collection object will simply be a proxy to the actual LINQ query. If you want to have an in-memory list of objects, you convert it into an IEnumerable and use it. That's exactly the purpose of the AsRequested() method. We wanted to make it possible for developers to reuse the same query result multiple times, to avoid useless roundtrips if not needed. Basically, the AsRequested() gives you what is already in memory, no matter what. Plus, when using AsRequested() you can apply in-memory filters with LINQ to Objects targeting the objects that are already in memory. This becomes useful when you need to apply query rules that are not supported by the custom LINQ query provider that we implemented in PnP Core SDK.

Now, we can for sure update the docs, and we should, definitely. Thanks for the feedback. What we have right now is described here, and for sure we should improve and highlight it.

Should we change the library implementation? I don't know ... personally I would say "no" because we've been there before (early alphas/betas) and we moved away. But I'm sincerely open to feedback and willing to discuss it with the whole community. BTW, changing the library now, after many months, would be a huge breaking change and the impact on already written code could be dramatic. At the same time, changing the library means changing the core part of the query engine, which has a huge impact on the whole query model, too.

If you want me to be more specific, or more detailed ... we can talk more. We are a community and we are to help and find the best solution for the whole community. Thanks.

[jansenbe]

Thanks for the detailed response @PaoloPia! I'm also not in favor of changing this behavior as it will be a large breaking change...but I fully agree with @s-KaiNet that we need to do a better job explaining how things work.

Possible work items I see are:

• Add some "getting started" tutorials, maybe with some backing videos, that explain the 'basics' which should also cover this
• Clarify the importance of AsRequested in the current docs
• Add a new page showing how PnP Core SDK relates/differs from CSOM to help folks that move over from CSOM to PnP Core SDK
• Include a line mentioning AsRequested in the IQueryable classes code comments, so developers typing context.Web.Lists... would see information about AsRequested

I'm open for feedback on these work items and the concept in general.

Thx!

[s-KaiNet]

Thank you @PaoloPia for the detailed information on the reasoning behind the IQueryable implementation!
I raised that question mostly because I saw quite a lot of developers, who don't fully understand IQueryable concepts, which leads to performance issues in code. This concept is very powerful and that's a really cool thing that we have it in the PnP SDK library. It adds a lot of flexibility and convenience. The only thing, which is missing in my pov is the transparency on how it works. Thus I suggested solving it either through the more detailed docs (since IQueryable concept is very important and also related to the performance, we should explain it better to avoid common mistakes from developers) or the code change. These common mistakes are implicit iterations multiple times over IQueryable (adds extra server roundtrips) or using it in CSOM-like style, which, well, also leads to the multiple enumerations of IQueryable (in some cases I highlighted above).

I expected that the code change solution is not that good, since most likely it will add breaking changes and possible regression bugs. Let's then stick with the suggestion from @jansenbe and create a few work items to clarify some usage scenarios/patterns. I do like a page on the difference between CSOM and PnP SDK, because I found myself tempted to apply CSOM code style to PnP SDK, but it doesn't always work well. I also do like the idea of improving current docs regarding AsRequested and IQueryable in general. That's something I can work on, since in a few last weeks I tried the library from the very beginning and faced with some common problems.

[jansenbe]

I've added these issues for now:

• Document the similarities/differences between CSOM and PnP Core SDK #300 : Document the similarities/differences between CSOM and PnP Core SDK
• Clarify the importance of AsRequested() in the current docs #638: Clarify the importance of AsRequested() in the current docs
• Include a line mentioning AsRequested in the IQueryable classes code comments #639: Include a line mentioning AsRequested in the IQueryable classes code comments

As I'm prepping for the European Collaboration Summit next week I'm adding new samples/tutorials...did not create an issue for that yet as I first want to setup a structure.