Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract projection overfetching when using intermediary projections #4995

Closed
1 task done
aradalvand opened this issue Apr 23, 2022 · 1 comment
Closed
1 task done

Comments

@aradalvand
Copy link
Contributor

aradalvand commented Apr 23, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

#3650 implements what was called "abstraction projections", which as far as I'm concerned, enables your GraphQL schema to follow your EF Core model if you're taking advantage of inheritance in EF Core.

So, given an app set up like the following:

Program.cs:

using Microsoft.EntityFrameworkCore;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddDbContext<AppDbContext>(o => o.UseNpgsql("[SOME-CONNECTION-STRING]"));

builder.Services
    .AddGraphQLServer()
    .AddQueryType(descriptor => {
        descriptor.Field("lessons")
            .UseProjection()
            .Resolve(ctx => {
                var db = ctx.Services.GetService<AppDbContext>();
                return db.Lessons;
            });
    })
    .AddProjections()
    .AddInterfaceType<Lesson>()
    .AddType<ArticleLesson>()
    .AddType<VideoLesson>();

var app = builder.Build();
app.MapGraphQL();
app.Run();

public abstract class Lesson
{
    public int Id { get; set; }
    public string Title { get; set; }
}

public class VideoLesson : Lesson
{
    public TimeSpan Duration { get; set; }
}

public class ArticleLesson : Lesson
{
    public string Content { get; set; }
}

public class AppDbContext : DbContext
{
    public AppDbContext(DbContextOptions<AppDbContext> options): base(options) {}

    public DbSet<Lesson> Lessons { get; set; }
    public DbSet<VideoLesson> VideoLessons { get; set; }
    public DbSet<ArticleLesson> ArticleLessons { get; set; }
}

Now, if a query like the following is executed against this GraphQL server:

{
    lessons {
        __typename
        ... on ArticleLesson {
            content
        }
        ... on VideoLesson {
            duration
        }
    }
}

The projection middleware now understands the inheritance side of things and thus generates the right projection expression, which is reflected in the resulting SQL that EF Core ultimately generates:

SELECT l."Discriminator" = 'VideoLesson', l."Duration", l."Discriminator" = 'ArticleLesson', l."Content"
FROM "Lessons" AS l

However, now imagine you have some sort of an "intermediary projection", here's the same setup above, with some additional DTO classes:

using Microsoft.EntityFrameworkCore;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddDbContext<AppDbContext>(o => o.UseNpgsql("[SOME-CONNECTION-STRING]"));

builder.Services
    .AddGraphQLServer()
    .AddQueryType(descriptor => {
        descriptor.Field("lessons")
            .UseProjection()
            .Resolve(ctx => {
                var db = ctx.Services.GetService<AppDbContext>();
                return db.Lessons.Select<Lesson, LessonDto>(l =>
                    l is ArticleLesson ? new ArticleLessonDto
                    {
                        Id = l.Id,
                        Title = l.Title,
                        Content = (l as ArticleLesson).Content,
                    } :
                    l is VideoLesson ? new VideoLessonDto
                    {
                        Id = l.Id,
                        Title = l.Title,
                        Duration = (l as VideoLesson).Duration,
                    }
                    : null
                );
            });
    })
    .AddProjections()
    .AddInterfaceType<LessonDto>()
    .AddType<ArticleLessonDto>()
    .AddType<VideoLessonDto>();

var app = builder.Build();
app.MapGraphQL();
app.Run();

public abstract class Lesson
{
    public int Id { get; set; }
    public string Title { get; set; }
}

public class VideoLesson : Lesson
{
    public TimeSpan Duration { get; set; }
}

public class ArticleLesson : Lesson
{
    public string Content { get; set; }
}

public class AppDbContext : DbContext
{
    public AppDbContext(DbContextOptions<AppDbContext> options): base(options) {}

