Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ApolloStore: remove(cascade = true) doesn't remove ApolloCacheReference from the corresponding record #4996

Closed
ihermandev opened this issue May 29, 2023 · 8 comments

Comments

@ihermandev
Copy link

ihermandev commented May 29, 2023

Version

3.8.2

Summary

The current issue is an extension of #4932
The fixed version of remove(cascade = true) works better in fact but what is the purpose of keeping ApolloCacheReference record of the removed key? It doesn't allow the cache to be used properly.

Context: I'm using SqlNormalizedCacheFactory with 7 cached rows in the database. I'm trying to perform delete actions using apolloClient.apolloStore.accessCache { cache -> // perform delete actions} or graphQlClient.apolloClient.apolloStore.remove(cacheKey = key, cascade = true).

Table: I have the following cached rows in the database:

_id key record
1 user.workPermit {“__typename":"WorkPermit","documents":["ApolloCacheReference{Document:191e771f-4c2e-4a2d-8732}","ApolloCacheReference{Document:6050a8eb-dbe0-4e9e-9afc}"]}
2 Document:191e771f-4c2e-4a2d-8732.group {}
3 Document:191e771f-4c2e-4a2d-8732.type {}
4 Document:191e771f-4c2e-4a2d-8732.validity {}
5 Document:191e771f-4c2e-4a2d-8732 {"id":"191e771f-4c2e-4a2d-8732”,”group":"ApolloCacheReference{Document:191e771f-4c2e-4a2d-8732.group}","type":"ApolloCacheReference{Document:191e771f-4c2e-4a2d-8732.type}","validityRange":"ApolloCacheReference{Document:191e771f-4c2e-4a2d-8732.validityRange}","__typename":"Document","pages":["ApolloCacheReference{Document:191e771f-4c2e-4a2d-8732.pages.0}"]}
6 Document:191e771f-4c2e-4a2d-8732.pages.0.type {}
7 Document:191e771f-4c2e-4a2d-8732.pages.0 {}

Here's what I observed:

cache.remove(CacheKey(key = "Document:191e771f-4c2e-4a2d-8732"),cascade = true ) or apolloStore.remove(CacheKey(key = "Document:191e771f-4c2e-4a2d-8732"), cascade = true)—> returns true and removes 2-7 IDs rows, but the reference ApolloCacheReference{Document:191e771f-4c2e-4a2d-8732} from the record of ID 1 row (user.workPermit) still exists.

The expected result for ID 1 row is

_id key record
1 user.workPermit {“__typename":"WorkPermit","documents":["ApolloCacheReference{Document:6050a8eb-dbe0-4e9e-9afc}"]}

Steps to reproduce the behavior

  • configureSqlNormalizedCacheFactory
  • populate the apollo cache with similar or identical data from the Summary section
  • try to use apolloClient.apolloStore.accessCache { cache -> cache.remove(key, cascade = true)}

@BoD
Copy link
Contributor

BoD commented May 30, 2023

Hi!

As of now, this is expected: the references still exist when a record is removed (FWIW it is also the case independently of cascade=true). This means that trying to access user.workPermit will result in a cache miss.

This issue apollographql/apollo-kotlin-normalized-cache-incubating#85 is about removing dangling references, which will work as you expect, but the exact ergonomics are still to be determined.

In the meantime, what you could do is use the ApolloStore APIs to manually alter the cache. Something like:

      // Save current cache data
      val currentCachedData = apolloClient.apolloStore.readOperation(WorkPermitQuery(userId))

      // Remove specific record
      apolloClient.apolloStore.accessCache { cache ->
        cache.remove(CacheKey("Document:191e771f-4c2e-4a2d-8732"), true)
      }

      // Update parent record to not reference removed record
      apolloClient.apolloStore.writeOperation(
          WorkPermitQuery(userId),
          currentCachedData.copy(
              documents = currentCachedData.documents.filter { it.id != "191e771f-4c2e-4a2d-8732" }
          )
      )

@ihermandev
Copy link
Author

Hi,

thank you for the explanation and information about the ongoing issue apollographql/apollo-kotlin-normalized-cache-incubating#85, I appreciate your efforts.
I have explored the ApolloStore APIs to manually alter the cache as suggested, and it has indeed helped me achieve the desired behavior.
However, I understand that this approach can be a bit overhead as it requires specific modifications for each query with possible common documents in my case, including knowing the exact query name and other details such as the precise type for the instance of __typename":"WorkPermitA", "WorkPermitB", "WorkPermitC":

  // Update parent record to not reference removed record
  apolloClient.apolloStore.writeOperation(
      WorkPermitQuery(userId),
      currentCachedData.copy(
          documents = currentCachedData.asWorkPermitA.documents.filter { it.id != "191e771f-4c2e-4a2d-8732" }
      )
      
      or
      
          currentCachedData.copy(
          documents = currentCachedData.asWorkPermitB.documents.filter { it.id != "191e771f-4c2e-4a2d-8732" }
      )
      
      or
      
          currentCachedData.copy(
          documents = currentCachedData.asWorkPermitC.documents.filter { it.id != "191e771f-4c2e-4a2d-8732" }
      )
      
      etc.
  )

@BoD
Copy link
Contributor

BoD commented May 30, 2023

I definitely agree that this is not ideal and a bit cumbersome. The real preferred solution is to have a version of apollographql/apollo-kotlin-normalized-cache-incubating#85 working.

If that is not too much to ask, in the meantime could you tell us a bit more about your use-case at a higher functional level. From what I see I understand you are trying to locally remove documents related to a user but if you can provide a bit more context that would be valuable to us (e.g. I would expect this deletion to happen on the server rather than locally, but I don't have the 'big picture').

@ihermandev
Copy link
Author

Sure, the main objective is to enhance the client app user experience by implementing graphql cache.
Imagine an app screen that displays user data, including a list of documents. When a user removes a document, I want to update the model and exclude the removed document without fetching the user data again. By manipulating the existing data and updating the cache, I can avoid unnecessary network requests. Furthermore, I aim to leverage the cached data when the user revisits the screen, without the need for fetching the data again, so I can achieve desired functionality only when the removed document cache reference doesn’t exist.
Additionally, proper cache manipulation helps me to 100% avoid on the client side some potential eventual consistency issues which are not related to the client side.
As I have mentioned above, the existing solution of removing cache references is a bit tricky, especially in the case of complex graphql queries.

query GetWorkPermitQuery {
  user {
    workPermit {
      ... on WorkPermitA {
		…
        documents {}
		…
      }
      ... on WorkPermitB {
		…
        documents {}
		…
      }
      ... on WorkPermitC {
		…
        documents {}
		…
      }
}}}

In such case, I need to know in advance what kind of … on WorkPermit my document belongs to in order to remove proper ApolloCacheReference. For some other query GetSomethingQuery with the same documents I need to handle a completely different query in order to remove ApolloCacheReference and so on.

If you have more questions I will be glad to help :)

@BoD
Copy link
Contributor

BoD commented May 31, 2023

Thanks a lot, that is useful context.

When a user removes a document, I want to update the model and exclude the removed document without fetching the user data again.

In most cases we've seen, the way this usually works is that a mutation API returns the new data, in order for the client to be up-to-date with the backend state. The idea being, if you are making a call to delete the document, you might as well use the returned data at this point. In that case it would be unneeded to manually manipulate the cache on the client. But maybe that is not how your API works currently? Or is there something else I'm not seeing.

@ihermandev
Copy link
Author

ihermandev commented May 31, 2023

Well, currently my API doesn't work that way indeed. However, could you please clarify one thing? If I remove documents and expect the WorkPermit cache to be updated, should the entire WorkPermit request be returned, including the documents? Something like the following mutation:

mutation RemoveDocuments($documentIds: [ID!]!) {
  removeDocuments(documentIds: $documentIds) {
    user {
      workPermit {
        ... on WorkPermitA {
          …
          documents {
         
          }
          …
        }
        ... on WorkPermitB {
          …
          documents {
          
          }
          …
        }
        ... on WorkPermitC {
          …
          documents {
            
          }
          …
        }
      }
    }
  }
}

In the case below, if I'm deleting the same documents that are now part of another layer, I would need to create a separate mutation. Here's an example of such a mutation:

mutation RemoveDocuments($documentIds: [ID!]!) {
  removeDocuments(documentIds: $documentIds) {
    profile {
      userData {
        documents {
          
        }
      }
    }
  }
}

Am I correct in assuming that if I expect a specific cache update, I need to specify exactly what I expect to be updated automatically? Hence, I need to create several mutations that do essentially the same thing but return different data.

@BoD
Copy link
Contributor

BoD commented May 31, 2023

Generally speaking, yes, to ensure your cache is up-to-date with the backend after a mutation, you will need to select all the fields that were impacted by the mutation.

If that's not possible (e.g. the schema doesn't return these fields), chain your mutation with a query - this is less efficient because you will be executing 2 calls instead of 1, but this ensures your cache is fresh without updating it manually.

This section of the doc explains this a bit.

If you have configured cache ids for your types (see this section), then no matter which Query or Mutation you are executing, the results will be stored in the same records in the cache - I think that replies your last question but let me know if I missed something.

Just a remark about your 1st query above: if WorkPermitA, WorkPermitB, etc. implement a common interface, you could certainly simplify your query with something like ... on IWorkPermit { documents { documentField } } without needing to have all possible cases in your query.

@ihermandev
Copy link
Author

ihermandev commented Jun 1, 2023

Thanks for your help and suggestions! Everything works as was discussed in the comments, but still, the only thing that is really missing, from my point of view, is the ability to handle dangling references of the parent cache object. This feature will simplify cache management a lot on the client side. In some cases, it just doesn't make sense to delete the cache key when the cache reference still exists and breaks the logical structure of the cache.
Anyway, it probably makes sense to close this issue and continue the discussion there apollographql/apollo-kotlin-normalized-cache-incubating#85.
Thanks again for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants