Apollo-client: Client V3 - TypePolicy Results in DetailView Result being Cached Without ID

[jamshally]

A related, but different, issue to #5709

I have a TypePolicy reference UserListView from a UserDetailView Query, as below:

export const UserTypePolicy: TypePolicies = {
        Query: {
          fields: {
            User_by_pk(existingData, { args, toReference }) {
              return existingData || toReference({ __typename: 'User', id: args!.id });
            },
          },
        },
      };

I execute a User DetailView query on a user that does not already exist in cache.

Intended outcome:
I would expect the following to end up in cache:

User_by_pk({"id":"88cfac84-e76a-4920-a7b4-c5d32b266f87"}): {"__ref":"User:88cfac84-e76a-4920-a7b4-c5d32b266f87"}

Actual outcome:

The following entry ends up in Cache:
User_by_pk: {"__ref":"User:88cfac84-e76a-4920-a7b4-c5d32b266f87"}

This causes several problems including, the existingData field for every hit to the above TypePolicy returning this single User entity, not matter what the actual ID.

How to reproduce the issue:

1. Setup TypePolicy as above
2. Execute a ListView Query to seed cache
3. Execute an Insert to create a new item (that will not be in cache)
4. Execute a Detail View Query
5. Observer the resulting cache

Versions
System:
OS: macOS 10.15.2
Binaries:
Node: 12.12.0 - /usr/local/bin/node
Yarn: 1.19.1 - /usr/local/bin/yarn
npm: 6.9.0 - /usr/local/bin/npm
Browsers:
Chrome: 79.0.3945.88
Safari: 13.0.4
npmPackages:
@apollo/client: ^3.0.0-beta.10 => 3.0.0-beta.16
@apollo/react-hooks: ^3.2.0-beta.0 => 3.2.0-beta.0
apollo-link-debounce: ^2.1.0 => 2.1.0

[benjamn]

Ahh, I think you are being bitten by the consequences of #5680, which introduced the breaking change (sorry) that any field with a read function (like your User_by_pk field) will have keyArgs: false by default, since we think that's _usually_ the desired behavior.

However, you can still override this default behavior by specifying keyArgs explicitly:

typePolicies: {
  Query: {
    fields: {
      User_by_pk: {
        keyArgs: ["id"],
        read(existingData, { args, toReference }) {
          return existingData || toReference({ __typename: 'User', id: args!.id });
        },
      },
    },
  },
}

This means the args.id property will be incorporated into the field name, as you expected. You didn't have to do this before #5680, but now you do. I'm open to debating the relative merits of different default behaviors, but I think this workaround should cover your use case?

[jamshally]

This workaround should cover your use case

Yep, that sorted it. Thanks!

I'm open to debating the relative merits of different default behaviors

I don't feel well placed to weigh in on the debate of what is best for most people. However, in case these are useful datapoints:

1. I am using a Hasura backend, which autogenerates an '_by_pk' query for every Entity - so this use case will come up a lot
2. Without a TypePolicy, the 'entity_by_pk' queries would cache with a keyArg (as I was expecting). With a TypePolicy it does not. So, _adding a TypePolicy changes the behavior of something only tangentially related (KeyArgs)_. This may catch a few people out
3. I am new to GraphQL and Apollo, so the above may be wrong-headed for reasons I am not yet aware of

Thanks for taking the time to respond

[benjamn]

Thanks, those are useful data points!

One of the goals of type policies and field policies is that it should be easy to write helper functions for common patterns, so you could do something like

function makeByPkFieldPolicy(__typename: string) {
  return {
    keyArgs: ["id"],
    read(existingData, { args, toReference }) {
      return existingData || toReference({ __typename, id: args!.id });
    },
  };
}

const cache = new InMemoryCache({
  typePolicies: {
    Query: {
      fields: {
        User_by_pk: makeByPkFieldPolicy('User'),
        Product_by_pk: makeByPkFieldPolicy('Product'),
        // and so on...
      },
    },
  },
});

[jamshally]

Thanks @benjamn - that helper function capability is very nice. I'll use that code in a apollo-hasura cogen library I am authoring. Sharing the link in-case a useful as a reference point.

[jamshally]

@benjamn - I have a further question related to the above example, if you don't mind:

Subsaquent to your suggested addition of keyArgs, the followingTypePolicy is working well as 'cache-redirect' substitue:

typePolicies: {
  Query: {
    fields: {
      User_by_pk: {
        keyArgs: ["id"],
        read(existingData, { args, toReference }) {
          return existingData || toReference({ __typename: 'User', id: args!.id });
        },
      },
    },
  },
}

Now I need to add an additional typePolicy on the referenced type (note Query.fields.User type this time):

typePolicies: {
  Query: {
    fields: {
      User: {
        keyArgs: ["id"],
        read(existingData, { args, toReference }) {
          console.log('User TypePolicy hit!');
          return existingData;
        },
      },
    },
  },
}

However, this type policy does not seem to get triggered after the initial User_by_pk reference/redirect. I have tried with and without the keyArgs parameter. Am I missing something, or is this as/designed or a bug?

Additional Context
My goal with the second typePolicy is to run a check on the User type for stale data in some @client fields (and clear as needed).

Thanks!

[benjamn]

@ahrnee Since User is a type, its policy should appear at the same level of nesting as the Query type:

typePolicies: {
  Query: {
    fields: {
      User_by_pk: {
        keyArgs: ["id"],
        read(existingData, { args, toReference }) {
          return existingData || toReference({ __typename: 'User', id: args!.id });
        },
      },
    },
  },
  User: {
    keyFields: ["id"],
    fields: {
      // field policies for User would go here
    },
  },
}

Note that keyFields is used instead of keyArgs for identifying typed objects. In this case, keyFields: ["id"] is correct, but you could avoid specifying that information here, because keyFields: ["id"] is the default behavior. Also, there's no way to define a read function for the whole User object, though you could define read functions for individual fields within User.

[jamshally]

@benjamn - OK, thanks. Unfortunatly that is not quite what I am looking for. I am trying to _inspect all the fields on a type_ on every query. Maybe this is not the correct use of TypePolicies.

I am really just looking for a place to run the following logic on every Query of the User type:

...

if( user.clientOnlyObject == null || 
    user.lastUpdated < user.clientOnlyObject.last_generated ) {

  // generate new clientOnlyObject and add to user object in cache

}

I am using a Resolver for the initial generation of the ClientObject, but this doesn't seem to get triggerd for every subsequent query, which was why I was turning to TypePolicies

[benjamn]

You could check those conditions whenever you're reading the clientOnlyObject field of a User object:

typePolicies: {
  User: {
    keyFields: ["id"],
    fields: {
      clientOnlyObject(coo, { readField }) {
        if (coo == null || readField("lastUpdated") < coo.last_generated) {
          // generate new clientOnlyObject and update coo before returning
        }
        return coo;
      },
    },
  },
}

The one bit I'm not sure about is writing to the cache as a side-effect of reading from the cache, especially when you're writing the very field that you're reading. Have you found a good way to do that?

[jamshally]

The one bit I'm not sure about is writing to the cache as a side-effect of reading from the cache

No, I haven't. It was only after further testing that I understood your point here. And, as you indicated, there is not a good way to persist the generated value back to cache at this point. As far as I can tell, the cache object is not exposed through the TypePolicy methods. I think there is a case for this, but I'll write up into a separate ticket, as this has strayed fairly far from the original ticket. (done)

[benjamn]

Note that @apollo/[email protected] contains #5862, which refines the assumption introduced by #5680 in a way that might have prevented this issue.