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

Cache doesn't seem to work when performing a query which maps to dynamoDB secondary index (i.e. not doing a query based on "id") #165

Closed
mrbarnaby opened this issue Jan 28, 2019 · 10 comments
Assignees
Labels
closing-soon-if-no-response This issue will be closed in 7 days unless further comments are made. question A question about how to use an existing feature

Comments

@mrbarnaby
Copy link

In my iOS project I'm using AppSync and DynamoDB.

The AppSync cache is working fine for me on 4 out of the 5 queries I'm using. However it isn't working on the 5th query and I'm not sure why. I can't figure it out.

The main difference with the 5th query is that the first 4 queries are standard "List with Filter (aka DynamoDB scan)" or "Get with Id (aka Dynamo query by id)", however with the 5th query I'm doing a query based on an attribute in a secondary index in DynamoDB. I've written a custom resolver in AppSync which maps to this attribute (it uses a DynamoDB 'query' not a 'scan'). The query works fine... the custom "User" objects which I'm requesting are returned as expected. However they never end up in the Apollo cache.

Any help would be greatly appreciated as not being able to retrieve the User object from the cache is breaking the offline behaviour of my app completely!

Debug I've tried so far

  • I have queried for my custom User objects using a standard "get by ID" query and they are cached by Apollo fine.
  • I have also added a print statement to the block passed to the cacheKeyForObject method and I can see that the object ID is returned as expected for these User Objects. I tried the
  • I have experimented to check it is not an issue with the cachePolicy setting, but that is working as expected.

Code snippets

I believe I'm calling the problematic query in a standard way.

