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.16k stars 736 forks source link

UseProjection on mutation with mutation conventions #5300

Closed Driedas closed 1 year ago

Driedas commented 2 years ago

Is there an existing issue for this?

Describe the bug

as per the hotchocolate #general slack channel thread, we are using hot chocolate 12.7 with the following mutation (actual entity and property names are replaced)

mutation CreateEntity {
  createEntity (input: {
    foo: 1,
    bar: "aaa",
    childId: 5
  }){
    entity {
      id,
      foo,
      bar,
      childId,
      child {
        id,
        name
      }
    }
  }
}

The expected response should look like below (note that both the childId is returned and the child property itself is expanded)

{
  "data": {
    "createEntity": {
      "entity": {
        "id": 1234,
        "foo": 1,
        "bar": "aaa",
        "childId": 5,
        "child": {
          "id": 5,
          "name": "foobar"
        }
      }
    }
  }
}

The actual response however looks like this:

{
  "data": {
    "createEntity": {
      "entity": {
        "id": 1234,
        "foo": 1,
        "bar": "aaa",
        "childId": 5,
        "child": null
      }
    }
  }
}

The mutation method looks like this:

    [HotChocolate.Data.UseFirstOrDefault]
    public async Task<IQueryable<Entity>> CreateEntity(int foo,
                string bar,
                int childId,
                [Service] SomeService service,
                [Service] DataContext dataContext,
                CancellationToken cancellationToken = default)
    {
        int id = await service.CreateEntity(foo,
                bar,
                                childId,
                cancellationToken);

        return dataContext.Entities.Where(cd => cd.Id == id);
    }

We have tried to apply the UseProjection attribute as well, thinking that that may be the reason why we are not getting the expanded navigation property child but hotchocolate then throws the following exception:

"Unable to cast object of type 'System.Linq.Expressions.Expression1`1[System.Func`2[System.Object,System.Object]]' to type 'System.Linq.Expressions.Expression`1[System.Func`2[Entity,Entity]]'.",
at HotChocolate.Data.Projections.Expressions.QueryableProjectionScopeExtensions.Project[T](QueryableProjectionScope scope)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectionContextExtensions.Project[T](QueryableProjectionContext context)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectionProvider.<>c__5`1.<CreateApplicatorAsync>b__5_0(IResolverContext context, Object input)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectionProvider.<>c__DisplayClass4_0`1.<<CreateExecutor>g__ExecuteAsync|1>d.MoveNext()
--- End of stack trace from previous location ---
   at HotChocolate.Data.Projections.FirstOrDefaultMiddleware`1.InvokeAsync(IMiddlewareContext context)
   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)
   at HotChocolate.Types.MutationConventionMiddleware.InvokeAsync(IMiddlewareContext context)
   at HotChocolate.Types.ErrorMiddleware.InvokeAsync(IMiddlewareContext context)
   at HotChocolate.AspNetCore.Authorization.AuthorizeMiddleware.InvokeAsync(IDirectiveContext context)
   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)
   at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)
   at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"

@michaelstaib is suggesting that this may be connected to the rewriting of the schema when mutation conventions are applied...

Steps to reproduce

  1. have an entity framework core entity with a navigation property
  2. create a graphql mutation (with mutation conventions applied) that exposes a method to persist an instance of this entity and return the newly created entity loaded from the data context (as IQueryable with UseFirstOrDefault applied)
  3. call the mutation method and request the result to return the entity with the navigation property expanded
  4. hotchocolate returns the entity, however the navigation property is not expanded, it is null (as per above code snippets)

Relevant log output

No response

Additional Context?

No response

Product

Hot Chocolate

Version

12.7.0

cts-tradeit commented 2 years ago

You need to use [UseProjection] to access data not directly on the DTO. That's what makes the magic happen as it will add additional Select() to the IQueryable that will load the related entity.

Try projecting manually :

class Mutations {
    [UseProjection]
    public Task<Stuff> SaveStuff(StuffInput input, [Service] DataContext dataContext, IResolverContext ctx) {
        int stuffId = /* do something with input etc... */;
        return dataContext.Entities.Where(cd => cd.Id == stuffId).Project(ctx).Single();
    }
}

Does it change anything?

Driedas commented 2 years ago

hi, the exception is the same, the stacktrace is (obviously) slightly different this time:

   at HotChocolate.Data.Projections.Expressions.QueryableProjectionScopeExtensions.Project[T](QueryableProjectionScope scope)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectionContextExtensions.Project[T](QueryableProjectionContext context)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectionProvider.<>c__5`1.<CreateApplicatorAsync>b__5_0(IResolverContext context, Object input)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectExtensions.ExecuteProject[T](T input, IResolverContext context, Type expectedType)
   at HotChocolate.Data.Projections.Expressions.QueryableProjectExtensions.Project[T](IQueryable`1 queryable, IResolverContext context)
   at Website.Graph.Extensions.Statements.ColumnDefinitionMutationExtensions.CreateColumnDefinition(Int32 columnArrangementId, Int32 ordinal, Nullable`1 columnId, Int32 columnFactoryId, String parameters, ColumnArrangementService service, RepositoryContext dataContext, IResolverContext resolverContext, CancellationToken cancellationToken) in C:\\Work\\Synchtank\\IRIS.Net\\Sources\\Server\\Website\\Graph\\Extensions\\Statements\\ColumnDefinitionMutationExtensions.cs:line 57
   at HotChocolate.Resolvers.Expressions.ExpressionHelper.AwaitTaskHelper[T](Task`1 task)
   at HotChocolate.Types.Helpers.FieldMiddlewareCompiler.<>c__DisplayClass9_0.<<CreateResolverMiddleware>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at HotChocolate.Data.Projections.Expressions.QueryableProjectionProvider.<>c__DisplayClass4_0`1.<<CreateExecutor>g__ExecuteAsync|1>d.MoveNext()
--- End of stack trace from previous location ---
   at HotChocolate.Types.MutationConventionMiddleware.InvokeAsync(IMiddlewareContext context)
   at HotChocolate.Types.ErrorMiddleware.InvokeAsync(IMiddlewareContext context)
   at HotChocolate.AspNetCore.Authorization.AuthorizeMiddleware.InvokeAsync(IDirectiveContext context)
   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)
   at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)
   at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)

