Added ability to directly update the Apollo cache using write methods

[Nickersoft]

A vital part of calling mutations with the Apollo libraries is the ability to update the Apollo store after a server-side update. While the Apollo iOS library possesses this functionality, it does not expose it. This PR exposes these methods to allow devs to update the store directly after mutation.

In React, the code to update the store looks something like:

update={(cache, { data: { addTodo } }) => {
  const { todos } = cache.readQuery({ query: GET_TODOS });
  cache.writeQuery({
    query: GET_TODOS,
     data: { todos: todos.concat([addTodo]) }
   });
}}
>

For iOS, this functionality now looks like:

do {
    try apollo.store.withinReadWriteTransaction { transaction in
        try transaction.update(
            query: query
        ) { (data: inout GetTodosQuery.Data) in
            data.todos?.append(addTodo)
        }
    }
} catch {
    print("An error occurred")
}

Credit to @kevbuchanan for introducing a fix for query writing in #394.

As a novice to this library, however, I am wondering what the difference between responseKeyForField and cacheKeyForField is, and whether this is the appropriate / best approach to fix the issue.

Input from repo maintainers would be very much appreciated :)

[Nickersoft]

@martijnwalraven Is this OK to be merged?

[martijnwalraven]

@Nickersoft Thanks for looking into this. Unfortunately, I don't have much time to give this the attention it deserves right now, but one thing that caught my eye is the use of responseKeyForField to get the cache key. You'll want to use cacheKeyForField there (so that also means there is no need to introduce keySelector).

The reason is that cache keys should not depend on aliases and should take argument values into account. The responseKey for all of these will be hero for example, but we need to differentiate between them when reading or writing from the cache:

query {
  hero(episode: EMPIRE) {
    name
  }
}

query {
  hero(episode: NEWHOPE) {
    name
  }
}

query {
  hero: someOtherField {
    title
  }
}

[paulmelnikow]

Hey @martijnwalraven I’ve started digging into this project and noticed there is a backlog of unreviewed contributions. A contributor on Slack suggested creating my own fork and merging some of the fixes.

Are you the only person who has push access? If a few of us want to pick this up and review each others’ work, would you be willing to give us push access? Do you have other ideas about this project might move forward?

[martijnwalraven]

@paulmelnikow I have some good news: We'll be hiring a full-time mobile library developer to get Apollo iOS in better shape. We'll be publishing a job posting next week, but if you or anyone you know is interested please get in touch! (Also see this Slack message.)

[Nickersoft]

Hey @martijnwalraven, thanks for the response! It's terrific to hear you guys will be investing more heavily in this repo, as I know myself and numerous other iOS devs here who have projects relying on this repo and the lack of support/maintenance can be troubling at times.

I'd like to pull in @kevbuchanan to see if he might be able to contribute to the conversation, as he was the one who originally wrote the code that uses responseKeyForField as opposed to cacheKeyForField. The previous implementation relied solely on cacheKeyForField, and it resulted in the cache update methods not working correctly in production (the tests were passing because they included a locally-defined cache structure, not representative of what the actual production library was creating). This issue was discussed in #357 as well as #394. From what it seemed like, Kevin wasn't 100% sure why responseKeyForField was working or if it was the best solution, but currently, it seems to be the only thing that works.

More or less just saying all this to get it written down and to further document the problem / where it's at. I'd be happy to look into producing a more appropriate solution to the problem. IIRC the actual issue is the fact the library caches entries using response keys but attempts to write/read them via cache keys. I talk about the problem in more detail here. If anyone can give me further pointers on what I can safely change without risking adverse behavior, I'll see what I can do :)

[kevbuchanan]

@Nickersoft Yeah like you said, I'm not completely sure why my fix was necessary, but it does seem to work, and has been working in our app for some time now. Best I can tell is exactly what you described:

IRC the actual issue is the fact the library caches entries using response keys but attempts to write/read them via cache keys.

[martijnwalraven]

@Nickersoft @kevbuchanan I'd hate for this PR to die! As I mentioned in
#413 (comment) however, cache reads/writes should never depend on the response key but use the cache key instead. Happy to clarify more if needed.

[Nickersoft]

