Caching for FDC

[maneesht]

No description provided.

[aashishpatil-g]

Is the response object meant to be exported or is it for internal SDK use?

[aashishpatil-g]

Should we remove the default here?

[aashishpatil-g]

i.e. maxAge = 30

[aashishpatil-g]

maxAge should be zero by default.

[stephenarosaj]

comments resolved - LGTM (waiting to approve until other comments resolve)

[stephenarosaj]

all comments resolved - LGTM

[aashishpatil-g]

I am logging and storing the original JSON for this case with either entity (similar to empty erray case) - https://github.com/firebase/flutterfire/pull/17988/changes#diff-b09e0ad5accf5925eb2fa32291b9d398531917c1b3f711c4cc3a181eedb6acc5R104

[aashishpatil-g]

This lets us reproduce the original query when hydrating.

[aashishpatil-g]

Do we need this?

[aashishpatil-g]

LGTM pending the comments.

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive caching layer for Data Connect, which is a significant and well-executed feature. The architecture is robust, handling entity normalization, cache invalidation on user changes, and providing different fetch policies. The code is well-structured with good separation of concerns and is accompanied by extensive tests. I've identified one high-severity issue with the implementation of the CACHE_ONLY fetch policy, where it incorrectly falls back to a network request. A code suggestion to fix this is provided. Overall, this is an excellent contribution.

[gemini-code-assist]

The current implementation of fetch policies doesn't correctly handle CACHE_ONLY. If an item is not in the cache or is stale, it sets shouldExecute = true and proceeds to make a network request. This violates the contract of a CACHE_ONLY policy, which should only ever read from the cache and never trigger a network fetch.

I suggest explicitly handling the CACHE_ONLY policy to prevent network requests and to throw an error if the requested data is not found in the cache. This will make the behavior of the fetch policies predictable and correct.

    if (options?.fetchPolicy === QueryFetchPolicy.SERVER_ONLY) {
      shouldExecute = true;
    } else if (options?.fetchPolicy === QueryFetchPolicy.CACHE_ONLY) {
      if (cachingEnabled) {
        if (await this.cache!.containsResultTree(key)) {
          queryResult = await this.getFromResultTreeCache(key, queryRef);
        }
      } else {
        const fromSubscriberCache = await this.getFromSubscriberCache(key);
        if (fromSubscriberCache) {
          queryResult = fromSubscriberCache as QueryResult<Data, Variables>;
        }
      }
      if (!queryResult) {
        throw new DataConnectError(
          Code.OTHER, // Consider a more specific error code like 'cache-miss'.
          'Query result not found in cache for CACHE_ONLY policy.'
        );
      }
    } else {
      // Default behavior for PREFER_CACHE or undefined
      if (cachingEnabled) {
        if (
          !(await this.cache?.containsResultTree(key)) ||
          (await this.cache?.getResultTree(key))!.isStale()
        ) {
          shouldExecute = true;
        } else {
          queryResult = await this.getFromResultTreeCache(key, queryRef);
        }
      } else {
        // read from subscriber cache.
        const fromSubscriberCache = await this.getFromSubscriberCache(key);
        if (!fromSubscriberCache) {
          shouldExecute = true;
        }
        queryResult = fromSubscriberCache as QueryResult<Data, Variables>;
      }
    }