ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.23k stars 745 forks source link

HotChocolate.Data.AutoMapper not applying projections correctly #4724

Closed nikolai-mb closed 2 years ago

nikolai-mb commented 2 years ago

Is there an existing issue for this?

Describe the bug

Issue originally posted in slack: https://hotchocolategraphql.slack.com/archives/CD9TNKT8T/p1643883475962069 Another related github issue exists, but has been closed #2002

When using AutoMapper and DTO projection, the correct selections are not being applied. I have created a very simple project to reproduce the issue: HotChocolateAutoMapperProjectionIssue

Not sure if i have overlooked something crucial about how the AutoMapper package should be used, but i have been unable to find a fix.

Steps to reproduce

Clone and run the project linked, this will create a entity framework in-memory sqlite database with seeded data. All sql commands will be printed to the output console.

Issue 1: DTO without relations

Run the following query: query { vehicles { guid } }

Expected SQL output:

SELECT "v"."Guid"
FROM "Vehicles" AS "v"

Actual SQL output:

SELECT "v"."Guid", "v"."ManufactureDate"
FROM "Vehicles" AS "v"

Assumption: ManufactureDate should not be retrieved from database in this query.

Issue 2: DTO with relations (relation not queried)

Run the following query: query { drivers { guid } }

Expected SQL output:

SELECT "d"."Guid"
FROM "Drivers" AS "d"
ORDER BY "d"."Id"

Actual SQL output:

SELECT "d"."Guid", "d"."Name", "d"."Id", "v"."Guid", "v"."ManufactureDate", "v"."Id"
FROM "Drivers" AS "d"
LEFT JOIN "Vehicles" AS "v" ON "d"."Id" = "v"."DriverId"
ORDER BY "d"."Id"

Assumption: No tables should be joined in this query, and only "Guid" should be retrieved from "Drivers" table.

The problems described above grows exponentially if the relation again has it's own relations, as all relationships in the tree will be queried.

Relevant log output

No response

Additional Context?

Issues has been reproduced in both SQL Server and Sqlite.

AutoMapper explicit expansion

I also tested mappings with explicit expansion:

CreateMap<Driver, DriverDto>().ForAllMembers(i => i.ExplicitExpansion());
CreateMap<Vehicle, VehicleDto>().ForAllMembers(i => i.ExplicitExpansion());

This results in the correct SQL query being created when no relations are requested query { drivers { guid } }

Results in:

SELECT "d"."Guid"
FROM "Drivers" AS "d"

Which is correct.

But now, relations are broken. Running the query:

query {
  drivers {
    guid
    vehicles {
      guid
    }
  }
}

Results in:

SELECT "d"."Guid"
FROM "Drivers" AS "d"

And now, the join is missing, resulting in a "Cannot return null for non-nullable field." error in the response.

Product

Hot Chocolate

Version

12.6.0 (also reproduced with 12.5.0)

PascalSenn commented 2 years ago

This is by design: https://github.com/ChilliCream/hotchocolate/blob/main/src/HotChocolate/Data/test/Data.AutoMapper.Tests/AutomapperTests.cs#L502 You have to do explicit mapping, otherwise automaker will include everything. We just delegate our projections to automapper

I will have to have a look at the example you provided to see why relations are not working

nhangeland commented 2 years ago

Have you had time to take a look at why relations aren't working? I'm having the same problem and it's quite critical for us to get this fixed.

PascalSenn commented 2 years ago

This seems to be a regression Automapper 11. It does work as intended with 10.11:

As you see in: https://github.com/nikolai-mb/HotChocolateAutoMapperProjectionIssue/pull/1

_s1 => Convert(new DriverDto() {Guid = _s1.Guid, Vehicles = _s1.Vehicles.Select(p1 => new VehicleDto() {Guid = p1.Guid}).ToList()}, Object)

Is translated by https://github.com/AutoMapper/AutoMapper/blob/master/src/AutoMapper/QueryableExtensions/Extensions.cs#L98 to

The member paths are computed wrong in version 11 image

I guess this should be fixed upstream in the automapper repository

michaelstaib commented 2 years ago

I am closing this one since its not on our side.

nikolai-mb commented 2 years ago

There is more to this than just AutoMapper 11 being the culprit as far as i can tell, i don't think this issue should be closed @michaelstaib .

I accepted the merge request from @PascalSenn in my reproduction project which resolves the specific examples i originally provided, but as far as i can tell, there are still issues with other queries.

In short, i have made two changes in my repro project:

Running the following query is not resolved by downgrading AutoMapper.

query {
  drivers(where: { vehicles: { any: true } }) {
    name
  }
}

Query results in the following exception:

