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

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

Closed
ygarg465 opened this issue Mar 13, 2024 · 16 comments

Comments

@ygarg465
Copy link

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):

  • Device: Pixel 7 Pro
  • OS: API 34
  • Version 5.6.3
@msfstef
Copy link

msfstef commented Mar 13, 2024

@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
Copy link
Collaborator

@msfstef use the transitionBegin, transitionCommit see transition doc

@jepiqueau
Copy link
Collaborator

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

@msfstef
Copy link

msfstef commented Mar 13, 2024

Opened PR for this: #527

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

@msfstef
Copy link

msfstef commented Mar 13, 2024

@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
Copy link
Author

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

This also occurs with API level 33

@jepiqueau
Copy link
Collaborator

@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
Copy link

msfstef commented Mar 13, 2024

@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
Copy link
Collaborator

@msfstef OK i understand i will have a look

@jepiqueau
Copy link
Collaborator

@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
Copy link

msfstef commented Mar 13, 2024

@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
Copy link
Author

@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
Copy link
Collaborator

@msfstef i will add to the documentation

  • SQLite Data Definition Language commands (CREATE, ALTER, DROP) and the SQLite Transaction Control commands (BEGIN TRANSACTION, COMMIT, ROLLBACK) must used execute method.
  • SQLite Data Manipulation Language commands (INSERT, UPDATE, DELETE, REPLACE) must used run method if they have bind values and can use execute or run methods if they do not have bind values.
  • SQLite Data Query Language commands (SELECT) must used query method.

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

msfstef added a commit to electric-sql/electric that referenced this issue Mar 13, 2024
…1052)

Using the `execute` API rather than the `run` one for when no bind
values are provided as it has less pre-processing and solves a parsing
issue that `run` has for statements like `BEGIN` and `ROLLBACK`.

Not strictly necessary but provides an immediate solution to a bug that
is blocking the use of this driver on Android without requiring changes
from the driver side.

See this issue: capacitor-community/sqlite#526
@jepiqueau
Copy link
Collaborator

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

@msfstef
Copy link

msfstef commented Mar 13, 2024

@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
Copy link
Collaborator

@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

msfstef added a commit to electric-sql/electric that referenced this issue Mar 14, 2024
…1052)

Using the `execute` API rather than the `run` one for when no bind
values are provided as it has less pre-processing and solves a parsing
issue that `run` has for statements like `BEGIN` and `ROLLBACK`.

Not strictly necessary but provides an immediate solution to a bug that
is blocking the use of this driver on Android without requiring changes
from the driver side.

See this issue: capacitor-community/sqlite#526
jepiqueau added a commit that referenced this issue Mar 27, 2024
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 a pull request may close this issue.

3 participants