I encountered an unexpected scenario and wanted to...

# apollo-kotlin

[wasyl]

I encountered an unexpected scenario and wanted to run it by you to make sure my understanding is correct. Suppose we have a query and @fieldPolicy as in the documentation:

type Query {

  book(id: String!): Book

}
# ...
extend type Query @fieldPolicy(forField: "book", keyArgs: "id")
extend type Book @typePolicy(keyFields: "id")

Note that the `book`'s result is optional. It's possible to get into a state where two observers for the same query have two different results, if:
• the backend returns a response with book ("book": { ... })
• a second observer fetches the same thing from the network but this time the server doesn't return a book
• the first original observer will not be updated with null book. The second will emit data with null
This is both surprising and not — on one hand, it seems reasonable that fields with @fieldPolicy should always return an item if it's found in the cache. On the other hand, feels like two observers observing the same query should always have the values in sync. Seems to me like reading from cache goes through a different code path than deserializing the json — one returns null just like in the response, the other returns an object since @fieldPolicy points to an object that exists in the cache.

Is this just incorrect usage of @fieldPolicy? That is, should it only be used if the field can't change from non-null to null?

[mbonnin]

Mmmm interesting case 👀
I wonder if it's just a matter of changing the order here: if we have a value in the record already take it, if not, go through @fieldPolicy

[wasyl]

Actually the original repro in my project is using programmatic field policy, not sure if it changes anything. I think our implementation is similar to the documentation (basically a catch-all for all fields with id argument 😬 which is not great now that I think of it)

But yeah I'd expect the record here to take precedence, since afaiu it's closer to whatever the server has returned last. I'm still not clear if using @fieldPolicy in this case is allowed in the first place, perhaps there's a hard requirement that the value doesn't disappear. It is kind of tricky though in case of data that does in fact disappear over time, given that there's no (?) pruning of unreachable objects

[wasyl]

I wonder if it's just a matter of changing the order here: if we have a value in the record already take it, if not, go through @fieldPolicy

That'd work only if DefaultCacheResolver.resolveField(field, variables, parent, parentId) distinguishes between null and absent, right? I mean, in the specific scenario I linked, there exists an entry in the cache for book(someId): null but the value is null. Not sure that'd qualify as a cache hit

[mbonnin]

I'd say that would qualify as a cache hit

[mbonnin]

null in the JSON is a value (unless there was an associated errors but that's opening another door)

[wasyl]

Right, I see here 🙂 https://github.com/apollographql/apollo-kotlin/blob/8fec9a54715b827255cbaa7324f9b9902b6a40bf/libraries/apollo-normalized-cache-api/src/commonMain/kotlin/com/apollographql/apollo3/cache/normalized/api/CacheResolver.kt#L94-L99

[mbonnin]

Yep

[wasyl]

unless there was an associated errors but that's opening another door

funny you should say that, the original-original case that I started with involved the field being null because there were associated errors 😄 But then I realized we may not use field policy correctly so I focused on that

[mbonnin]

Error + nullability can become hairy quite quickly. But I think we do not store fields with errors by default (yep, it's done here)

[wasyl]

Guess who requested to be able to store responses with errors in the first place and is still doing that 😄 https://github.com/apollographql/apollo-kotlin/issues/2358

[mbonnin]

🙂 yea, in that case we're storing null in places where we shouldn't

[wasyl]

Yeah in any case I wouldn't focus on that, storing partial responses is iffy and eventually I want to get rid of that at some point

[mbonnin]

Want to copy/pasta the FieldPolicyCacheResolver with the logic inverted (only lookup by id arg if the record doesn't already contain something)? If working well, we could make it the default in the lib

[mbonnin]

I think this is a reasonnable behaviour

[wasyl]

will try later today/tomorrow 👀 I'd paste it in place of where my current implementation would return null or replace the current one completely? 🤔 The current one is

internal object IdBaseCacheKeyGenerator : CacheKeyGenerator, CacheKeyResolver() {

    private const val FIELD_ID = "id"
    private const val FIELD_IDS = "ids"

    override fun cacheKeyForObject(
        obj: Map<String, Any?>,
        context: CacheKeyGeneratorContext,
    ) = obj[FIELD_ID]?.asCacheKey() ?: TypePolicyCacheKeyGenerator.cacheKeyForObject(obj, context)

    override fun cacheKeyForField(
        field: CompiledField,
        variables: Executable.Variables,
    ): CacheKey? = field.resolveArgument(FIELD_ID, variables)?.asCacheKey()

    override fun listOfCacheKeysForField(
        field: CompiledField,
        variables: Executable.Variables,
    ): List<CacheKey?>? {
        val ids = field.resolveArgument(FIELD_IDS, variables)

        return if (ids is List<*>) {
            ids.map { it?.asCacheKey() }
        } else {
            null
        }
    }

    private fun Any.asCacheKey() = (this as? String)
        ?.takeUnless(String::isBlank)
        ?.let(::CacheKey)
}

[mbonnin]

This is the CacheKeyGenerator , you need to change the CacheResolver (I know this is confusing but... didn't really find better names, this stuff is confusing)

[mbonnin]

Oh wait sorry, it is both

[mbonnin]

You'll need to extend from CacheResolver and implement resolveField completely (including the list handling provided by CacheKeyGenerator if you're using it)

[wasyl]

Ok, I think I got it. So first of all, I took not the FieldPolicyCacheResolver because I (sorry!) used @fieldPolicy annotation in the snippet but in the code I was using a custom CacheKeyResolver. So I copied CacheResolver#resolveField instead, and I made it so that the abstract fun cacheKeyForField is called only after the default resolver doesn't find anything. I did it only for non-list types too just for PoC purposes (img with diff for easier comparison)

override fun resolveField(
    field: CompiledField,
    variables: Executable.Variables,
    parent: Map<String, @JvmSuppressWildcards Any?>,
    parentId: String,
): Any? {
    var type = field.type
    if (type is CompiledNotNullType) {
        type = type.ofType
    }
    if (type is CompiledNamedType && type.isComposite()) {
        val result = try {
            return DefaultCacheResolver.resolveField(field, variables, parent, parentId)
        } catch (ex: CacheMissException) {
            cacheKeyForField(field, variables)
        }
        if (result != null) {
            return result
        }
    }

    if (type is CompiledListType) {
        type = type.ofType
        if (type is CompiledNotNullType) {
            type = type.ofType
        }
        if (type is CompiledNamedType && type.isComposite()) {
            val result = listOfCacheKeysForField(field, variables)
            if (result != null) {
                return result
            }
        }
    }

    return DefaultCacheResolver.resolveField(field, variables, parent, parentId)
}

With this I observe expected result:
• type policy is respected, I don't observe network request when making a query with type policy for object that is in the cache already
• if the backend returns an object and then later null, I get the null in subsequent observers
• I still get the object when making another query with fieldPolicy, because it's in the cache

[wasyl]

That makes sense, right? Only it means the behavior would need to be changed in more than one place — at least FieldPolicyCacheResolver and CacheKeyResolver, but I see a number of other resolvers in the code too, and I'm not sure what they're for 😄

Good news is that the API is perfect in that we can step down and implement everything manually, adjust the behavior only in our project and we we don't need any changes in Apollo 👌 So question is whether it's something that should be upstreamed or not

[wasyl]

and as a complete side note, I'm not sure the example in the docs https://www.apollographql.com/docs/kotlin/caching/programmatic-ids/#cachekeyresolver is great — we followed a similar thing, pretty sure it was taken from some previous version of the docs, and only now I realized it's not great:
— only the declarative cache docs mentions We happen to know that this query returns whichever Book object has an id field that matches the required argument., which I think is vital — not every query satisifes this
— just copying and pasting the sample implementation seems to work for a simple case, but afaiu it breaks down if the query doesn't return an object with the id argument — say Query.nextBookRecommendationFor(id: $bookId) { book { ... } }. Perhaps it would make sense to make it more obvious, like have the sample implement an opt-in for a particular query path field only?

[mbonnin]

I think it makes a lot of sense to upstream this in FieldPolicyCacheResolver . FieldPolicyCacheResolver is the only place that uses keyArgs and I'm borderline tempted to deprecated CacheKeyResolver altogether

[mbonnin]

Re docs: agreed could definitely do a lot better. Since it's quite the advanced topic and doesn't get that many views those pages do not get a lot of love but that's no reason

[wasyl]

To be fair I was extremely confused about the topic and the declarative cache docs made great job of explaining the principle (that's why I singled out programmatic cache one). It was some effort to translate this into the resolver APIs but it was manageable. The thing with programmatic cache docs is a working-but-a-footgun snippet, which it seems I also copied to the project at some point in time 😄

[wasyl]

About CacheKeyResolver — so the only way to implement field policy would be with an extension or by manually implementing full CacheResolver, if someone understands the details and is comfortable with the APIs? That'd be a +1 from me really, because AFAIU it would make it much more difficult to implement a catch-all field policy which is rarely always correct. But I'm also somewhat sure a) someone would complain because they do in fact need a catch-all resolver, and b) extensions are more verbose

[mbonnin]

I'm not 100% sure but the whole CacheKeyResolver feels a bit asymmetrical. It's sole reason for existence is to handle lists of id arguments but then why does it stop at dimension 1. Agree there aren't many 2-dimension inputs around but still feels weird. Also requires the caller to know about keyArgs

[wasyl]

🤔 oh yeah, the lists stuff is something that we definitely use, is there a way to do that declaratively?

[mbonnin]

Sadly not 😞 .

[mbonnin]

Maybe

extend type Query @fieldPolicy(forField: "books", keyArg: "id", dimension: 1)

But that's quite a mouthful

[wasyl]

or keyArgsList if only first-order lists were supported, but that's quite limiting. On the other hand CacheKeyResolver is limiting too, and again for a rare scenario with nested lists you could implement your own CacheResolver right?

[mbonnin]

Yep, there's always the "build your own" solution