Discussion for announcement: EF Core 2.0: Warning as error for ignored calls to Include()

[divega]

This is the discussion thread for aspnet/Announcements#256.

[Mertsch]

I like the change, hunting down warnings in the log (like "this is run locally, not in SQL") is pretty tedious. So I'd rather have a clear break point and fix the issue immediately.

[wizarrc]

Love this. Anything that promotes correctness. I for one did not know that navigation properties in the filter clause would auto include. As long as the error message is easy enough to understand, and it should be easy enough to correct (just remove the extra include). A well worth breaking change. Any chance of getting design time query checking?

[divega]

I for one did not know that navigation properties in the filter clause would auto include...

Technically, this does not "auto include" e.g. the object referenced in the filter just doesn't need to get materialized, while Include() is all about asking that extra objects get materialized.

Any chance of getting design time query checking?

We have talked about this sometimes but haven't done anything serious with it. Probably a good place to use Roslyn Analyzers. Would you like to create an issue for this and describe what you would want it to look like? (or check if we already have one).

[pkellner]

Agreed. My only concern is if this is a breaking change to code that might update from v1 without errors. Otherwise, include errors can have huge perf implications and finding them later is very frustrating/expensive.

[Reonekot]

Agree with the previous comments and +1 for this breaking change.

[wizarrc]

@divega Thanks for the clarification. It makes sense that it can be filtered remotely but not projected aka materialized.

We have talked about this sometimes but haven't done anything serious with it. Probably a good place to use Roslyn Analyzers. Would you like to create an issue for this and describe what you would want it to look like? (or check if we already have one).

I think a Roslyn Analyzer is exactly the right technology for a query analyzer. Some quick thoughts would be to include query time estimations during construction so that you can get a feel for how long your query will take in rough orders of magnitude and with some sort of live preview would be nice. But that's off the top of my head. I don't have any real concrete specifics. I'll take a look and see if there is anything already out there.

My only concern is if this is a breaking change to code that might update from v1 without errors.

1. For people upgrading from version 1, might be a good idea to suggest that they use IncludeIgnoredWarning in the migration docs if they are running into issues, even go as far at to suggest enable it for production at first, and during the dev environment, go to the default setting.

2. Offer a way to toggle this programmatically such as an extension method IgnoreIncludeError() per query. This might be a pain point for those who generate EF queries dynamically and can't statically verify, but eventually want better runtime assertion that they are not wasting performance.

[MisterFantastic]

Please let know how many levels of child's will be considered like this ?
Parent.Where(e=>e.Child1.Where(d=>d.Prop1=="Test")).Select(o=>o.Child1)

[divega]

