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

UsePagination / UseFiltering causes dropped data for 1:N relationships #5131

Open zaneclaes opened 2 years ago

zaneclaes commented 2 years ago

Is there an existing issue for this?

Describe the bug

I am using pagination/filtering/sorting with an IEnumerable output from my MySQL database (using Pomelo).

When I issue a GraphQL query to filter upon some query input, the filter is matched and the correct items are returned, but the nested arrays are cleared (empty), despite the fact that the where clause required that they not be empty. In fact, it appears as though the Lists are Clear()ed some time between the application of the filter and the returning of the data.

Steps to reproduce

GraphQL endpoint:

[UseFurDataContext] [UsePaging] [UseFiltering] [UseSorting]
  public IEnumerable<Furball> SearchFurballs([Service] FurContext context) => context.Data.Furballs.IncludeDefaultFurball();

Where Data is a DbContext. The IncludeDefaultFurball performs .Include(fb => fb.Inventory).ThenInclude(c => c.Items). Thus, I am able to successfully filter the output with the query:

query {
  searchFurballs(where: { inventory: { items: { some: { itemId: { eq: 66561 } } } } }) {

The top level items returned are correct, but the nested inventory.items are missing from the output:

{
  "data": {
    "searchFurballs": {
      "nodes": [
        {
          "id": "0x010201070300070a07081800",
          "inventory": {
            "id": "0x010201070300070a07081800",
            "items": []
          }
        },
        {
          "id": "0x01040713040001060a010400",
          "inventory": {
            "id": "0x01040713040001060a010400",
            "items": []
          }
        },

I have verified that there should be items present by also double checking the database, as well as performing other queries on the same objects.

The same bug also happens on any other nested List style field that I attempt to filter upon.

Curiously, the bug can be worked around by adding a id: { neq: "" }, to the where clause. Suddenly, the correct data is returned when I run this query:

query {
  searchFurballs(where: { id: { neq: "" }, inventory: { items: { some: { itemId: { eq: 66561 } } } } }) {

Relevant log output

No response

Additional Context?

No response

Product

Hot Chocolate

Version

12.9.0

PascalSenn commented 2 years ago

@zaneclaes i guess you need to use projections to project related entites

PascalSenn commented 2 years ago

this is the way we do includes

zaneclaes commented 2 years ago

@PascalSenn I've never been able to use Projections in any of my HotChocolate projects due to your comment here: https://github.com/ChilliCream/hotchocolate/issues/1658#issuecomment-716068068 (my projects all make heavy use of abstract base classes). It's been a while since I checked in, but I now notice now that it was supposed to be fixed last year with https://github.com/ChilliCream/hotchocolate/pull/3650 , but I'm still getting the same error with version 12.9.0:

C# code:

[UseFurDataContext] [UsePaging] [UseProjection] [UseFiltering] [UseSorting]
  public IQueryable<Furball> SearchFurballs([Service] FurContext context) => context.Data.Furballs;

GraphQL result (same error as OP from 2020):

{
  "errors": [
    {
      "message": "Can't compile a NewExpression with a constructor declared on an abstract class",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "searchFurballs"
      ],
      "extensions": {
        "code": "NotFound"
      }
    }
  ],
  "data": {
    "searchFurballs": null
  }
}
PascalSenn commented 2 years ago

the issue is resolved in the way that you can now project abstract types. the limitation you run into is still there and is not really fixable with how projections work at the moment. I suppose you have a abstract Furball somewhere in your abstractions and a concrete implementation somwhere else, this makes it no possible to project like new Furball() { id = x.id} as the class is abstract. We also do not know the concrete types so we can also not project into new ConcreteFurball() { Id =x.Id}.

in v13 we provide a context for Projections. so that you have easier access to the selected fields (and can include manually)

PascalSenn commented 2 years ago

what's weird is indeed that the includes are ignored. How does the expression look like?

You can flilter in the resolver to have a look at the expression:

[UseFurDataContext] [UsePaging] [UseFiltering] [UseSorting]
  public IEnumerable<Furball> SearchFurballs([Service] FurContext context, IResolverContext rc) => context.Data.Furballs.IncludeDefaultFurball().Filter(rc).Sort(rc); <-- what is this expression
zaneclaes commented 2 years ago

I suppose you have a abstract Furball somewhere in your abstractions and a concrete implementation somwhere else

No, I do not. The base abstract class does not share a name with the concrete implementation.

the includes are ignored.

I don't think that's true. Notice that the where clause is filtering using the included fields!!

This query: inventory: { items: { some: { itemId: { eq: 66561 } } } } Should make it impossible to have this result: inventory { items: [] }

There is no way the query would have returned any data at all if the Includes were ignored.

Also, the where clause works correctly, filtering invalid results as-expected.

HotChocolate must be emptying the included Lists' data at some point between the where clause and returning the data.

You can flilter in the resolver to have a look at the expression:

Where should I see the expression? I added that but don't see any logs or anything.

zaneclaes commented 2 years ago

It seems the problem with Projections is nested abstract relationships.

In the query I gave, the items are an array of abstract GameItem objects. If I remove inventory.items from the GraphQL selection set, the query works:

Screen Shot 2022-06-07 at 7 24 05 AM

And including the items breaks it:

Screen Shot 2022-06-07 at 7 24 16 AM

Where items are: public List<GameItem> Items { get; set; } = new List<GameItem>();

And abstract GameItem is a unique class name (not shared by any other class).

PascalSenn commented 2 years ago

@zaneclaes is GameItem a graphql interface/union?

zaneclaes commented 2 years ago

@PascalSenn thanks, that explains the projection problem. I can see why that's necessary.

However, I still have the original problem, even using projections, in certain cases. The root cause is that some of the queryable fields are derived from other fields. For example, notice how public StatEntity BattleStats => GameStats.Last(); breaks this query:

Screen Shot 2022-06-07 at 9 46 50 AM

Again, just like in the original issue, we know that the gameStats must have at least one item during the query. However, the battleStats field throws an exception because gameStats is empty.

I suppose at this point I need to customize the projection so that battleStats causes gameStats to be included?

Note that returning the entire gameStats array in the response (instead of battleStats) works fine. So in fact, I could also use a hack like this to force the projection to load the data...


  searchFurballs(
    where: {
      gameStats: { 
        any: true,
        all: { skills: { some: { skillDefinitionId: { in: [1] } } } }
      }
    }
  ) {
    totalCount
    nodes {
      id 
      gameStats { id }            # FORCE PROJECTION LOAD
      battleStats {
        id
        skills {
          id
          skillDefinitionId
        }
      }
    }
  }
}```
zaneclaes commented 2 years ago

I tried adding [UseProjection(true)] to gameStats. Unfortunately, this leads to a new error — it seems at least 4 other people are running into this error, and nobody has found a solution yet. I've tried adding it both to the nested object (which causes the error described in the link) and the fields on that nested object (which doesn't fix the problem).

PascalSenn commented 2 years ago

hm... i see you problem. So projecting gamestats would resolve the whole list of gamestats, so even if that would work, then we would load unnecessary columns. In v13 we add support for expression in filtering and sorting. We also want to add expression support to projection, so that you could actually express your resolver as a expression (GameStats.Last). This would make you able to project these things. The Problem we have there is that currently we project directly into the model, essentially .Select(x => new Furball { Id = x.Id). This works great for Models, but as soon as you have fancy resolvers on them, we have to backing field to project. What we plan to do is to rewrite how projections work internally so that we can project everything we want. But to implement this we need a bunch of changes to the execution engine.