Query keys with object property set to undefined are not considered equal to missing property during invalidation

[chekrd]

Describe the bug

Query keys with object property set to undefined are not considered equal to missing property. Based on the docs, it should be the same: https://react-query.tanstack.com/guides/query-keys#query-keys-are-hashed-deterministically

Your minimal, reproducible example

https://codesandbox.io/s/react-query-trz8sx?file=/src/App.tsx

Steps to reproduce

1. Open the codesandbox link
2. Show codesandbox console
3. There are two queries rendered, the first fetch all items, and the second fetch filtered ones.
4. You can see two buttons with invalidation hooks. The first one invalidates both queries, the second one invalidates only non-filtered results. See the console for refetch logs.

Expected behavior

I expect both keys should invalidate the same records no matter of filter: undefined as mentioned in react-query docs.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

macOS v12.4
Chrome 103

react-query version

Both v3.39.1 and v4.0.0-beta.23

TypeScript version

v4.7.3

Additional context

No response

[TkDodo]

query keys are hashed deterministically, but when comparing queries, there is no hashing going on, because we have to partially compare them. For that, we use Object.keys to iterate over the objects. If you explicitly pass filter:undefined, we cannot invalidate a query that has filter: { done: true }, why should we? That filter clearly doesn't match filter: undefined.