The response works as expected only when I remove UseProjection and manually include the child navigation property via

return dataContext.Entities
  .Include(x => x.Child)
  .Where(cd => cd.Id == id);
cts-tradeit commented 2 years ago

This is strange, we use this in our project on 12.9.0 and it works. Can you test it against 12.9.0?

Driedas commented 2 years ago

Same after upgrading to 12.9.0. The UseProjection attribute only works with no issues on the Query endpoint, it expands the child navigation property correctly...

cts-tradeit commented 2 years ago

Btw did u remove UseSingleOrDefault before testing manual projections?

Driedas commented 2 years ago

yep, only the UseProjection attribute is applied, as per your code snippet. We can try this the other way round, do you have a working sample somewhere that I could checkout/fork and figure out where the differences are between your setup and ours?

cts-tradeit commented 2 years ago

I made a working sample for you. It uses EFCore's in-memory databse. Entities Car and Owner have configure nav. properties to each other. In Program.cs there a data seeding code which you may adjust.

Link: https://github.com/cts-tradeit/hc-mutation-projection

Schema:

type Queries {
  cars: [Car!]!
  owners: [Owner!]!
}

type Mutations {
  changeLicensePlate(id: Int!, newPlate: String!): Car!
}

type Owner {
  id: Int!
  name: String
  cars: [Car!]
}

type Car {
  id: Int!
  licensePlate: String
  ownerId: Int!
  owner: Owner
}

Sample:

mutation {
  changeLicensePlate(id: 1, newPlate: "TESTING PLATE 2") {
    id
    licensePlate
    owner {
      name
    }
  }
}

will produce

{
  "data": {
    "changeLicensePlate": {
      "id": 1,
      "licensePlate": "TESTING PLATE 2",
      "owner": {
        "name": "John Doe"
      }
    }
  }
}

The Mutations class looks like:

    public class Mutations
    {
        [UseProjection]
        public async Task<Car> ChangeLicensePlateAsync(int id, string newPlate, TestDbContext dbContext, IResolverContext ctx)
        {
            Car car = await dbContext.Set<Car>().SingleAsync(c => c.Id == id);
            car.LicensePlate = newPlate;
            dbContext.Update(car);
            await dbContext.SaveChangesAsync();

            return await dbContext.Set<Car>().Where(c => c.Id == id).Project(ctx).SingleAsync();
        }
    }
Driedas commented 2 years ago

thanks a lot, I'll give it a go...

Driedas commented 2 years ago

have done a fork of the project and it works out of the box. However, as soon as you add mutation conventions into the mix, the same exception is back. In order to reproduce, try changing the graphql configuration to

builder.Services.AddGraphQLServer()
    .RegisterDbContext<TestDbContext>()
    .AddQueryType<Queries>()
    .AddMutationType<Mutations>()
    .AddMutationConventions() // line added
    .AddProjections();

and try calling the following mutation (amended to comply with the mutation conventions applied)

