apollographql / apollo-kotlin

:rocket:  A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.75k stars 651 forks source link

CacheKeyResolver fromFieldArguments does not work #2233

Closed shasharm closed 4 years ago

shasharm commented 4 years ago

Creating a duplicate issue of https://github.com/apollographql/apollo-android/issues/821 as I cannot reopen it.

I am struggling for months and finally, I found the upper mentioned issue that it is actually an issue with the library and I wonder why it wasn't reopened.

Following is my CacheKeyResolver

override val cacheKeyResolver: CacheKeyResolver = object : CacheKeyResolver() {
        override fun fromFieldRecordSet(field: ResponseField, recordSet: MutableMap<String, Any>): CacheKey {
            val id: String? = recordSet["id"]?.toString()
            val typename: String? = recordSet["__typename"] as String
            return formatCacheKey(id, typename)
        }

        override fun fromFieldArguments(field: ResponseField, variables: Operation.Variables): CacheKey {
            return formatCacheKey(field.resolveArgument("id", variables)?.toString(),
                    field.fieldName())
        }

        private fun formatCacheKey(id: String?, typeName: String?): CacheKey {
            if (id == null || typeName == null) {
                return CacheKey.NO_KEY
            }
            return CacheKey.from("$typeName:$id")
        }

    }

For the query :

query profileInfo {
    me {
        person {
            id
            __typename
            firstName            
            lastName
            organizations {
                id                
                brandSettings {
                    __typename
                    communityLogo
                    }
                }
            }
        }
    }
}

the key is resolved to CacheKey.NO_KEY and the caching works fine.

But for the queries that resolve the key as a pair of __typename and id, the cache is a miss, and response always fetched from the network. Of course, fromFieldArguments and formatCacheKey resolve a valid and same key.

I am using ApolloResponseFetchers.CACHE_FIRST for both the queries.

API version: graphQL_runtime_version = '1.2.1' implementation "com.apollographql.apollo:apollo-runtime:$graphQL_runtime_version" implementation "com.apollographql.apollo:apollo-rx2-support:$graphQL_runtime_version" implementation "com.apollographql.apollo:apollo-android-support:$graphQL_runtime_version"

shasharm commented 4 years ago

FYI, one of the queries that always fetch the response from the network is:

query featuredContent($orgId: Int!) {
    organization(id: $orgId) {
        id
        __typename
        homepageSettings {
            __typename
            id
            heroes {
                __typename
                ...ContentCardOrgPage
            }
        }
    }
}

fragment ContentCardOrgPage on Content {
    ... on Story {
        __typename
        id
        ...StoryCardOrgPage
    }
}

fragment StoryCardOrgPage on Story {
    tag: __typename
    id
    __typename
    name
    subheading
}
tasomaniac commented 4 years ago

Thanks for bringing this to our attention again. Let's try to fix it. I noticed you are using an older version. Can you try with latest 1.4.x or 2.0.1?

shasharm commented 4 years ago

Thanks for responding @tasomaniac, I prefer upgrading to 2.0.1, but as per the release notes of 2.0.1, there will be many network layer changes that I could pick up later after a few weeks. So, let's keep this bug open and I will get back with the status on caching when I have upgraded.

tasomaniac commented 4 years ago

Can you try with 1.4.5?

shasharm commented 4 years ago

I just did try with 1.4.5 but while generating the models, I have the following error:

error: cannot find symbol
import com.apollographql.apollo.api.FragmentResponseFieldMapper;
                                   ^
  symbol:   class FragmentResponseFieldMapper
  location: package com.apollographql.apollo.api
tasomaniac commented 4 years ago

We unfortunately had a breaking change within minor releases. It was too late when we noticed it. You just need to re-import

shasharm commented 4 years ago

I am not sure what are you asking to reimport? I am getting this error in a generated query model. build/generated/source/apollo/classes/org/salesforce/pcapollo/graphql/ContentDetailsVolunteeringCampaignQuery.java

Do I need to change any of these?

    implementation "com.apollographql.apollo:apollo-runtime:$graphQL_runtime_version"
    implementation "com.apollographql.apollo:apollo-rx2-support:$graphQL_runtime_version"
    implementation "com.apollographql.apollo:apollo-android-support:$graphQL_runtime_version"
tasomaniac commented 4 years ago

Hmm, that would mean that you are using different version of Apollo somewhere. Are you sure all versions are the same?

shasharm commented 4 years ago

Yes this is the only module we have Apollo implementation in.

shasharm commented 4 years ago

Hey @tasomaniac !

I just updated to version 2.0.3 but the same issue persists. The key is being returned by fromFieldRecordSet but when I re-try a request, fromFieldArguments is hit but due to cache miss, network call is being made and again lands in fromFieldRecordSet.