so yeah, for partial matching, there is a difference between a property being present (independent of the value, even if it's undefined), and a property being missing.

[chekrd]

Thanks for the quick response @TkDodo.
Does it make sense to differentiate between no prop and undefined prop? It still seems to be an issue to me. I have to filter out the filter prop in the key factory if not set. I can imagine another dev is not going to cover this for different entities. It can introduce stale cache errors (as I just found out). In my example you can see I didn't set filter:undefined explicitly, I just passed down the optional argument. I don't think creating a specific key is the right way. It would introduce copy-pasted hook for the same resource, with the required filter argument.
What do you recommend in this situation?

[TkDodo]

I would probably create two different functions to build keys that compose each other:

const todoListKeys = {
  list: () =>
    [
      {
        entity: "todos",
        scope: "list"
      }
    ] as const,
  filteredList: (filter: Filter) =>
    [{ ...todoListKeys.list()[0], filter }] as const
};

[chekrd]

Solution covered by a single key

Please, take another look. I can accept your position that invalidateQueries([{ entity: "todos", scope: "list", filter: undefined }]) should find and invalidate only this explicit query key (as you have said, filter: undefined is an explicit part of the key). But because of react-query hashing, the query key [{ entity: "todos", scope: "list", filter: undefined }] is stored without the filter prop as [{ entity: "todos", scope: "list" }]. So I cannot consider [{ entity: "todos", scope: "list", filter: undefined }] as an explicit key because I have no way to create that record in the cache.

In my example, invalidateQueries([{ entity: "todos", scope: "list", filter: undefined }]) invalidates both records [{ entity: "todos", scope: "list", filter: undefined }] and [{ entity: "todos", scope: "list" }]. But because the filter: undefined part cannot exist in the cache, only all entities in scope 'list' are invalidated instead. Filtered ones (for example [{ entity: "todos", scope: "list", filter: { isDone: true }] are persisted.

In comparison with another explicit invalidation, for example invalidateQueries([{ entity: "todos", scope: "list", filter: { isDone: true } }]), only key [{ entity: "todos", scope: "list", filter: { isDone: true } }] is invalidated. All other todo lists are untouched.

So two explicit queries invalidate differently because of undefined value. Key with prop: undefined cannot be invalidated as it cannot exist in the cache, on the other hand, the same key does not invalidate all todos of scope 'list' because it is considered to be a specific key. Is this still consistent with your perspective? I don't see any usable scenario in current behavior.

Current behavior in the context of undefined prop

For keys in cache:
0: [{ entity: "todos", scope: "list" } }] (as a result of a query set for [{ entity: "todos", scope: "list”, filter: undefined } }])
1: [{ entity: "todos", scope: "list", filter: { isDone: true } }]
2: [{ entity: "todos", scope: "list", filter: { isDone: false } }]
Actions:
invalidateQueries([{ entity: "todos", scope: "list" }]) => invalidates all three records
invalidateQueries([{ entity: "todos", scope: "list", filter: { isDone: true } }]) => invalidates 1
invalidateQueries([{ entity: "todos", scope: "list", filter: {} }]) => invalidates 1 and 2
invalidateQueries([{ entity: "todos", scope: "list", filter: undefined }]) => invalidates only 0, so it behaves like invalidateQueries([{ entity: "todos", scope: "list" }], { exact: true }). Is this intended?

To have consistent behavior, react-query should do non-undefined value filtering in the comparator code before executing. Because that's exactly what seems to be happening in key hashing during record creation.

My expectation in the context of undefined prop

If react-query removes undefined values the same way as it does for cache record creation, it would be like this:
For keys in cache:
0: [{ entity: "todos", scope: "list” } }] (as a result of a query set for [{ entity: "todos", scope: "list”, filter: undefined } }])
1: [{ entity: "todos", scope: "list", filter: { isDone: true } }]
2: [{ entity: "todos", scope: "list", filter: { isDone: false } }]
Actions:
invalidateQueries([{ entity: "todos", scope: "list" }]) => invalidates all three records
invalidateQueries([{ entity: "todos", scope: "list", filter: { isDone: true } }]) => invalidates 1
invalidateQueries([{ entity: "todos", scope: "list", filter: {} }]) => invalidates 1 and 2
invalidateQueries([{ entity: "todos", scope: "list", filter: undefined }]) => invalidates all three records, because undefined is removed, so key parameter is reduced to [{ entity: "todos", scope: "list” }]

Another solution is to allow the creation of undefined prop in the cache key record, so we can invalidate specifically only [{ entity: "todos", scope: "list", filter: undefined }] record without unexpected fallback to [{ entity: "todos", scope: "list” } }] strict: true.

Solution with list and filteredList keys

Having two keys for one query useTodos(filter?: Filter) is challenging to maintain as I need a TypeScript typeguard to parse the query context in queryFn, and higher-level key factory creating the key based on useTodos params:
const getTodosQueryKey = (useTodosParams: Params): (TodoKeys['list'] | TodoKeys['filteredList']) => useTodosParams.filter ? todosKeys.filteredList(params.filter) : todosKeys.list();
The latter must be used for useQuery queryKey and in all places where I want to invalidate fetched todos or read them using getQueryState. Not nice, not terrible, still a workaround, though.

Using two separate hooks is not a solution as I cannot swap them once the user sets the filter for my component.

Other workarounds

Fallback to null object

const createTodoListKey = (filter: Filter = {}) => [{ entity: "todos", scope: "list", filter }]};
For keys in cache:
0: [{ entity: "todos", scope: "list", filter: {} }]
1: [{ entity: "todos", scope: "list", filter: { isDone: true } }]
2: [{ entity: "todos", scope: "list", filter: { isDone: false } }]
Actions:
invalidateQueries([{ entity: "todos", scope: "list" }]) => invalidates all three records
invalidateQueries([{ entity: "todos", scope: "list", filter: { isDone: true } }]) => invalidates 1
invalidateQueries([{ entity: "todos", scope: "list", filter: {} }]) => invalidates all three records

This is the closest to what I think react-query should do with prop: undefined out of the box, although losing the possibility to invalidate only all queries with filter set to any object.

Fallback to null

If we want to consider undefined as a value, there is no other way than fallback to null (what I think is the right solution for everyone who depends on it):
const createTodoListKey = (filter: Filter = null) => [{ entity: "todos", scope: "list", filter }]};
For keys in cache:
0: [{ entity: "todos", scope: "list", filter: null }]
1: [{ entity: "todos", scope: "list", filter: { isDone: true } }]
2: [{ entity: "todos", scope: "list", filter: { isDone: false } }]
Actions:
invalidateQueries([{ entity: "todos", scope: "list" }]) => invalidates all three records
invalidateQueries([{ entity: "todos", scope: "list", filter: { isDone: true } }]) => invalidates 1
invalidateQueries([{ entity: "todos", scope: "list", filter: {} }]) => invalidates 1 and 2
invalidateQueries([{ entity: "todos", scope: "list", filter: null }]) => invalidates 0

That way I can really have something like filter: undefined in the cache. It also adds granularity as I can invalidate only filtered records.

It seems like the old topic about what is undefined and null. Undefined is JS implicit fallback for something missing. Null, on the other hand, is an explicit empty value. No matter of opinion on this, react-query is not consistent in context of undefined prop.

Conclusion

Our codebase can go with a fallback to the null or null object. But we must educate our developers that undefined value during key creation can lead to app inconsistency.

[TkDodo]

The default key hashing uses JSON.stringify, and keys where the value is undefined are filtered out by this. However, you have total control over that by using your own, custom queryKeyHashFn.
If you want to make filter: undefined be serialized to filter: null, you can do that globally.

I wouldn't really know how to change the current behaviour in a non-breaking way, do you?

[TkDodo]

I'll reopen this because I think I misunderstood the situation. I don't think hashing is involved at all when doing partial comparisons. I'll look at this more closely again.

[maxandron]

Thanks for reopening. It would be my pleasure to help with this.

The current behavior is very unintuitive (it took me digging through the TanStack query source code to figure out why our queries were not invalidating as expected).
I understand, however, that simply fixing the behavior by ignoring undefined objects will be breaking.

A few ideas come to mind at the moment:

1. An opt-in option (e.g., ignoreUndefinedObjectKeys) that can be set globally on the client and will be propagated to partialDeepEqual)
2. A utility function that will take in a key and strip from it any undefined object keys

I'm guessing that the closest we can get to the comparison behavior to work like that hash key function, the better.
One note from what I see: undefined values will get serialized as null with the query hash function. This means that the fix should apply only to keys that contain objects. (though perhaps undefined values in arrays should be converted to null in that case).

[TkDodo]

first thing would be to create a failing test case. could you do that?

[maxandron]

#4616

I put the test with the queryCache tests since it seems to be the place closest to the problem

[pschaub]

I am concerned about allowing undefined states in query keys because of this open issue. Is it currently necessary to ensure that no undefined is used in query keys to avoid this issue?

There are situations where I use useQuery in a hook with parameters that could be undefined in the worst case. Currently I intercept this by setting enabled to false if a required parameter is undefined. Nevertheless, an inadvertent programming error could at some point lead to undefined ending up in the query key.

[PieterT2000]

Agree with @pschaub. Same issue here.