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

SQLite cache doesn't work #211

Closed
airstance opened this issue Mar 31, 2019 · 18 comments
Closed

SQLite cache doesn't work #211

airstance opened this issue Mar 31, 2019 · 18 comments
Assignees
Labels
closing-soon-if-no-response This issue will be closed in 7 days unless further comments are made. requesting info Further information is needed before this is actionable

Comments

@airstance
Copy link

Describe the bug
When I initialize an AWSAppSyncClient object, I'm getting SQLite errors after it creates the initial table.

To Reproduce
Steps to reproduce the behavior:

  1. Start with the simple sample ios application
  2. Run it
  3. Notice the '[logging] unrecognized token: ":"' in the output - this indicates the error
  4. It falls back to the in-memory cache

Environment(please complete the following information):

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

Device Information (please complete the following information):

  • Device: [e.g. iPhone6, Simulator]
  • iOS Version: [e.g. iOS 11.4]
  • Specific to simulators:

Additional context
this line in AWSSQLLiteNormalizedCache.swift is throwing an error:
71 let recordCount = try db.scalar(queryRootRecords.count)

2019-03-31 07:46:10.896731-0700 Trips[3429:79351] [logging] unrecognized token: ":"
(SQLite.Result) $E7 = error {
error = {
message = "unrecognized token: ":""
code = 1
statement = nil
}
}

It seems to be building an odd request for SQLite:

(SQLite.ScalarQuery) $R4 = {
clauses = {
select = {
distinct = false
columns = 1 value {
[0] = {
template = "count(_:)(*)"
bindings = 0 values {}
}
}
}
from = {
name = "records"
alias = nil
database = nil
}
join = 0 values {}
filters = some {
template = "("key" = ?)"
bindings = 1 value {
[0] = "QUERY_ROOT"
}
}
group = nil
order = 0 values {}
limit = nil
union = 0 values {}
}
}

@airstance
Copy link
Author

Apparently, it's a problem with SQLite.swift:

stephencelis/SQLite.swift#888

I suppose there are two issues now:

  1. Integration of the fix from SQLite.swift
  2. The AppSync ios cache test case doesn't verify that a db cache was created

@airstance
Copy link
Author

Oh joy, the SQLite.swift cocoapod hasn't been updated in nearly a year. I added this to my podfile to force download the latest:

pod 'SQLite.swift', :git => 'https://github.com/stephencelis/SQLite.swift', :branch => 'master'

I'm still trying to verify that the latest version fixes the bug and doesn't cause any new ones.

@airstance
Copy link
Author

Just to follow up... the above workaround fixed local caching for me. I'm still somewhat amazed that this wasn't brought up earlier. I'm guessing this is somehow unique to my setup.

@minbi minbi added the AppSync label Apr 1, 2019
@minbi minbi added the pending investigation It has been triaged and discussed but the actual work is still pending label Apr 1, 2019
@zigfreid5
Copy link

@airstance Haha, honestly I haven't brought it up because no one else brought it up and it seems like it would be a widespread issue, so I thought I just did something wrong.

@zigfreid5
Copy link

zigfreid5 commented Apr 1, 2019

For what it's worth, the workaround posted above didn't fix the issue for me. I still get the unrecognized token: ":" output and the persisted cache still doesn't work.

Edit
I take that back. I moved the reference to the SQLite branch below the reference to AppSync and I no longer have the caching issues. +1 for the workaround, @airstance

@mrbarnaby
Copy link

This issue has impacted our app. Cache stopped working. We didn't notice initially because we didn't test the offline usecase before last release. The workaround from @airstance above seems to work.

@freshtechs
Copy link

Can someone Deny or confirm?

@palpatim
Copy link
Contributor

Hi all, I'm investigating this issue today and will update when I have more info. If the update from @airstance is accurate, the fix might be as easy as just updating our SQLite dependency to point to the master branch instead of an officially tagged release. However, we'll need to review the changes to make sure there aren't any surprises laying in wait for us taking the latest release.

@adolffre
Copy link

I'm also having this problem!
And maybe related to this,
If I fetch first having internet. Turn off the internet and try to fetch from cache: works ... But if I close the app and try again ( still without internet connection ) doens't work!

palpatim added a commit that referenced this issue Apr 17, 2019
- Updated SQLite.swift to master branch instead of official release
- Added unit tests for #211
- Fixed whitespace linting violations
- Import AppSyncLogHelper instead of CocoaLumberjack in AppSync Objective-C
  classes
- Added SQLite to TestCommon dependencies to allow for direct DB access for
  some setup unit tests
- Updated AWS dependencies to 2.9.5
- Moved long-running performance tests to integration test suite to allow for
  rapid execution of unit tests during development
palpatim added a commit that referenced this issue Apr 17, 2019
- Updated SQLite.swift to master branch instead of official release
- Added unit tests for #211
- Fixed whitespace linting violations
- Import AppSyncLogHelper instead of CocoaLumberjack in AppSync Objective-C
  classes
- Added SQLite to TestCommon dependencies to allow for direct DB access for
  some setup unit tests
- Updated AWS dependencies to 2.9.5
- Moved long-running performance tests to integration test suite to allow for
  rapid execution of unit tests during development
palpatim added a commit that referenced this issue Apr 17, 2019
- Updated SQLite.swift to specific master branch commit instead of official
  tagged release
- Added unit tests for #211
- Fixed whitespace linting violations
- Import AppSyncLogHelper instead of CocoaLumberjack in AppSync Objective-C
  classes
- Added SQLite to TestCommon dependencies to allow for direct DB access for
  some setup unit tests