{
  "errors": [
    {
      "message": "Unexpected Execution Error",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "drivers"
      ],
      "extensions": {
        "message": "The LINQ expression 'DbSet<Driver>()\r\n    .Where(d => new DriverDto{ Name = d.Name }\r\n    .Vehicles\r\n        .Any())' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to 'AsEnumerable', 'AsAsyncEnumerable', 'ToList', or 'ToListAsync'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.",
        "stackTrace": "   at Microsoft.EntityFrameworkCore.Query.QueryableMethodTranslatingExpressionVisitor.<VisitMethodCall>g__CheckTranslated|15_0(ShapedQueryExpression translated, <>c__DisplayClass15_0& )\r\n   at Microsoft.EntityFrameworkCore.Query.QueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)\r\n   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)\r\n   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)\r\n   at Microsoft.EntityFrameworkCore.Query.QueryableMethodTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)\r\n   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor visitor)\r\n   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)\r\n   at Microsoft.EntityFrameworkCore.Query.QueryCompilationContext.CreateQueryExecutor[TResult](Expression query)\r\n   at Microsoft.EntityFrameworkCore.Storage.Database.CompileQuery[TResult](Expression query, Boolean async)\r\n   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async)\r\n   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass12_0`1.<ExecuteAsync>b__0()\r\n   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler)\r\n   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.ExecuteAsync[TResult](Expression query, CancellationToken cancellationToken)\r\n   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.ExecuteAsync[TResult](Expression expression, CancellationToken cancellationToken)\r\n   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1.GetAsyncEnumerator(CancellationToken cancellationToken)\r\n   at System.Runtime.CompilerServices.ConfiguredCancelableAsyncEnumerable`1.GetAsyncEnumerator()\r\n   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable`1 source, CancellationToken cancellationToken)\r\n   at HotChocolate.Data.ToListMiddleware`1.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Types.EntityFrameworkObjectFieldDescriptorExtensions.<>c__DisplayClass2_1`1.<<UseDbContext>b__4>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at HotChocolate.Types.EntityFrameworkObjectFieldDescriptorExtensions.<>c__DisplayClass2_1`1.<<UseDbContext>b__4>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)\r\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"
      }
    }
  ]
}

Another query which causes a similar exception:

query {
  drivers(where: { vehicles: { some: { manufactureDate: { gt: "2010-01-01T00:00:00" } } } }) {
    name
  }
}

I also noticed another thing: Running the query:

query {
  drivers {
    name
    vehicles {
      manufactureDate
    }
  }
}

Results in the following SQL:

SELECT "d"."Name", "d"."Id", "v"."ManufactureDate", "v"."Id"
FROM "Drivers" AS "d"
LEFT JOIN "Vehicles" AS "v" ON "d"."Id" = "v"."DriverId"
ORDER BY "d"."Id"

I don't understand why d.Id or v.Id is included in the selection here. The DTO's don't even have a "Id" property on them. They are obviously used as part of the join and order, but as far as i'm aware, should not be included in the SELECT (they do not exist in the HC schema, nor on the DTO)

Is it possible for you to take another look at this @PascalSenn, would really appreciate any help. I tested both AutoMapper 10 and 11 in the above examples. I was unable to downgrade AutoMapper any further or test with EF Core 5 because of dependencies.

I have more examples involving more advanced queries and models, but i suspect that they might be caused by the same root issue as above.

PascalSenn commented 2 years ago

@nikolai-mb

The resulting projection for filtering is based on the expression tree. So

"The LINQ expression 'DbSet<Driver>()\r\n    .Where(d => new DriverDto{ Name = d.Name }\r\n    .Vehicles\r\n        .Any())' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to 'AsEnumerable', 'AsAsyncEnumerable', 'ToList', or 'ToListAsync'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

is the exception of ef that the expression is too complex for ef to translate. This is a directly caused by having a translation layer between the database and the GraphQL API.

I do not have a solution for this problem, as we can not make the expression any simpler. The only way around this would be to not use Automapper. It just adds another translation layer and increases the complexity.

SELECT "d"."Name", "d"."Id", "v"."ManufactureDate", "v"."Id"
FROM "Drivers" AS "d"
LEFT JOIN "Vehicles" AS "v" ON "d"."Id" = "v"."DriverId"
ORDER BY "d"."Id"

The selected field that you see here is what EF Core has selected. They generate SQL based on the expressions that we generate. I guess they include the Ids for the tracking in the store, but this is some implementation detail of ef core. Returning an Id is not much of an overhead for SQL, so I guess they will just fetch it always.

nikolai-mb commented 2 years ago

@PascalSenn Please forgive my ignorance on this topic as i don't have a lot of personal experience with AutoMapper nor expression trees, or how EF handles these under the hood. My assumption was that the projection would occur after the database has returned the results (with the filters applied). The examples i last provided do work with AutoMapper without the explicit expansion for all members as well as with Mapster (has the same issue with not applying projections correctly).

If what you're saying is correct (that AutoMapper / mapping tools are not applicable in this scenario), that would likely make certain architectures partially or completely useless in combination with hot chocolate (such as clean architecture, which i'm currently using). This is because our domain layer has no knowledge of hot chocolate nor the resolvers which fetches external data, since these live in the application layer on the associated DTO classes. I would assume that this would come as a big blow to many others as well, since the only alternative will most likely be either leaving hot chocolate, the current architecture, or all the seperation principles behind.

