Fix rewinding the Json stream when lists are involved

[martinbonnin]

Many thanks again to @mateuszkwiecinski for catching this.

Now we don't try to write the lists/objects in place and instead merge when emitting a value. It's much more easier to reason about it this way.

See #3672

[BoD]

💪

[mateuszkwiecinski]

I'll just mention that I think I made a mistake when defining the IdBasedCacheKeyResolver and AFAIU the implementation I provided might not work in an expected way.
According to how I understand CacheResolver I should have used CacheKeyResolver as the base class and define the

  override fun cacheKeyForField(field: CompiledField, variables: Executable.Variables): CacheKey? =
    (field.resolveArgument("id", variables) as? String)?.let(::CacheKey)

(that's what the migration guide suggests)

I see current form passed the tests, can you help me understand why does it work? Doesn't the default cache resolver expect cache keys in "$typename:$id" format?

[martinbonnin]

cacheResolver isn't strictly needed to deduplicate your data. It's only helpful to save a network roundtrip if you know a field is in the cache and you can get its id from the arguments:

query GetAllBooks {
  books {
    id
    title
    author {
      name
    }
  }
}

query GetBookById {
  book(id: "42") {
    title
  }
}

In the above case, if you do a AllBooksQuery first, you can use cacheResolver to avoid a network request when executing GetBookByIdQuery.

Because the test uses only one query, what cacheResolver isn't important. Even if it doesn't find an id, the cache will use the path as cache key and the path is always the same because there is only one query.

Doesn't the default cache resolver expect cache keys in "$typename:$id" format?

The cache in general has no expectation about the format of cache keys. The only important thing is that they are unique and that cacheResolver and objectIdGenerator return consistent cache keys.

All in all, cacheResolver is a lot less important than objectIdGenerator. While you need the later, most apps will work perfectly fine and deduplicate data without a cacheResolver

[martinbonnin]

Also, I realize this API is not easy. There is some documentation about it but even with that, it's not easy to explain and it remains complex. If you have any idea how to rephrase/improve this, this is very welcome.

Ultimately, some kind of visual cache viewer or so would be super helpful ... but it's a long way 😅

[mateuszkwiecinski]

Thanks for the explanation ❤️ All clear now!