I am not sure whether it's an issue with writing the keys, or reading them. Is the normalized caching not supported for some response structures?

Appreciate your help.

tasomaniac commented 4 years ago

Will need to have a deeper look into what's going on here.

shasharm commented 4 years ago

Thanks for the response @tasomaniac. What information would you like to know more about?

This is another query for which caching doesn't work:

query search($orgId: Int!, $input: SearchInput!) {
  organization(id: $orgId) {
    id
    name
    search(input: $input) {
      metaData {
        queryId
        __typename
        algorithmName
      }
      pagination {
        limit
        offset
        totalCount
        __typename
      }
      results {
        id
        typename
        name
        address {
          locality
          country
          __typename
        }
        shifts {
          type
          startDateTime {
            dateTime
            __typename
          }
          endDateTime {
            dateTime
            __typename
          }
          __typename
        }
        __typename
      }
      __typename
    }
    __typename
  }
}

Query variables:

{
  "orgId": 123,
  "input": {
    "personId": 345,
    "types": {
      "type1": false,
      "type2": true
    },
    "filters": {
      "keywords": "",
      "location": {
        "longitude": <lat>,
        "latitude": <lng>      
      }
    },
    "pagination": {
      "offset": <int>,
      "limit": <int>
    }
  }
}

I am chaining the in-memory and disk cache:


fun getOptimalCacheSize(): Long {
        return Runtime.getRuntime().maxMemory() / 1024 / 8
    }

val inMemoryNormalizedCacheFactory: LruNormalizedCacheFactory by lazy {
        LruNormalizedCacheFactory(EvictionPolicy.builder()
                .maxSizeBytes(getOptimalCacheSize())
                .expireAfterAccess(CACHE_VALIDITY, TimeUnit.DAYS)
                .build())
    }

val diskNormalizedCacheFactory = SqlNormalizedCacheFactory(applicationContext, DISK_CACHE_SQLLITE_DB_NAME)

val memoryFirstThenSqlCacheFactory: NormalizedCacheFactory<LruNormalizedCache> by lazy {
        inMemoryNormalizedCacheFactory.chain(diskNormalizedCacheFactory)
    }
martinbonnin commented 4 years ago

@shasharm my first guess would be that there is no way to retrieve results.id from your query variables. So the cache stores the results object at key results.key but later on, there's no way to retrieve this object.

What you can try it:

        override fun fromFieldRecordSet(field: ResponseField, recordSet: MutableMap<String, Any>): CacheKey {
            val id: String? = recordSet["id"]?.toString()
            val typename: String? = recordSet["__typename"] as String
            if (typename == "Results") {
                return CacheKey.NO_KEY
            }
            return formatCacheKey(id, typename)
        }
shasharm commented 4 years ago

Great suggestion @martinbonnin ! I tried with it but still is a cache miss. The organization being the root is resolved first in fromFieldArguments with a key id:organization while reading from cache but that's the only point fromFieldArguments is called and looks like it is a cache-miss and then the network call is made. It appears to me that fromFieldRecordSet is not storing the cache with the provided keys and all resolutions with the custom key lead to a cache-miss.

shasharm commented 4 years ago

FYI, I just tried with only in-memory cache

val inMemoryCacheFactory = LruNormalizedCacheFactory(EvictionPolicy.builder().maxSizeBytes(10 * 1024).build())

val apolloClient: ApolloClient by lazy {
        ApolloClient.builder()
                .serverUrl(getGQLEndPoint())
                .okHttpClient(okHttpClient)
                .dispatcher(requestDispatcher.getExecutorService())
                .normalizedCache(
                       inMemoryCacheFactory,
                       cacheKeyResolver)
                .addCustomTypeAdapter(CustomType.LOCALECODE, CustomResponseFieldAdapter())
                .addCustomTypeAdapter(CustomType.CURRENCYCODE, CustomResponseFieldAdapter())
                .build()
    }

and it still has the same behavior.

martinbonnin commented 4 years ago

I think I got it! Assuming your organisation has id 123, your cache keys will be:

fromFieldRecordSet => "Organization:123"
fromFieldArguments => "organization:123"

One is uppercase, the other lower case because in the first case it refers to the type name and in the other the field name. The field name could theorically be something completely different like company or foobar and still be of type Organization.

If you want to namespace your ids, you'll need to have access to the __typename from fromFieldArguments but I'm not sure if this is possible at the moment.

You could make a mapping from organization to Organization but that might not be super scalable.

shasharm commented 4 years ago

YES!! That's the issue here. And I agree with you that using fieldname is not super scalable. I tried with the lowercase keys and it works but not for all queries as fieldName is different than __typename and I don't see any other option than field name for fromFieldArguments. I think it'll be good to consider in the future about building consistent keys in CacheKeyResolver. Thanks for the help @martinbonnin and @tasomaniac!