@MisterFantastic can you clarify what you mean? I.e. considered for what? If you are asking how many levels of navigation can be used in a filter without having to use Include(), the answer is that it is not limited to a number of levels. Include() was never meant to be a way to bring objects into memory to make complex filters work. Filters can either be fully translated to SQL or in some cases part of them may need to be evaluated in memory (e.g. if we can't find a translation), but even in the latter case Include() would not help.

Include() is only about telling EF Core what additional navigations to traverse and bring into memory alongside the final result of the query.

[MisterFantastic]

@divega that answers my question. I agree part of the filters may have to done in memory.

[John0King]

How you guys do paging ?
For me is:

var query = _db.DbSet<Book>().Include(n=>n.Author);
if(....)
{
    query = query.Where(xxxx)  // filter
}
var pageSize = 8;
var currentPage = Sharp.ParsetInt(Request.Query["Page"]);// Sharp.ParseInt is a helper method 
var data = query.ToPagedList(currentPage, pageSize)

Inside ToPagedList, it automatice call .Count()

Now you tell me the good of linq is gone, and I shuold rewrite the linq for a .Count() operation because .Count() will ignore Author ?

[aghe]

This will throw error.

var query = _dataContext.FormResponses.Include(x => x.Form).AsNoTracking();
    if (!string.IsNullOrWhiteSpace(search))
        query = query.Where(x => x.Form.Name.Contains(search) || x.Response.Contains(search));
    return await query.ToPagedListAsync(index, size, sortby, sortdesc, search);

The method will look like this for paging:

public static async Task<PagedList<T>> ToPagedListAsync<T>(this IQueryable<T> source, int index, int size, string sortby = null, bool sortdesc = false, string search = null)
{
    source = source.OrderBy(sortby, sortdesc);

    int total = 0;
    if (source != null)
        total = await source.CountAsync().ConfigureAwait(false);

    if (index <= 0) index = 0;
    if (size <= 0) size = 10;

    PagedList<T> pagedList = new PagedList<T>();
    pagedList.TotalCount = total;
    pagedList.TotalPages = total / size;

    if (total % size > 0)
        pagedList.TotalPages++;

    if (index > pagedList.TotalPages - 1)
        index = 0;

    pagedList.PageSize = size;
    pagedList.PageIndex = index;

    if (source != null && total > 0)
    {
        List<T> pageItems = await source.Skip(index * size).Take(size).ToListAsync().ConfigureAwait(false);
        pagedList.AddRange(pageItems);
    }

    pagedList.Search = search;
    pagedList.SortBy = sortby;
    pagedList.SortDesc = sortdesc;

    return pagedList;
}

And think about other methods that will do some select for values on the same query where the include is not needed!?

[aghe]

Why consume processor, memory, IO to analyze queries just to inform/throw error!?

[thiennn]

How about

var pids = context.Orders
    .Include(o => o.Product)
    .Where(o => o.Product.Name == "Baked Beans")
    .Select(o => new { OrderId = o.Id, ProductName = o.Product.Name })
    .ToList();

If I include Product => warning as error. But if I don't the o.Product in select is NULL.
What can I do?

Updated
@divega I have figured out. In my project, I wrote an extension to do pagination, and it take an object type System.Func<TModel, TResult> as the selector. I worked fine. But after upgrading to EF Core 2.0. I got the error described above. After some investigating I changed the type System.Func<TModel, TResult>to System.Linq.Expressions.Expression<Func<TModel, TResult>>. And it work perfectly.

[divega]

But if I don't the o.Product in select is NULL

@thiennn that sounds like a bug. Could you please create a new issue with a full repro, stack trace, detailing what version you are using, etc? Thanks!

[ajcvickers]

@aghe This case (where a single expression tree is built and then sometimes used in ways that the Include is needed and sometimes used in ways where it is not) is the one case we discussed as it being legitimate to have the Include in the expression tree even though it sometimes isn't used. For someone who is doing this intentionally and fully understands what is happening, then it would be the perfect case to configure this as a warning or even set the warning to be ignored. Whether or not we keep this behavior for 2.0 RTM or revert it will depend somewhat on our best estimate of the balance between people doing this verses just thinking the include is needed when it isn't.

[divega]

Offer a way to toggle this programmatically such as an extension method IgnoreIncludeError() per query.

@wizarrc agreed that allowing the behavior to be overridden at a query level could help. In particular, general purpose methods like the one presented by @aghe could use this feature to avoid the error without disabling it globally.

[wizarrc]

@divega I looked back at some of my old projects, and having a query level ignore could help a lot.

[John0King]

@divega what about a extension mehod called .MustInclude(n=>n.prop) ? it can provide the error if you actually need without break current application written with EF core 1.0

[wizarrc]

@John0King I think the point of the change is to cause awareness. If you want to upgrade and just to forget about improving your code performance, then you can globally opt out during the upgrade. I feel changing the default is the right move going forward, especially with new projects.

[sky-code]

In my opinion default include behaviour must not throw any exception but emit log warning.
From #9064 my vote is for 4 option, make by default not throw any exception but can be reconfigured and must be configured default template for stronger semantics and throws if ignored. Then anybody who don't want this semantic, can comment or delete one line in startup.cs file.

[ajcvickers]

@sky-code Can you clarify what you mean by "default include behavior"? Do you mean that the default behavior when include is used incorrectly should be to warn and not to throw? If so, can you explain why you don't want it to throw when used incorrectly?

[John0King]

@wizarrc Throw a error can improving performance ? I don't think so. Since this is configurable the framework need to check it anyway. (please let me know if I'm wrong)