- Updated AWS dependencies to 2.9.5
- Moved long-running performance tests to integration test suite to allow for
  rapid execution of unit tests during development
palpatim added a commit that referenced this issue Apr 17, 2019
- Updated SQLite.swift to specific master branch commit instead of official
  tagged release
- Added unit tests for #211
- Fixed whitespace linting violations
- Import AppSyncLogHelper instead of CocoaLumberjack in AppSync Objective-C
  classes
- Added SQLite to TestCommon dependencies to allow for direct DB access for
  some setup unit tests
- Updated AWS dependencies to 2.9.5
- Moved long-running performance tests to integration test suite to allow for
  rapid execution of unit tests during development
@palpatim
Copy link
Contributor

palpatim commented Apr 18, 2019

What this boils down to is, as @airstance pointed out, a bug in SQLite.swift. Xcode 10.2 changed the behavior of the #function expression, which the SQLite.swift framework used to construct its expressions.

The bug is fixed in the master branch of the SQLite.swift package, but they haven't done a release with the fix yet. We attempted to work around this by pinning our dependency to the appropriate commit in the master branch, which fixes the issue locally, but doesn't propagate to client apps that consume AppSync, because CocoaPods only allows us to specify versions, not commits, inside a Podspec.

Our options are all sort of unpalatable at this point:

  1. Fork SQLite.swift into our code, removing it as a dependency in the Podfile and Podspec.

    • Pros: Fixes the bug immediately

    • Cons: Breaking change. Customers who rely on SQLite in their own code would have to remove that dependency from their own Podfile and use the code vended by the forked code within AppSync.

      In addition, we would either have to maintain the fork going forward (which might actually be a net positive, despite the extra maintenance cost), or submit another breaking change to revert back to using the GitHub source.

  2. Require AppSync customers to specify the correct commit of SQLite.swift in their own Podfile alongside the AppSync dependency

    • Pros: Fixes the bug immediately

    • Cons: Breaking change, and extra work for developers even if they don't depend directly on SQLite.swift in their own app, and even if they don't encounter the bug because they don't build on Xcode 10.2.

      Further, once SQLite.swift releases 0.11.6, we would need to do another breaking change to remove the pinned version requirement.

  3. Wait for SQLite.swift to release 0.11.6. There is an in-process PR to get an 0.11.6 release out that supports Xcode 10.2.

    • Pros: Most compatible for customers
    • Cons: The PR is a couple of weeks old at this point, and is currently stalled pending fix of failing tests. There is a recent update on that PR with Apple's workaround for the problem, but no ETA from the SQLite.swift team for incorporating that workaround or the actual release afterwards.

We're conferring to see which way we want to go. At this point I'm leaning toward Option 1, since it's the one that fixes the bug fastest with the least likelihood of impact. My guess is that the number of customers using AppSync and also relying on SQLite is probably relatively small compared to the number who would be impacted by Option 2, and the increasing number of people who would be impacted by the bug and therefore having to wait if we go with Option 3.

@palpatim
Copy link
Contributor

It looks like SQLite.swift has released an 0.11.6 tag to their repo, but not yet published it to the CocoaPods master spec repository. We're investigating setting up a dedicated spec repo as part of our source tree to get this fixed ASAP for AppSync.

@adolffre
Copy link

well. For me is still not working =T
And still having the '[logging] unrecognized token: ":"' in the output

@albertchubakov
Copy link

well. For me is still not working =T
And still having the '[logging] unrecognized token: ":"' in the output

If you use cocoa pods - you can try add to pod file line : "pod 'SQLite.swift', :git => 'https://github.com/stephencelis/SQLite.swift', :branch => 'master'
" and then clean pod cache and reinstall all pods

@adolffre
Copy link

adolffre commented Apr 23, 2019

well. For me is still not working =T
And still having the '[logging] unrecognized token: ":"' in the output

If you use cocoa pods - you can try add to pod file line : "pod 'SQLite.swift', :git => 'https://github.com/stephencelis/SQLite.swift', :branch => 'master'
" and then clean pod cache and reinstall all pods

Actually this works for me =)...
Just giving feedback on this new version! But thank you!

@palpatim
Copy link
Contributor

@adolffre There isn't a new version of AppSync that consumes the SQLite.swift 0.11.6 version yet. However, I do see that the CocoaPods trunk is updated with 0.11.6. I'm testing those changes locally and hope to have the new version pushed later today.

@albertchubakov Rather than :branch => 'master', I recommend using :tag => '0.11.6', since the master branch may be updated without warning. Notably, there is an in-process PR to upgrade SQLite.swift to Swift 5 syntax as version 0.12, which one would assume will be posted to master relatively soon.

@palpatim
Copy link
Contributor

I've confirmed that we can pull in SQLite.swift 0.11.6. Technically, this will be a breaking change, as AppSync must now be built using Xcode 10.2 or later even though we are maintaining our Swift version as 4.2.1. However, the fix is only necessary for folks building on Xcode 10.2 anyway, so it seems like a good tradeoff, letting customers who don't currently build on Xcode 10.2 migrate.

@palpatim
Copy link
Contributor

This is fixed in 2.12.0. Please let us know if you continue to see problems.

@palpatim palpatim added requesting info Further information is needed before this is actionable closing-soon-if-no-response This issue will be closed in 7 days unless further comments are made. and removed pending investigation It has been triaged and discussed but the actual work is still pending labels Apr 24, 2019
@stale
Copy link

stale bot commented May 1, 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 May 1, 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. requesting info Further information is needed before this is actionable
Projects
None yet
Development

No branches or pull requests

9 participants