mutation ChangeLicensePlate {
  changeLicensePlate(input: { id: 1, newPlate: "foobar" }) {
    car {
      id
      licensePlate,
      ownerId,
      owner {
        id,
        name
      }
    }
  }
}

It looks like the mutation conventions break the UseProjection functionality... In fact, you don't even need to add the expanded owner property into the request, even this fails:

mutation ChangeLicensePlate {
  changeLicensePlate(input: { id: 1, newPlate: "foobar" }) {
    car {
      id
    }
  }
}
cts-tradeit commented 2 years ago

As a workaround you may opt out from conventions on methods that require projection.

Driedas commented 2 years ago

Unfortunately that is not an option at the moment, we've spent the previous "sprint" enabling mutation conventions and updating all of the frontend calls. Once we've stumbled upon this issue, we've recommended to the frontend team in the interim to follow mutation calls with a query for details, if the use case requires anything beyond the first level entity properties - hoping that this is just us having something misconfigured on our end...

Is this something that is going to be fixed? Something I can help with?

Strandedpirate commented 2 years ago

I'm having the exact same issue. AddMutationConventions(true) in program.cs, and then any attempt to use a projection on a mutation throws an exception. This is not ideal, because now I have to map thousands of unnecessary LH-RH mappings for every mutation that I need a projection on.

[Authorize()]
[UseProjection]
[UseFirstOrDefault]
public async Task<IQueryable<UserCryptographyHistory>> UserCryptographyPinItem(
       MyDbContext dbContext,
       CancellationToken ct,
       [GlobalState(nameof(User.UserId))] int userId,
       [ID] int userCryptographyHistoryId)
{
  ...

  return dbContext.UserCryptographyHistories
      .Where(a => a.UserCryptographyHistoryId == userCryptographyHistoryId && a.UserId == userId)
      .AsQueryable(); // SQL query executes OK, but HC explodes
}
Unable to cast object of type 'System.Linq.Expressions.Expression1`1[System.Func`2[System.Object,System.Object]]' to type 'System.Linq.Expressions.Expression`1[System.Func`2[Ripple.Domain.Entities.Ripple.UserCryptographyHistory,Ripple.Domain.Entities.Ripple.UserCryptographyHistory]]

This works:

[Authorize()]
public async Task<UserCryptographyHistory> UserCryptographyPinItem(
       MyDbContext dbContext,
       CancellationToken ct,
       [GlobalState(nameof(User.UserId))] int userId,
       [ID] int userCryptographyHistoryId)
{
  ...

  return await dbContext.UserCryptographyHistories
        .Where(a => a.UserCryptographyHistoryId == userCryptographyHistoryId && a.UserId == userId)
        // this sucks, having to map LH-RH for all the things, cause I need one column from the CryptographyType table...
        .Select(a => new UserCryptographyHistory
        {
            CreatedOn = a.CreatedOn,
            CryptographyType = new CryptographyType
            {
                Name = a.CryptographyType.Name
            },
            CryptographyTypeId = a.CryptographyTypeId,
            Input = a.Input,
            IsFavorite = a.IsFavorite,
            IsPinned = a.IsPinned,
            Output = a.Output,
            User = a.User,
            UserCryptographyHistoryId = a.UserCryptographyHistoryId,
            UserId = userId,
        })
        .FirstOrDefaultAsync();
}
Rlamotte commented 1 year ago

I reproduce the same behaviour which seems to be a bug due to conflict between UseProjection and UseMutationConvention.

Currently I'm forced to include manually child elements which represents a lack of performance and causes graphQL to be a bit obsolete...

A fix which give the opportunity to use MutationConvention (which seems great to make mutation sustainable) and UseProjection (which seems perfect to be able to let the client choose only variables needs) together would be appreciated.

jordan-gistlens commented 1 year ago

I've been running into this also using v13.0.5, I'm a big fan of mutation conventions but without projections it's making some of the more complex responses involving multiple nested navigation properties really hard to support. @michaelstaib is this on any roadmap or intended to be resolved in any upcoming release?

Here is another example of how I'm using them where it produces the below error.