This one does cache successfully:
appSyncClient?.fetch(query: GetUserQuery(id: userId), cachePolicy: cachePolicy) { (result, error) in

This one doesn't cache:
appSyncClient?.fetch(query: GetUsersByCanonicalEmailQuery(canonicalEmail: canonicalEmail, limit: 10), cachePolicy: cachePolicy) { (result, error) in

Environment(please complete the following information):

  • AppSync SDK Version: 2.9.0
  • Dependency Manager: Cocoapods
  • Swift Version : 4.2

Device Information (please complete the following information):

  • Device: iPhone 7 Plus, iPhone 6s
  • iOS Version: 12.1.2
  • Specific to simulators:
@mrbarnaby mrbarnaby changed the title Cache doesn't see to work when performing dynamoDB query on secondary index Cache doesn't see to work when performing query which maps to dynamoDB secondary index (i.e. not doing a query based on "id") Jan 28, 2019
@sunchunqiang sunchunqiang self-assigned this Jan 29, 2019
@mrbarnaby mrbarnaby changed the title Cache doesn't see to work when performing query which maps to dynamoDB secondary index (i.e. not doing a query based on "id") Cache doesn't seem to work when performing a query which maps to dynamoDB secondary index (i.e. not doing a query based on "id") Jan 29, 2019
@mrbarnaby
Copy link
Author

I think I've discovered the issue. For this particular object type I'm using an email address as the AppSync ID, and so this is then used as the cache key by apollo. However it looks like the apollo cache doesn't like the cache key to contain to contain "@" or ".". I'll dig into this more tomorrow.

@palpatim
Copy link
Contributor

Thanks for the update @mrbarnaby.

However it looks like the apollo cache doesn't like the cache key to contain to contain "@" or ".".

Are you basing this on an inspection of the Apollo code, or on your observations of behavior?

@sunchunqiang sunchunqiang added the question A question about how to use an existing feature label Jan 29, 2019
@palpatim palpatim added requesting info Further information is needed before this is actionable and removed help wanted labels Jan 29, 2019
@artekr
Copy link

artekr commented Jan 30, 2019

@mrbarnaby @palpatim I have encountered the same issue where the cache key contains a ".".

For example
appSyncClient?.fetch(query: GetOrderQuery(orderId: 123, shippingTotal: 10.5), cachePolicy: cachePolicy)
This query will never be stored in 'QUERY_ROOT' as the reference, since shippingTotal a float type and has a ".", there was a git issue opened before about this: #57

I tested another scenario with String.
appSyncClient?.fetch(query: GetOrderQuery(orderId: 123, shippingTotal: "10.5"), cachePolicy: cachePolicy)
In this case, shippingTotal take a string, but there is a dot in it. It behaves the same as the previous one, where the query getOrderQuery(orderId: 123, shippingTotal: "10.5") did not get save in "QUERY_ROOT".

Therefore, in both cases, it will return nil when fetching queries from cache in offline mode.

Can you help to take a look?
Thanks

@palpatim
Copy link
Contributor

@mrbarnaby

Thanks for the comments and explanation of your use case. I'll take a look.

@palpatim palpatim removed the requesting info Further information is needed before this is actionable label Feb 4, 2019
@palpatim
Copy link
Contributor

palpatim commented Feb 4, 2019

Based on my read of the code, the . character is used as a cache key path separator in both Apollo and AppSync code. I haven't traced your use cases specifically, but it seems reasonable that the dot character could be causing the behavior you describe.

Unfortunately, I'm not sure what to do for a workaround at this point, and will have to consider this. We may need to allow an option for replacing the path separator for nested objects. Once we fix that, we'll need to update the docs to call this out.

@amarincas
Copy link

amarincas commented Feb 16, 2019

@mrbarnaby @palpatim @artekr

Hi, I am experiencing the same issue with the "." separator. In the following example it will never fetch from cache, only from server.

let query = GetProfileQuery(email: "[email protected]")
appSyncClient?.fetch(query: query, cachePolicy: .returnCacheDataAndFetch) { result, error in 
}

The problem appears when AWSSQLiteNormalizedCache tries to merge records and fails to obtain the recordCacheKey from fieldCacheKey.

fieldCacheKey: QUERY_ROOT.getProfile(email:[email protected])
recordCacheKey: QUERY_ROOT.getProfile(email:johndoe@gmail

Expected recordCacheKey above is QUERY_ROOT.

After some digging I found out the recordCacheKey is extracted by splitting the fieldCacheKey by "." into components and removing the last component.

To work around this I implemented a different way for extracting the recordCacheKey. I'm trying to figure out if a "." is either a separator or belongs to a parameter value, by exploiting the fact that params are enclosed in parentheses ( and ):

private func recordCacheKey(forFieldCacheKey fieldCacheKey: CacheKey) -> CacheKey {
    var index = fieldCacheKey.endIndex
    var open = 0
    var closed = 0
    while index > fieldCacheKey.startIndex {
        index = fieldCacheKey.index(before: index)
        switch fieldCacheKey[index] {
        case "(": open += 1
        case ")": closed += 1
        case "." where open == closed: return CacheKey(fieldCacheKey[..<index])
        default: continue
        }
    }
    return fieldCacheKey
}

Now the recordCacheKey QUERY_ROOT is extracted from the fieldCacheKey QUERY_ROOT.getProfile(email:[email protected])

Counting parentheses should also work if cache key is composed recursively, but I haven't tested.

This works for emails or float values because they don't contain parentheses. It doesn't always work when String params are used which can contain ( or ). For example it fails for QUERY_ROOT.getProfile(email:[email protected]() because there's an extra (. For this a more generic solution is needed.

@palpatim
Copy link
Contributor

@amarincas

Thanks for diving into this. I'm looking at this and will update when I have more info.

@palpatim palpatim added the pending investigation It has been triaged and discussed but the actual work is still pending label Feb 18, 2019
palpatim added a commit that referenced this issue Feb 20, 2019
- Fix optimistic update tests to wait for optimistic update to complete before
  fetching
- Added unit test helper for some utility functions
- Ported Apollo's CachePersistenceTests from their HEAD
- Added new field, 'friendsFilteredById', to allow easier testing of arguments
  with arbitrary string constructions
- Added persistence tests for arguments with dots
palpatim added a commit that referenced this issue Feb 20, 2019
- Fix optimistic update tests to wait for optimistic update to complete before
  fetching
- Added unit test helper for some utility functions
- Ported Apollo's CachePersistenceTests from their HEAD
- Added new field, 'friendsFilteredById', to allow easier testing of arguments
  with arbitrary string constructions
- Added persistence tests for arguments with dots
@palpatim
Copy link
Contributor

#186 should resolve the issue with using dots in cache keys

Discussion

This problem actually exists in the SQLite normalized cache implementation we forked from Apollo. The solution I've put in place caches incoming records by composing a list of "candidate record keys" based on the changed keys. If the candidate keys exist in the incoming changed records, the cache will be updated with the new value. This has the downside of a larger number of comparisons, but should catch cases where the cache key includes dots as part of the argument chain, including cases like QUERY_ROOT.foo(id:[email protected]).friendsOlderThan(id:34.56)

I'm not thrilled with this implementation. I also explored using a different cache path delimiter, but any of those solutions are susceptible to collisions. Even using non-printing characters has problems with debugging usability/logging.

The right thing to do here is probably to change the results of the "changed cache key" to be something like a "changed cache paths", so that path elements are clearly delimited. However, that was going to be a pretty far-reaching change, and more than I was comfortable taking on for this fix.

@palpatim palpatim added Reviewing and removed pending investigation It has been triaged and discussed but the actual work is still pending labels Feb 20, 2019
palpatim added a commit that referenced this issue Feb 20, 2019
- Fix optimistic update tests to wait for optimistic update to complete before
  fetching
- Added unit test helper for some utility functions
- Ported Apollo's CachePersistenceTests from their HEAD
- Added new field, 'friendsFilteredById', to allow easier testing of arguments
  with arbitrary string constructions
- Added persistence tests for arguments with dots
scb01 pushed a commit that referenced this issue Feb 25, 2019
- Fix optimistic update tests to wait for optimistic update to complete before
  fetching
- Added unit test helper for some utility functions
- Ported Apollo's CachePersistenceTests from their HEAD
- Added new field, 'friendsFilteredById', to allow easier testing of arguments
  with arbitrary string constructions
- Added persistence tests for arguments with dots
@palpatim palpatim added pending-release Work is done, but issue can't be closed since the changes were not released yet and removed Reviewing labels Feb 25, 2019
@palpatim
Copy link
Contributor

This is released on 2.10.2. Please let us know if you continue to experience problems.

@palpatim palpatim added closing-soon-if-no-response This issue will be closed in 7 days unless further comments are made. and removed pending-release Work is done, but issue can't be closed since the changes were not released yet labels Feb 27, 2019
@stale
Copy link

stale bot commented Mar 6, 2019

This issue has been automatically closed because of inactivity. Please open a new issue if are still encountering problems.

@stale stale bot closed this as completed Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closing-soon-if-no-response This issue will be closed in 7 days unless further comments are made. question A question about how to use an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants