capacitor-community / sqlite

Community plugin for native & electron SQLite databases
MIT License
426 stars 104 forks source link

Bug(Android): Calling `run` method with `BEGIN` statement throws StringOutOfBoundIndexException #526

Closed ygarg465 closed 2 months ago

ygarg465 commented 2 months ago

Describe the bug To start the transaction using the BEGIN statement results in Error: Run: begin 0, end 6, length 5 on Android.

This is because of StringOutOfBoundException thrown on this line.

A possible solution provided by @msfstef

String stmtType = statement.replaceAll("\n", "").trim().toUpperCase();
String firstWord = stmtType.split("\s")[0];
 // Limiting to the first 6 characters if the statement is longer
stmtType = firstWord.length() > 6 ? firstWord.substring(0, 6) : stmtType;

Expected behaviour It should extract the statements of less than 6 characters without any exception being thrown.

Smartphone (please complete the following information):

msfstef commented 2 months ago

@jepiqueau this is a bit of a blocker for running transactions using the Capacitor driver on Android - the above solution has been tested and it indeed resolves the issue.

jepiqueau commented 2 months ago

@msfstef use the transitionBegin, transitionCommit see transition doc

jepiqueau commented 2 months ago

@msfstef i saw that you use API34 which is not compatible with Capacitor5

msfstef commented 2 months ago

Opened PR for this: https://github.com/capacitor-community/sqlite/pull/527

@jepiqueau we need to use the executeSet run and query for everything for flexibility

msfstef commented 2 months ago

@msfstef i saw that you use API34 which is not compatible with Capacitor5

I don't think that's related to this issue - clearly this parsing being done here is failing for a valid statement type?

ygarg465 commented 2 months ago

@msfstef i saw that you use API34 which is not compatible with Capacitor5

This also occurs with API level 33

jepiqueau commented 2 months ago

@msfstef have you look at the doc provide iin v5.6.3 this is full tested. Are you working with @ygarg465 if yes please can only one register the issue it and follow it up it will be less confusion for me. Provide me a full code on github so i can add the case you are using

msfstef commented 2 months ago

@msfstef have you look at the doc provide iin v5.6.3 this is full tested. Are you working with @ygarg465 if yes please can only one register the issue it and follow it up it will be less confusion for me. Provide me a full code on github so i can add the case you are using

Both @ygarg465 and I are trying to unblock the usage of the driver with ElectricSQL - currently running the statement BEGIN fails because of the lines mentioned by @ygarg465 above and it can be fixed with this PR.

I understand that you have a transaction API (I looked at the doc you mentioned) but for the sake of interoperability with multiple SQLite drivers we prefer to just be able to run SQLite statements and manage the rest on our side.

For reproducing the issue you can just run a BEGIN statement and you should get the error.

jepiqueau commented 2 months ago

@msfstef OK i understand i will have a look

jepiqueau commented 2 months ago

@msfstef why do not use execute in tha case it will work. Run is more for insert, replace, update, delete which have all 6 characters for the command and is made when you have a statement with or without values to bind

msfstef commented 2 months ago

@jepiqueau you're right that for those particular statements with no bind values we can bypass the prepared statement route, good idea! Will have an immediate fix using that.

That being said, the parsing of the command with the arbitrary substring(0,6) is still a bit problematic and the same statements should be able to run with the run API as well, so fixing this issue I think is still important

ygarg465 commented 2 months ago

@jepiqueau you're right that for those particular statements with no bind values we can bypass the prepared statement route, good idea! Will have an immediate fix using that.

That being said, the parsing of the command with the arbitrary substring(0,6) is still a bit problematic and the same statements should be able to run with the run API as well, so fixing this issue I think is still important

I agree as substring(0,6) is not the best way to extract the statement type.

jepiqueau commented 2 months ago

@msfstef i will add to the documentation

Will this clarify how to use the capacitor-community/sqlite plugin

jepiqueau commented 2 months ago

@msfstef Can i close the issue, i will reject the PR if you now agree

msfstef commented 2 months ago

@jepiqueau sure - I still think the current parsing method of the command is a bit arbitrary and error-prone, but I understand that you want to support run only for the 4 DML commands.

Either way my issue is unblocked through using execute so feel free to close.

jepiqueau commented 2 months ago

@msfstef @ygarg465 thanks for your understanding this is the way i develop the software i must have put this at the early stage of développement. This will have avoid mis-understanding and waste of time for developers