I think linq should be smart, save our workload and do what we want , because we trade it with SQL's powerful , flexible and performance.
on the other hand:

1. Do you really not know your navigation property is ignored ?
2. This behaviour (throw error) will break any automatic operation (such like OData's paging or a ResultFilter for support auto pageing api) if you do want to include some navagation property

[sky-code]

@ajcvickers default include behavior" in this case is warning (2 option from #9064)
"why you don't want it to throw when used incorrectly" because it's not about correctness, it's all about optimization and semantic of this problem is the same as with N+1 with lazy navigation properties, sometimes you just don't care about performance and optimization, as example code to be executed once per month for some job written by junior with N+1 and unused include and this still ok because job is done (sometimes bad quality is ok) but if it will throw exception much more time will be required to obtain same result.
I am not "don't want it to throw" but this must be configured by people who want this behavior and will be good to have this enabled by default in asp.net templates with some comment about available options.

[wizarrc]

@John0King Obviously throwing an error itself does not improve performance. What I like about it is during development time, it helps you optimize your query to be more lean, which in turn is faster than fetching data that is not used in your projection.

This behaviour (throw error) will break any automatic operation (such like OData's paging or a ResultFilter for support auto pageing api) if you do want to include some navagation property

I don't understand how this would break paging. I thought this is about throwing errors only when fetching (include) data that is not being materialized. If it does, I think something bigger is seriously wrong.

[wizarrc]

@ajcvickers I think a better approach might be to have a commented out section with the config needed to ignore throwing error in new project templates (maybe even migrated projects if possible). Along with the commented out config, text explaining the option along with text explaining how to opt out on a per query basis as I proposed instead. Documentation everywhere might help people make the right choice for them. I still feel opt-in ignore errors is the right model.

[ajcvickers]

Thanks for all the comments.

Just to clarify something, when the Include is ignored and the warning/error is generated, it means that the SQL query sent to the database would be exactly the same as if the LINQ query did not have the calls to Include. In other words, the application behaves the same way with or without the Include being there. So there is no efficiency gain from not having the Include. (Other than parsing the Include call, but that's negligible.)

The reason for having the warning, and for turning it into an error, is to try to educate people about what Include does. We see so many bugs filed where people get confused because "the Include isn't working" when it is in fact never used. This was also a problem with the old stack so we have been discussing for years how to educate people on this. Maybe this is going a step too far--which is why the feedback is awesome--but we're also open to any other ideas people have for education on Include.

[ajcvickers]

After much discussion, we have decided to revert this back to a pure warning for 2.0--see #9064 for more details.

Please continue to give us any feedback/ideas on this issue since we will be continuing to think about this going forward and appreciate any input people have.

[sky-code]

You have one more option not listed in #9064
revert back to a pure warning and add into default asp.net templates commented code for enabling exception throwing with link to article with details about what is this.
for example:

            services.AddDbContext<ApplicationDbContext>(options =>
            {
            // summary about it and link to details http://docs.asp.net/123
            //  options.ConfigureWarnings(w => w.Log(CoreEventId.IncludeIgnoredThrowException));
            });

you spoke the main reason for it is education, than people can open the link in comment and read how to work with include feature

[divega]

@sky-code thanks. That would be a variation of option (4) in #9064. In general the bar for adding something to the template is high, as we don't want to make the signal-to-noise ratio in the template worse. Commented code almost by definition cannot meet that bar (i.e. if it is important enough to add it, why is it commented out?). So we are more likely going to look at other ways to "educate" about Include() before we resort to something like this.

[sky-code]

@divega in that case don't comment code, but configure same value as default, with comment link to details?

            services.AddDbContext<ApplicationDbContext>(options =>
            {
               // summary about it and link to details http://docs.asp.net/123
               options.ConfigureWarnings(w => w.Log(CoreEventId.IncludeIgnoredWarning));
            });

main question is to place the comment to details or not and where to do it?