fix: Broken fragment spreads

[budde377]

#199 Introduced a new way to resolve fragments.

#223 Broke that integration, the motivation being that it seemed too strict in the case where the newly introduced possibleTypes weren't defined. The solution was making a distinction between how fragment spreads and inline fragment spreads were handled, which is fundamentally wrong. The test which should have caught this was basically disabled because of bad naming. I've renamed this to possible_types_test.dart

This PR allows us to detect if the possibleTypes map isn't provided and in this case optimistically spread every field, regardless of it being an inline fragment spread or fragment spread. This is a proposal to a fallback logic.

I've also updated the union_type_inline_fragments_test.dart test. The current logic is wrong. Without possible types, there's no way for us to know if Author is an implementation of Book, or as is probably intended here a union type.

[smkhalsa]

Thanks for your efforts on this. Please see my comments.

[smkhalsa]

The normalized map shouldn't include fields that don't exist in the source data (i.e. 'name': null on Book).

A field with a value of null is not semantically identical to the absence of the field. In this case, if our Book type doesn't include a name field in the schema, it doesn't make sense for it to be included in the normalized data.

Furthermore, if Book does have a name field which is non-nullable, but the query that fetched this data didn't ask for it, we'd now have an invalid state. This would cause an error when the user reads a query from the cache that does include the name field, resulting in a null value for a non-nullable field.

[budde377]

Bear in mind that this is the same behavior as you've introduced in #223 for named fragments. I would love to hear your opinion on the best way to handle the fallback behavior in a uniform way for inline fragments and named fragments.

The problem is that without possibleTypes we can't know if the schame is either

union BookAndAuthor = Book | Author

or

interface Book {
  title: String
}

type Author implements Book {
  title: String
  name: String
}

or

interface Author {
  name: String
}

type Book implements Author {
  title: String
  name: String
}

or

interface Author {
  name: String
  title: String
}

type Book implements Author {
  title: String
  name: String
}

The current solution optimistically expands every field, when the possibleTypes is not defined. This is bound to be wrong in some cases.

The logic implemented in #199 was more stringent and required the typename and type condition to exactly match if the possibleTypes weren't defined, which was a safe bet. I don't know the motivation for its removal in #223.

[smkhalsa]

What if we expand only the fields that include data in the response? If the server is sending data (including an explicit null), we can assume that it's a valid field on the underlying type for the given query.

[budde377]

@smkhalsa, I've had a look over this. The problem with this approach, as I see it, is that if we want to filter the fields by data, then we'll be allowing fetching partial data. E.g., consider the query

query Q1 {
  vehicle {
    ... on Vehicle { driver { __typename id name } } 
    ... on Car { driver { __typename id age } } 
  }
} 

against schema (1)

type Person { id: ID! name: String age: Int }

interface Vehicle {
  id: ID!
  driver: Person
}

type Car implements Vehicle {
  id: ID!
  driver: Person
}

and the schema (2)

type Driver { id: ID! name: String age: Int }

type Vehicle {
  id: ID!
  driver: Person
}

type Car  {
  id: ID!
  driver: Person
}

Against the first schema the data should be:

{ 
  ...
  "Driver:1": { "name": "Allan", "age": 30 }
  ...
}

and the second

{ 
  ...
  "Driver:2": { "name": "Allan" }
  ... 
}

or

{ 
  ...
  "Driver:2": { "age": "30" }
  ...
}

so this will be a problem for all selection set under a fragment.

I'm curious about the reason for the aversion around only expanding fragments on exact matches, which was the logic implemented in #199. It still seems to me like the best approach. Sure, we can go in and make all kinds of convoluted rules to expand these fragments, but ultimately it'll make the logic harder to understand and never be perfect. Why can't we expect the users to provide possibleTypes if their fragments aren't expanding properly?

[smkhalsa]

Ok, I think I agree with you. While following the shape of the data should work during normalization, there's no way to know what fields must be selected during denormalization, so only expanding fragments on exact matches seems like the only way to ensure correctness.

I'd like to keep the API as simple as possible with minimal configuration. To that end, I've done some initial work (untested) to generate the possibleTypes from the schema which the user can include when creating a client.

[budde377]

Awesome. I've updated the PR to revert to the stricter check originally part of #199.

[budde377]

Interesting suggestion. Let me have a go at this this weekend.

…

On Tue, 9 Nov 2021 at 13:32, Sat Mandir S. Khalsa ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/normalize/test/query_fragments/inline_and_named_fragments_are_same_test.dart <#300 (comment)>: > + 'title': 'My awesome blog post' + }, + 'Author:324': {'id': '324', '__typename': 'Author', 'name': 'Nicole'} + }; + final normalizedMapWithoutPossibleTypes = { + 'Query': { + 'booksAndAuthors': [ + {'\$ref': 'Book:123'}, + {'\$ref': 'Author:324'} + ] + }, + 'Book:123': { + 'id': '123', + '__typename': 'Book', + 'title': 'My awesome blog post', + 'name': null What if we expand only the fields that include data in the response? If the server is sending data (including an explicit null), that means it's a valid field on the underlying type for the given query. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#300 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2UFS6XI42I2L57FGRPNU3ULEPFJANCNFSM5HQVXPHQ> .

[smkhalsa]

LGTM. Thanks for your work on this.

[budde377]

@smkhalsa, we're getting some issue reports on GraphQL Client on fragments and we'll need to communicate these changes out to our users. IIUC, this hasn't been released yet, what is your plan for release, so I can coordinate comms?

[smkhalsa]

@budde377 I just published normalize 0.6.0