[ExtendObjectType(OperationTypeNames.Mutation)]
public class TenantCreateMutation
{
    [UseFirstOrDefault]
    [UseProjection]
    [Error(typeof(DomainException))]
    public async Task<IQueryable<Tenant>> TenantCreate(
        [Service] ApplicationDbContext database,
        [Service] IRequestClient<TenantCreateCommand> client,
        string name,
        CancellationToken cancellation)
    {
        var command = new TenantCreateCommand { Name = name };

        Response response =
            await client.GetResponse<TenantCreateCommandResult, TenantCreateCommandRejected>(command, cancellation);
        return response switch
        {
            (_, TenantCreateCommandResult result) => database.Tenants.Where(x => x.Id == result.Id),
            (_, TenantCreateCommandRejected error) => throw new DomainException(error.ErrorCode),
            _ => throw new InvalidOperationException()
        };
    }
}
      "extensions": {
        "message": "Unable to cast object of type 'System.Linq.Expressions.Expression1`1[System.Func`2[System.Object,System.Object]]' to type 'System.Linq.Expressions.Expression`1[System.Func`2[Giantnodes.Service.Tenants.Domain.Entities.Tenant,Giantnodes.Service.Tenants.Domain.Entities.Tenant]]'.",
        "stackTrace": "   at HotChocolate.Data.Projections.Expressions.QueryableProjectionScopeExtensions.Project[T](QueryableProjectionScope scope)\r\n   at HotChocolate.Data.Projections.Expressions.QueryableProjectionContextExtensions.Project[T](QueryableProjectionContext context)\r\n   at HotChocolate.Data.Projections.Expressions.QueryableProjectionProvider.<CreateApplicator>b__7_0[TEntityType](IResolverContext context, Object input)\r\n   at HotChocolate.Data.Projections.Expressions.QueryableProjectionProvider.<>c__DisplayClass4_0`1.<<CreateExecutor>g__ExecuteAsync|1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at HotChocolate.Data.Projections.FirstOrDefaultMiddleware`1.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)\r\n   at HotChocolate.Types.MutationConventionTypeInterceptor.<>c__DisplayClass16_0.<<ApplyResultMiddleware>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at HotChocolate.Types.MutationConventionMiddleware.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Types.ErrorMiddleware.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)\r\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"
      }
Pjotrtje commented 1 year ago

FYI, when not using the "ErrorAttributes flow" but when using the "MutationResult flow" it is also broken. Thus someting like (rewrote above code in other flow):

[ExtendObjectType(OperationTypeNames.Mutation)]
public class TenantCreateMutation
{
    [UseFirstOrDefault]
    [UseProjection]
    public async Task<IQueryable<MutationResult<Tenant, DomainError>>> TenantCreate(
        [Service] ApplicationDbContext database,
        [Service] IRequestClient<TenantCreateCommand> client,
        string name,
        CancellationToken cancellation)
    {
        var command = new TenantCreateCommand { Name = name };

        Response response =
            await client.GetResponse<TenantCreateCommandResult, TenantCreateCommandRejected>(command, cancellation);
        return response switch
        {
            (_, TenantCreateCommandResult result) => database.Tenants.Where(x => x.Id == result.Id),
            (_, TenantCreateCommandRejected error) => new DomainError(error.ErrorCode),
            _ => throw new InvalidOperationException()
        };
    }
}

also fails.

I think "Stage 6a Errors" are the way to go, but to make it usefull it should be able to work with Projections... Best would be that code as above would work. But a temp workable workaround would also be very valuable I guess. Something like the above mentioned return await dbContext.Set<Car>().Where(c => c.Id == id).Project(ctx).SingleAsync(); Something like return await dbContext.Set<Car>().Where(c => c.Id == id).ProjectOkResult(ctx).SingleAsync(); (which only uses the "non-errors" part of the projection)

hahn-kev commented 1 year ago

I'm also running into this. It looks like there's some code for handling a similar issue when using Relay conventions. I'm not sure how much work it might be, but I'm considering looking into this and making a PR for it.

Pjotrtje commented 1 year ago

@michaelstaib, @hahn-kev , did we expect the linked PR to also solve the issue for MutationResults + Projections? Thus something like:

[ExtendObjectType(OperationTypeNames.Mutation)]
public class TenantCreateMutation
{
    [UseFirstOrDefault]
    [UseProjection]
    public async Task<MutationResult<IQueryable<Tenant>, DomainError>> TenantCreate(
        [..]
        CancellationToken cancellation)
    {
        [..]
    }
}

i.c.w. .AddMutationConventions

Becasue it throws "Cannot Resolve PayLoadType"; even with latest versions of HC

hahn-kev commented 1 year ago

I didn't test that case. You should be able to just use the queryable as the return type and let the mutation convention change the type in the scheme. So basically the Error attribute flow.

Pjotrtje commented 1 year ago

Ah, but we do not want to use the "Error attribute" flow but want to use the "HotChocolate.MutationResult<TResult, TError>" flow. I will create a new issue. Thanks for the response!

nicomoya123 commented 6 months ago

@Pjotrtje did you open an issue on it ? I am facting the same issue

Pjotrtje commented 6 months ago

@nicomoya123 nope. So feel free to open issue. And I will give it a heart :)