    public DbSet<Lesson> Lessons { get; set; }
    public DbSet<VideoLesson> VideoLessons { get; set; }
    public DbSet<ArticleLesson> ArticleLessons { get; set; }
}

public abstract class LessonDto
{
    public int Id { get; set; }
    public string Title { get; set; }
}

public class VideoLessonDto : LessonDto
{
    public TimeSpan Duration { get; set; }
}

public class ArticleLessonDto : LessonDto
{
    public string Content { get; set; }
}

Now, if you execute the same GraphQL query mentioned above against this GraphQL server, the resulting SQL will be very different:

SELECT l."Discriminator" = 'ArticleLesson', l."Id", l."Title", l."Content", l."Discriminator" = 'VideoLesson', l."Duration"
FROM "Lessons" AS l

Notice how Title is being retrieved even though it was not requested in the GraphQL query. Actually, right now, ALL the columns are fetched from the data and the projection, therefore, is more or less useless.

Now, I think it's worth mentioning that this is fundamentally related to #2373, as both problems actually share the same underlying issue. That being the fact that EF doesn't really recognize that it should treat instances of LessonDto and LessonDto as equivalent when used in a projection expression on their own.

To get a better understanding of this, take this as an example:

var result = db.Lessons.Select(l => l is ArticleLesson ? "Article!" : "Something else!").ToList();

The generated SQL will be:

SELECT CASE
      WHEN l."Discriminator" = 'ArticleLesson' THEN 'Article!'
      ELSE 'Something else!'
END
FROM "Lessons" AS l

As expected.
But if you do:

var query = db.Lessons.Select<Lesson, LessonDto>(l =>
    l is ArticleLesson ? new ArticleLessonDto
    {
        Id = l.Id,
        Title = l.Title,
        Content = (l as ArticleLesson).Content,
    } :

    l is VideoLesson ? new VideoLessonDto
    {
        Id = l.Id,
        Title = l.Title,
        Duration = (l as VideoLesson).Duration,
    }
    : null
);
var result2 = query.Select(l => l is ArticleLessonDto ? "Article!" : "Something else!").ToList();

Now we get quite a different looking SQL:

SELECT l."Discriminator" = 'ArticleLesson', l."Id", l."Title", l."Content", l."Discriminator" = 'VideoLesson', l."Duration"
FROM "Lessons" AS l

Clearly, EF Core is fetching everything once again, and the fundamental reason for this is that EF Core doesn't understand that l in a .Select(l => ...) where l is of type LessonDto should be considered equivalent to l in .Select(l => ...) where l is Lesson, when the parameter is used on its own as an operand in an operation like this, or the case of #2373, in a null-checking operation, which was the root of the overfetching problem there as well.

I'd like us to perhaps discuss the various possible solutions to problems of this sort. If HC itself can't do much about this, maybe we can prepare a proposal for a feature for EF Core that would solve all the problems of this nature, problems that arise when using intermediary projections.
In other words, maybe solutions like #4985 are, in a sense, treating the symptoms, rather than the actual root cause of the problem.

Let me know your thoughts.

Steps to reproduce

Described above.

Relevant log output

No response

Additional Context?

No response

Product

Hot Chocolate

Version

12.8.0

@aradalvand aradalvand added the 🐛 bug Something isn't working label Apr 23, 2022
@michaelstaib
Copy link
Member

In other words, maybe solutions like #4985 are, in a sense, treating the symptoms, rather than the actual root cause of the problem.

This is exactly why we are so opposed to the key solution. I will talk to Shay from the EF team about this. HotChocolate.Data will get a major update for 13 and we have these issues on our list to solve.

@michaelstaib michaelstaib added this to the HC-13.0.0 milestone Apr 23, 2022
@michaelstaib michaelstaib removed this from the HC-13.0.0 milestone May 3, 2022
@ChilliCream ChilliCream locked and limited conversation to collaborators May 3, 2022
@michaelstaib michaelstaib converted this issue into discussion #5031 May 3, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

2 participants