Null fields being written to cache for partial object (overwriting existing fields)

[kvenn]

Describe the issue A partial model provided to optimisticResult is overwriting the cache for the fields that were not provided with null values.

To Reproduce (MUST BE PROVIDED)

I provide this object to optimisticResult:

{
   id: 123
   friendStatus: SENT_PENDING
}

There's already a user with id 123 in cache:

{
   id: 123
   name: kyle
   friendStatus: NOT_FRIENDS
}

After the request is made, the cache will update user with id 123 to the following:

{
   id: 123
   name: null
   friendStatus: SENT_PENDING
}

Expected behavior The omitted fields will not overwrite the existing cache fields with null.

{
   id: 123
   name: kyle
   friendStatus: SENT_PENDING
}

device / execution context iOS

Full code

    final store = await HiveStore.open();
    final cache = GraphQLCache(
      store: store,
      possibleTypes: POSSIBLE_TYPES_MAP,
    );
    final token = getToken();
    final link = HttpLink(networkUrlManager.getGraphQLUrl(),
        defaultHeaders: {'Authorization': 'Bearer $token'});
    final client = GraphQLClient(
      cache: cache,
      link: link,
    );

    final variables = VariablesMutationCreateFriendRequest(
        input: InputFriendRequestInput(otherUserId: userId));

    QueryResult result = await client.mutate(MutationOptions(
        // ISSUE - this is setting the friendStats correctly, but setting all other fields on the User to null (the ones not provided)
        optimisticResult: <String, dynamic>{
          'data': {
            'createFriendRequest': {
              '__typename': 'User',
              'id': userId,
              'friendStatus': 'SENT_PENDING',
            },
            '__typename': 'Mutation'
          }
        },
        document: OptionsMutationCreateFriendRequest(variables: variables).document,
        variables: variables.toJson()));

[kvenn]

I think this issue might be in the normalize_node library.

          final fieldData = normalizeNode(
            selectionSet: field.selectionSet,
            dataForNode: dataForNode[inputKey],
            existingNormalizedData: existingFieldData,
            config: config,
            write: write,
          );

dataForNode is null, because there's no value at inputKey. existingNormalizeData will have a value, but be overwritten. And therefore fieldData will be null.

But I'd assume that it would first check if the partial (optimistic) object contains the key. If it does not, it should take the existing data. If it does contain the key, even if null, it'll overwrite.

[budde377]

Can you provide the mutation of your example?

[vincenzopalazzo]

This is a client error and I'm sorry for that.

We are missing this feature because we skip the cache at all https://github.com/zino-hofmann/graphql-flutter/blob/main/packages/graphql/lib/src/core/query_manager.dart#L75 I guess due to the complexity introduced by the optimistic result with the cache!

I will take this task, because I was already in mind refactoring this class a little bit, and maybe this issue is a good excuse.

The correct behavior is reported by the Appollo docs https://www.apollographql.com/docs/react/performance/optimistic-ui/#optimistic-mutation-lifecycle and in summary it is:

1. When the code above calls mutate, the Apollo Client cache stores a Comment object with the field values specified in optimisticResponse. However, it does not overwrite the existing cached Comment with the same cache identifier. Instead, it stores a separate, optimistic version of the object. This ensures that our cached data remains accurate if our optimisticResponse is wrong.
2. Apollo Client notifies all active queries that include the modified comment. Those queries automatically update, and their associated components re-render to reflect the optimistic data. Because this doesn't require any network requests, it's nearly instantaneous to the user. Eventually, our server responds with the mutation's actual resulting Comment object.
3. The Apollo Client cache removes the optimistic version of the Comment object. It also overwrites the canonical cached version with values returned from the server.
4. Apollo Client notifies all affected queries again. The associated components re-render, but if the server's response matches our optimisticResponse, this is invisible to the user.

[kvenn]

Thank you, @vincenzopalazzo, for the response! That was also my expectation for how optimistic query updates would work.

cache stores a Comment object with the field values specified in optimisticResponse.