@martijnwalraven I totally agree. That's what it should be, and I also don't feel comfortable having this PR merged if the code uses its current hack. Unfortunately what I noticed in my app is that the cache is actually filled with entries using the response key, not the correct cache key. So instead of the query myQuery(id: 1) being written under myQuery(id: 1), its currently written under myQuery. When the read method attempts to retrieve it, it looks under myQuery(id: 1), can't find it, and doesn't update the UI with the updated cache entry. This is why using the response key works. Hopefully what I'm saying makes sense.

Now, I've hunted through the code looking to see where the discrepancy is between the read and write ops (what could be responsible for this behavior), and can't seem to find anything. The next step would be to hook the library up to my app as a prod dependency (no Carthage) and add various breakpoints/logs to figure it out. Perhaps it would help if you could give a high-level overview of how caching is managed? The code is decently complex and it might save on debugging time to have a bit of context going in.

[martijnwalraven]

@Nickersoft Hmmm, that is surprising. The code is indeed quite complicated, so I could very well be missing something :)

Ultimately, all writes to the cache (whether from the network or by directly manipulating the store) will go through:

apollo-ios/Sources/Apollo/NormalizedCache.swift

Line 12 in f095ec2

func merge(records: RecordSet) -> Promise<Set<CacheKey>>

Which is likely:

apollo-ios/Sources/Apollo/InMemoryNormalizedCache.swift

Line 13 in f095ec2

