https://kotlinlang.org logo
#apollo-kotlin
Title
# apollo-kotlin
w

wasyl

11/22/2023, 7:04 PM
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:
Copy code
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?
m

mbonnin

11/22/2023, 7:50 PM
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
w

wasyl

11/22/2023, 8:02 PM
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
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
m

mbonnin

11/22/2023, 8:07 PM
I'd say that would qualify as a cache hit
null
in the JSON is a value (unless there was an associated errors but that's opening another door)
m

mbonnin

11/22/2023, 8:09 PM
Yep
w

wasyl

11/22/2023, 8:09 PM
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
😄 1
m

mbonnin

11/22/2023, 8:10 PM
Error + nullability can become hairy quite quickly. But I think we do not store fields with errors by default (yep, it's done here)
w

wasyl

11/22/2023, 8:13 PM
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
😃 1
m

mbonnin

11/22/2023, 8:13 PM
🙂 yea, in that case we're storing
null
in places where we shouldn't
w

wasyl

11/22/2023, 8:14 PM
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
👍 1
m

mbonnin

11/22/2023, 8:16 PM
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
I think this is a reasonnable behaviour
w

wasyl

11/22/2023, 8:25 PM
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
Copy code
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)
}
m

mbonnin

11/22/2023, 8:26 PM
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)
Oh wait sorry, it is both
You'll need to extend from
CacheResolver
and implement
resolveField
completely (including the list handling provided by
CacheKeyGenerator
if you're using it)
👍 1
w

wasyl

11/23/2023, 5:03 PM
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)
Copy code
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
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
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?
m

mbonnin

11/23/2023, 5:16 PM
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
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
w

wasyl

11/23/2023, 5:30 PM
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 😄
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
m

mbonnin

11/23/2023, 5:35 PM
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
w

wasyl

11/23/2023, 5:40 PM
🤔 oh yeah, the lists stuff is something that we definitely use, is there a way to do that declaratively?
m

mbonnin

11/23/2023, 5:41 PM
Sadly not 😞 .
Maybe
Copy code
extend type Query @fieldPolicy(forField: "books", keyArg: "id", dimension: 1)
But that's quite a mouthful
w

wasyl

11/23/2023, 5:51 PM
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?
m

mbonnin

11/23/2023, 6:01 PM
Yep, there's always the "build your own" solution