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

Send PRAGMA queries to SELECT handler (Android) #91

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

beeks
Copy link
Contributor

@beeks beeks commented Mar 8, 2021

This patch addresses Issue #65. Basically we just want to treat a PRAGMA query (but not an assignment) with the same handler as the one which does SELECT, so that it can return the results of the query (and not throw the exception named in the issue).

Semi-related, and I didn't include this change in this pull request, but in order to enable foreign key support I had to add a call to database.setForeignKeyConstraintsEnabled(true); in the Module just after database = SQLiteDatabase.openOrCreateDatabase(file, null);. Apparently Android won't respect PRAGMA foreign_keys = ON if you do it at a later time. I wasn't sure how best to address this, whether to add a parameter to openDatabase in src/index.js (which would defy the websql API it's going for) or to add a parameter to both the RNSqlite2Package and RNSqlite2Module constructors (which looks ugly but at least doesn't mess with the API). I was wondering if you had any other ideas or if you want me to do it one of these ways I can submit another PR.

Cheers

@craftzdog
Copy link
Owner

Thanks for the PR.
Would you make it also for iOS?

Are there any drawbacks to always call database.setForeignKeyConstraintsEnabled(true); such as performance?

@beeks
Copy link
Contributor Author

beeks commented Mar 9, 2021

Thanks for getting back so quickly :)

So I don't actually have access to a mac/iOS simulator and don't have any experience with swift or objective-c - I can't really even say if this is a problem on that platform. According to the error message, which references the android-specific API methods query and rawQuery, I'd assume that this issue only effects android?

Re: always enabling foreign key constraints, from what I can gather from https://www.sqlite.org/foreignkeys.html#fk_enable, the only reason they haven't done it is for "backwards compatibility", but they also note that

(Note, however, that future releases of SQLite might change so that foreign key constraints enabled by default. Careful developers will not make any assumptions about whether or not foreign keys are enabled by default but will instead enable or disable them as necessary.)

To me, that means it would be fine to always call it, because if someone is writing SQL queries that include foreign keys, that means they would want to be utilizing that feature. But it's obviously your library and your call whether you want it in. If so, I can either add it to this PR or make a new one.

As far as iOS is concerned, again I just don't know if they have the same issue. The android API for that method explicitly forbids calling it after the database has been set up, and I don't know if the iOS code has the same limitation/requirement.

For someone who does have access to iOS, they could run something like
new Promise((resolve,reject) => db.transaction(txn => { txn.executeSql("PRAGMA foreign_keys = on", [], (txn, res) => { console.log(txn, res); resolve(res.rows); }, (txn, err) => {reject(err)}); })).then(() => { new Promise((resolve,reject) => db.transaction(txn => { txn.executeSql("PRAGMA foreign_keys", [], (txn, res) => { console.log(txn, res); resolve(res.rows.item(0)); }, (txn, err) => {reject(err)}); }))}).then(res => console.log("foreign_keys:", res)).catch(e => console.log(e));
and check that foreign keys: 1 was logged. And if part 1 of this issue affected iOS, we'd know that for sure with this code as well because the second query would fail like it does for android

@craftzdog craftzdog merged commit e768ea8 into craftzdog:master Mar 17, 2021
@craftzdog
Copy link
Owner

Thanks for the explanation.
Okay, that makes sense. I think it's safe to always enable foreign key constraints.
Can you please make another PR for that?

@beeks beeks mentioned this pull request Mar 21, 2021
@beeks
Copy link
Contributor Author

beeks commented Mar 21, 2021

Yes sir, here's the PR for that:
#92

@bbalan
Copy link

bbalan commented Mar 24, 2021

I can confirm PRAGMA foreign_keys = ON does not work on iOS 😢

  await db.transaction((txn) => {
    txn.executeSql('PRAGMA foreign_keys = ON', [])
  })
  await db.transaction((txn) => {
    txn.executeSql('PRAGMA foreign_keys', [], (tx, res) => {
      console.log(res)
    })
  })

Result: {foreign_keys: 0}. SQLite 3.32.3

@craftzdog
Copy link
Owner

@bbalan PR would be appreciated as I don't use it personally.

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

Successfully merging this pull request may close these issues.

None yet

4 participants