Based on my assumption described above, i thought the execution flow would be something on the line of:

If i come across as difficult in my reply, i apologize, it's not my intention. I just need to be absolutely sure, that this it not something which is fixable, since it would result in a lot of work for me and quite possibly many others.

PascalSenn commented 2 years ago

My assumption was that the projection would occur after the database has returned the results (with the filters applied).

Hm.. I mean, theoretically you could first filter and then do the projection, which would resolve the issue. But then you would filter on the domain objects rather than on the DTOs

The examples i last provided do work with AutoMapper without the explicit expansion for all members as well as with Mapster (has the same issue with not applying projections correctly).

That's correct, as the second layer of translation breaks the filtering. Lets have a look at a simplified example:

dbContext.Users

// When we apply the filters we do basically this
dbContext.Users.Where(x => x.Name == "SomeValue")

// When we apply the projections we do this
dbContext.Users.Where(x => x.Name == "SomeValue").Select(x => new User { Name = x.Name })

// with automapper we first do the mapping

dbContext.Users.Select(x => new UserDto { SomeProperty = x.Name }).Where(x => x.Name == "SomeValue")

// your specific example leads to this:
“dbContext.Drivers.Select(dtoDriver => new DriverDto{ Name = dtoDriver.Name }    )    .Where(_s0 => _s0.Vehicles .Any(_s1 => _s1.ManufactureDate > (DateTime)<>c__DisplayClass26_0<DateTime>.value))"

As we project first, EF core loses the context of what the filter is set on.

If what you're saying is correct (that AutoMapper / mapping tools are not applicable in this scenario), that would likely make certain architectures partially or completely useless in combination with hot chocolate (such as clean architecture, which i'm currently using). This is because our domain layer has no knowledge of hot chocolate nor the resolvers which fetches external data, since these live in the application layer on the associated DTO classes

I don’t think that this is the case. I know the clean architecture patterns. I do not want to go into detail of your current architecture as this would not really be helpful. But still it is important where AutoMapper and DTO’s are used. Usually in the Application Layer, not in the domain layer. Meaning, in the MVC days to map into the return structure of controllers.

That said, in GraphQL, the types are you DTO. You can easily extend a type with a resolver e.g.

So if you receive something like this from you domain layer

public class User  {  // or UserDto, whatever you have 
  public string FirstName {get; set;}
  public string LastName {get; set;}

}

You can extend this type in a lot of different ways

// annotation based
[ExtendObjectType(typeof(User), IgnoreProperties = {nameofUser.SomeHiddenField} ]
public class UserExtensions

{
 
  
   public string DisplayName([Parent] User user) => $”{user.FirstName} {user.LastName}”

}


 
  
   
 

   //or code first 
public class UserType : ObjectType<User> 

{
    protected override void Configure(IObjectTypeDescriptor<User> descriptor) 
   
    {
           
           descriptor.Ignore(x => x.SomeHiddenField);

           descriptor.Field(“displayName”).ResolveWith<Resolvers>(x => x.DisplayName(default));
   
     }

    public class Resolvers 
   
    {  
        
          public string DisplayName([Parent] User user) => $”{user.FirstName} {user.LastName}”
 ;   
    }

}

If you have a clean abstraction, then filtering, sorting and pagination is probably also a concern of the domain layer, rather than the GraphQL layer. You most likely do not want to expose all properties as filterable (probably not even one), as you only want to expose the stuff that the ui really needs. e.g. instead of doing

query GetUsers($search: String!) {
   
   users(where: { OR [{ name: {contains: $search}}, {lastName: {contains:$search}}]}) {
        name
        lastName
   }
}

You most likely want

query GetUsers($search: String!) {
   
    searchUsers(search: $search) {
        name
        lastName
   }
}

And let the search be a concern of the domain layer. This way the UI does not need to care about how you search.

I would assume that this would come as a big blow to many others as well, since the only alternative will most likely be either leaving hot chocolate, the current architecture, or all the seperation principles behind.

I would not say so. There are plenty of people using HotChocolate in a clean architecture project. We are always open for improvements. It’s just that we cannot optimise for every other third party library. Also seperation principles => X DO NOT use AutoMapper to support a complex layered architecture

Based on my assumption described above, i thought the execution flow would be something on the line of:

This assumption is correct.

The query is received and executed against the database based on the arguments recieved (select, filter, order etc..)

This is essentially

    [UseDbContext(typeof(AutomobileRepository))]
    [UseProjection]
    [UseFiltering]
    public IQueryable<DriverDto> Drivers(
        [ScopedService] AutomobileRepository repository,
        IResolverContext context)
    {
        var queryable = repository.Drivers.ProjectTo<Driver, DriverDto>(context).Filter(context);
        return queryable;
         // after you return here filtering is executed before any other resolver of this type is called
    }

I hope this reply was somewhat helpful.