public func merge(records: RecordSet) -> Promise<Set<CacheKey>> {

These methods deal with RecordSets, which are normalized representations of a GraphQL result. Turning a result into a RecordSet is the responsibility of https://github.com/apollographql/apollo-ios/blob/master/Sources/Apollo/GraphQLResultNormalizer.swift

That relies on info.cachePathForField and info.cachePath being set by the executor:

apollo-ios/Sources/Apollo/GraphQLExecutor.swift

Lines 201 to 205 in f095ec2

	if shouldComputeCachePath {
	let cacheKey = try firstField.cacheKey(with: info.variables)
	info.cacheKeyForField = cacheKey
	info.cachePath.append(cacheKey)
	}

Which in turn relies on

apollo-ios/Sources/Apollo/GraphQLSelectionSet.swift

Line 53 in f095ec2

func cacheKey(with variables: [String: JSONEncodable]?) throws -> String {

Hopefully this is useful in tracking down what might be going on. Let me know if you have more questions.

[kevbuchanan]

@martijnwalraven Thanks for the clarification and overview. I'll try to dig into this further in the next few days.

[kevbuchanan]

@martijnwalraven After revisiting this issue and the changes I made, I think I can explain the problem a little bit better:

Within the readWriteTransaction we call update, which reads the data from the cache, returning a GraphQLSelectionSet. In my new test case this is a HeroAndFriendsNameQuery.Data.

apollo-ios/Sources/Apollo/ApolloStore.swift

Line 214 in f095ec2

var data = try read(query: query)

The body of the update is executed on the data, and then the write is performed:

apollo-ios/Sources/Apollo/ApolloStore.swift

Line 216 in f095ec2

try write(data: data, forQuery: query)

This ultimately gets us to where the problem lies, when the GraphQLExecutor executes the static selections of the selection set against the jsonObject of the selection set, in order to generate a RecordSet to merge into the cache:

apollo-ios/Sources/Apollo/ApolloStore.swift

Lines 229 to 231 in f095ec2

	public func write(object: GraphQLSelectionSet, withKey key: CacheKey, variables: GraphQLMap? = nil) throws {
	try write(object: object.jsonObject, forSelections: type(of: object).selections, withKey: key, variables: variables)
	}

apollo-ios/Sources/Apollo/ApolloStore.swift

Lines 233 to 237 in f095ec2

	private func write(object: JSONObject, forSelections selections: [GraphQLSelection], withKey key: CacheKey, variables: GraphQLMap?) throws {
	let normalizer = GraphQLResultNormalizer()
	try self.makeExecutor().execute(selections: selections, on: object, withKey: key, variables: variables, accumulator: normalizer)
	.flatMap {
	self.cache.merge(records: $0)

The changes I've made in the PR don't change the fact that the cache key is used for cache reads and writes. It just makes this execution of a selection set against its own data use the response key, since the jsonObject of the selection set doesn't include the cache key. It appears that this may have been somewhat of an afterthought of a use case for the GraphQLExecutor:

apollo-ios/Sources/Apollo/GraphQLExecutor.swift

Line 42 in f095ec2

/// An executor is used both to parse a response received from the server, and to read from the normalized cache. It can also be configured with a accumulator that receives events during execution, and these execution events are used by `GraphQLResultNormalizer` to normalize a response into a flat set of records and keep track of dependent keys.

which may explain why this wasn't obvious.

When the executor translates the selection set into a record set, the resolved record set will contain the cache key at that point, before being merged into the cache, thus preserving the expected behavior of writing to the cache with the cache key.

apollo-ios/Sources/Apollo/ApolloStore.swift

Line 237 in f095ec2

self.cache.merge(records: $0)

I may be overlooking something here, so please let me know if this doesn't line up with your understanding. I can also try to provide a better explanation or test case if this isn't clear. But I am reasonably certain that the problem lies in the translation of the GraphQLSelectionSet data returned from the read (modified with the update function body) to a RecordSet via executing the data's own selections against the new data with the GraphQLExecutor, and not in the actual reading from or merging to the cache.

[Nickersoft]

@kevbuchanan @martijnwalraven I think your explanation may also explain what I observed tonight. In the update() code block, data (line 214) winds up basically being the exact result from a query operation. For example, if you pass in the query user(id:999), then data has the shape: [ "user": <queried data> ] (similar to how you would call data?.user? in Swift code).

apollo-ios/Sources/Apollo/ApolloStore.swift

Lines 213 to 216 in f095ec2

	public func update<Query: GraphQLQuery>(query: Query, _ body: (inout Query.Data) throws -> Void) throws {
	var data = try read(query: query)
	try body(&data)
	try write(data: data, forQuery: query)

This conflicts with the logic further down the chain. If we follow that write op (line 216), it eventually lands us in the following block:

apollo-ios/Sources/Apollo/ApolloStore.swift

Lines 233 to 242 in f095ec2

	private func write(object: JSONObject, forSelections selections: [GraphQLSelection], withKey key: CacheKey, variables: GraphQLMap?) throws {
	let normalizer = GraphQLResultNormalizer()
	try self.makeExecutor().execute(selections: selections, on: object, withKey: key, variables: variables, accumulator: normalizer)
	.flatMap {
	self.cache.merge(records: $0)
	}.andThen { changedKeys in
	if let didChangeKeysFunc = self.updateChangedKeysFunc {
	didChangeKeysFunc(changedKeys, nil)
	}
	}.wait()

Following the executor statement (235), we eventually land in the executor block:

apollo-ios/Sources/Apollo/GraphQLExecutor.swift

Lines 191 to 205 in f095ec2

	private func execute<Accumulator: GraphQLResultAccumulator>(fields: [GraphQLField], on object: JSONObject, info: GraphQLResolveInfo, accumulator: Accumulator) throws -> ResultOrPromise<Accumulator.FieldEntry> {
	// GraphQL validation makes sure all fields sharing the same response key have the same arguments and are of the same type, so we only need to resolve one field.
	let firstField = fields[0]
	var info = info
	let responseKey = firstField.responseKey
	info.responseKeyForField = responseKey
	info.responsePath.append(responseKey)
	if shouldComputeCachePath {
	let cacheKey = try firstField.cacheKey(with: info.variables)
	info.cacheKeyForField = cacheKey
	info.cachePath.append(cacheKey)
	}

This is where the cache key is calculated and we attempt to read the value from the object that has been passed down, which still only uses the user key. However, the cache key is attempting to retrieve user(id:999) from the object, which doesn't exist. This causes an error on line 214, causing the initial write operation promise to exit:

apollo-ios/Sources/Apollo/GraphQLExecutor.swift

Lines 191 to 215 in f095ec2

	private func execute<Accumulator: GraphQLResultAccumulator>(fields: [GraphQLField], on object: JSONObject, info: GraphQLResolveInfo, accumulator: Accumulator) throws -> ResultOrPromise<Accumulator.FieldEntry> {
	// GraphQL validation makes sure all fields sharing the same response key have the same arguments and are of the same type, so we only need to resolve one field.
	let firstField = fields[0]
	var info = info
	let responseKey = firstField.responseKey
	info.responseKeyForField = responseKey
	info.responsePath.append(responseKey)
	if shouldComputeCachePath {
	let cacheKey = try firstField.cacheKey(with: info.variables)
	info.cacheKeyForField = cacheKey
	info.cachePath.append(cacheKey)
	}
	// We still need all fields to complete the value, because they may have different selection sets.
	info.fields = fields
	let resultOrPromise = resolver(object, info)
	return resultOrPromise.on(queue: queue).flatMap { value in
	guard let value = value else {
	throw JSONDecodingError.missingValue
	}

Hopefully this helps to validate or further explain or illustrate the aforementioned point. There definitely seems to be a disconnect in how keys are being read and written.

[martijnwalraven]

Ah, that makes sense! makeExecutor expects to be reading from the normalized cache, and reusing that logic to read from the jsonObject representation breaks because it corresponds to a result object, not a normalized record.

While we definitely want to use responseKeyForField to read from a jsonObject, the existing logic in makeExecutor is actually unnecessary and may slow things down. The call to complete(value:) is only needed to resolve references and (potentially asynchronously) load additional records from the normalized cache.

In the jsonObject case, we can simplify this and load everything synchronously from the object itself, similar to how this is done for network responses:

apollo-ios/Sources/Apollo/GraphQLResponse.swift

Lines 21 to 23 in f095ec2

	let executor = GraphQLExecutor { object, info in
	return .result(.success(object[info.responseKeyForField]))
	}

(I think we could just inline this code into write(object:), instead of calling makeExecutor, although I can see how that gets confusing. So maybe we should inline makeExecutor into execute as well.)

[martijnwalraven]

I'd prefer not to allow users to change these objects after client creation. Could we make this private(set) public instead?

[Nickersoft]

Sure thing – I had just set them all to public for quick testing.

[Nickersoft]

@martijnwalraven So I did some experimenting and I think you may be right. I replaced:

apollo-ios/Sources/Apollo/ApolloStore.swift

Lines 233 to 235 in f095ec2

	private func write(object: JSONObject, forSelections selections: [GraphQLSelection], withKey key: CacheKey, variables: GraphQLMap?) throws {
	let normalizer = GraphQLResultNormalizer()
	try self.makeExecutor().execute(selections: selections, on: object, withKey: key, variables: variables, accumulator: normalizer)

with:

private func write(object: JSONObject, forSelections selections: [GraphQLSelection], withKey key: CacheKey, variables: GraphQLMap?) throws { 
   let normalizer = GraphQLResultNormalizer() 
   let executor = GraphQLExecutor { object, info in
      return .result(.success(object[info.responseKeyForField]))
   }
        
   executor.dispatchDataLoads = self.loader.dispatch
   executor.cacheKeyForObject = self.cacheKeyForObject

   try executor.execute(selections: selections, on: object, withKey: key, variables: variables, accumulator: normalizer)

and it seems to work, assuming that's the recycling of network request executor logic you were talking about. In regards to inlining makeExecutor()... I'm wondering what the best way to modularize this logic is. dispatchDataLoads and cacheKeyForObject, from what I can tell, must be set on the executor in order for it to function properly. The above executor declaration could perhaps somehow be extracted out and used both here and in the GraphQLResponse file.. thoughts? What if there were subclasses of the GraphQLExecutor such as GraphQLResponseExecutor and GraphQLCacheExecutor?

[martijnwalraven]

dispatchDataLoads and cacheKeyForObject shouldn't actually be needed when reading directly from a result object, so things should still work if you leave that out.

That means there isn't too much shared logic, so I think inlining makeExecutor into execute might make sense.

[martijnwalraven]

Oh wait, we do still need cacheKeyForObject to be set for the normalization to work!

[martijnwalraven]

I don't think subclasses are the answer here however, because cacheKeyForObject and dispatchDataLoads (only for the loading from cache case) need to be set from outside the executor. So inlining makeExecutor still seems like a fine solution.

[kevbuchanan]

@Nickersoft I have an updated fix for this here that can replace my two commits currently in this PR.

[Nickersoft]

Thanks, @kevbuchanan! Updated the code accordingly. @martijnwalraven please let us know what you think! Also, something I forgot I added to my fork (i.e. this PR), which isn't super relevant to but still super important:

I upgraded Starscream from 3.0.5 to 3.0.6, because 3.0.6 adds support for XCode 10 and Swift 4.2. It's a very minor change – but if you need me to break it out in another PR, I'd be happy to. That is, assuming it can be merged very quickly. My app uses Swift 4.2 and I need 3.0.6 in order for it to run.

[martijnwalraven]

@Nickersoft @kevbuchanan Thanks to you both for getting this done!

[Nickersoft]

Thanks so much @martijnwalraven :) Happy to see this feature finally become part of the library! I wrote a sister PR containing some brief docs about these functions just so people know they exist: #452.