Leads me to assume it just stores the provided fields (and doesn't store the omitted values with null).

associated components re-render to reflect the optimistic data

This is kind of how graphql-flutter is working now, it's just not merging the models correctly (as far as I can tell).

---

And sure, @budde377. Please let me know if you see I'm doing anything wrong.

mutation CreateFriendRequest($input: FriendRequestInput!) {
  createFriendRequest(input: $input) {
    ...UserWithFriendship
  }
}

fragment UserWithFriendship on User {
    ...User
    friendStatus
}

fragment User on User {
    id
    name
}

[vincenzopalazzo]

We don't update anything here the code https://github.com/zino-hofmann/graphql-flutter/blob/main/packages/graphql/lib/src/core/query_manager.dart#L75

[budde377]

Okay, so I think there're two things here. Arguably, we shouldn't be writing the optimistic result to the cache for the reasons mentioned in the apollo docs. I haven't looked into the details of how this is currently done, but I suspect it'll require a significant amount of work to get this feature to par.

The second problem is submitting partial data in the optimistic response. I can't make the examples in the issue fit with the mutation and optimistic response you've provided, so I assume that the fields that are being set to null are firstName and school on the User (rather than birthday). I would expect it to be a fair assumption that the optimistic response is a complete response to the operation you're performing. I.e., that it has all fields specified by the operation. Given this assumption, it is fair for the normalize library to assume that any omitted data is null (or actually throw a partial data exception). I.e., I wouldn't be surprised to find firstName and school to be set to null.

Assuming that my understanding of your problem is correct, a work-around would be to use the cache access methods to update the cache before the mutation. In your example, you could use the GraphQLClient.writeFragment to update the field:

fragment UpdateFriendStatusOnUser on User {
  friendStatus
}

[kvenn]

we shouldn't be writing the optimistic result to the cache for the reasons mentioned in the apollo docs

Very much agree the Apollo implementation makes the most sense (since it supports undoing errored mutations).

Just to confirm, this is how the library works now, though, right? Via _getOptimisticQueryResult->attemptCacheWriteFromClient

I can't make the examples in the issue fit with the mutation and optimistic response you've provided

Oops that's my bad, I provided a fake example in my first message for simplicity. I just updated my original post and my response to more accurately represent what I'm seeing. But it's exactly as you said - I'm providing a partial object in the optimistic response and it's overwriting all omitted values in cache. In my updated example, name is being set to null.

I would expect it to be a fair assumption that the optimistic response is a complete response to the operation you're performing

I would have assumed the opposite (considering Apollo and this library support partial responses from network, therefore partial model merging is supported). But in doing some Googling, it looks like Apollo also doesn't support this, so I'll take it my assumption is just wrong. I'd still personally love it as a feature if it supported partials (since most of my mutations modify only a single field on a much larger object). But without that feature, I'm cool with finding a suitable workaround.

Assuming that my understanding of your problem is correct, a work-around would be to use the cache access methods to update the cache before the mutation

So instead of using optimisticResult, you're suggesting I just do the cache update myself via writeFragment. So something like

final fragmentDoc = gql(
  r'''
  fragment UpdateFriendStatusOnUser on User {
    friendStatus
  }
  ''',
);

var fragmentRequest = FragmentRequest(
  fragment: Fragment(
    document: fragmentDoc,
  ),
  idFields: {'__typename': 'User', 'id': 123},
);

final data = client.readFragment(fragmentRequest);
// QUESTION: THE PART WHERE I DO THE OPTIMISTIC CHANGE?
data.friendStatus = 'SEND_PENDING';
client.writeFragment(fragmentRequest, data);

Does this seem right?

The other alternative I could see would be reading the full fragment of my request from cache (similar to above, but with the full fragment I provided UserWithFriendship). But then instead of calling writeFragment directly, I provide data to the the optimisticResult of my mutation. So effectively doing the partial model merging myself.

Would both approaches handle updating all the other listeners I have on my streams?

The biggest negative to both of these workarounds is it's a lot of boiler plate and I have a lot of mutations and a lot of models. But I could always find a way to abstract it.

P.S. Thank you for your reply :) And this awesome library.

[UPDATE] Looks like our codegen library supports this cache read and partial overwrite with copyWith

From their docs:

main () {
  // ...
  final person = client.readFragmentPersonSummary(
    idFields: {'__typename': 'Person', 'id': '1'},
  );
  assert(person != null);
  client.writeFragmentPersonSummary(
    idFields: {'__typename': 'Person', 'id': '1'},
    data: person.copyWith(name: 'Kurt'),
  );
}

[budde377]

Just to confirm, this is how the library works now, though, right? Via _getOptimisticQueryResult->attemptCacheWriteFromClient

It is my understanding that the current implementation is writing the optimistic result to the cache. I might be wrong though.

The other alternative I could see would be reading the full fragment of my request from cache (similar to above, but with the full fragment I provided UserWithFriendship). But then instead of calling writeFragment directly, I provide data to the the optimisticResult of my mutation. So effectively doing the partial model merging myself.

This sounds like a good approach 🚀

[vincenzopalazzo]

Ops! Yes, we write to the cache the result, I was looking in the wrong place, sorry!

Also, I think that the approach suggested by @budde377 is the correct one, from the apollo spec I can understand that the store in the cache with a separate key the optimistic result, let's say "1234_optimistic", the query is run, and the result is compared with the optimistic one.

In this way, we can check if the optimistic result is correct.

On the other hand, I assume that from a library viewpoint we can accept a partial object given by the user, and merge this object with what we have in the cache after we run the optimistic process.

What do you think? sound good to you? maybe with some flag like "partialObj: true" to have the possibility to skip the emerging that can take some time!

[budde377]

@vincenzopalazzo, Without having looked too much into this, my thoughts are:

• Separate libraries handle caching, so if this is not already supported (which it might be), it will require significant effort to implement.
• There are some non-trivial questions around merging. What if the optimistic response is performed on an empty cache? How do we make sure the behavior is consistent? How do we do this without breaking serializers depending on output shape?
• With these efforts taken into consideration, how big is this problem? I.e., how many users are affected, approximately? I.e., if apollo hasn't implemented it, maybe the current implementation is a fair assumption with the fewest exceptions. I'm worried that we spend too much effort complicating a design that might just need better documentation.