Implementation of getGeneratedKeys() by prrvchr · Pull Request #1067 · xerial/sqlite-jdbc

prrvchr · 2024-02-06T02:37:32Z

No description provided.

gotson · 2024-02-06T03:04:39Z

we would need some unit test coverage

gotson · 2024-02-06T03:08:16Z

+        int pos = sql.lastIndexOf(';');
+        int index = pos != -1 ? pos : sql.length();
+        String prefix = sql.substring(0, index);
+        String suffix = sql.substring(index);
+        sql = prefix + " RETURNING " + keys + suffix;


I believe this is quite dangerous to perform such changes on the fly on the query. There is no query manipulation in this driver, and if we were to add it, it would need a proper sql parser.

My understanding from your issue was that if the user submits a query with RETURNING, the keys would be made available, else they would be ignored.

I believe this is quite dangerous to perform such changes on the fly on the query. There is no query manipulation in this driver, and if we were to add it, it would need a proper sql parser.

Simply add (before a possible ;) the word RETURNING and a * for all the columns or columns chosen by their name. I don't think a parser is needed to make such simple changes.
This may seem a bit ugly but I believe it is the right way to get as close as possible to the JDBC API.

My understanding from your issue was that if the user submits a query with RETURNING, the keys would be made available, else they would be ignored.

No more precisely if the user submits a query with:

execute(String sql, int autoGeneratedKeys) or executeUpdate(String sql, int autoGeneratedKeys) with autoGeneratedKeys == RETURN_GENERATED_KEYS

execute(String sql, String[] colnames)

executeUpdate(String sql, String[] colnames)

executeLargeUpdate(String sql, String[] colnames)
Then all the keys or the one requested will be available in the getGeneratedKeys() method.

This may seem a bit ugly

you got the gist of it. I am not keen on proceeding on merging this PR in that state.

OK what do you suggest?

as i said, if a statement contains RETURNING, the driver could return the generated keys.

Maybe we can parse it like this:

int pos = sql.indexOf(';'); int index = pos != -1 ? pos : sql.length(); StringBuilder buffer = new StringBuilder(sql.substring(0, index)); buffer.append(" RETURNING "); buffer.append(keys); buffer.append(sql.substring(index)); sql = buffer.toString();

In any case, any parser won't do much different than that?

as i said, if a statement contains RETURNING, the driver could return the generated keys.

I understood that well, but why ask me to do PR if it's to answer me that?

Your solution does not follow the JDBC API for two reasons:

an executeUpdate cannot return a resultset.

SQL INSERT commands must be carried out using the executeUpdate() method.

As a result, software following JDBC specifications will not work with sqlite-jdbc. It's really a shame...

As a result, software following JDBC specifications will not work with sqlite-jdbc. It's really a shame...

compromises have to be made, SQLite is very different from other DB engines.

gotson · 2024-02-06T09:39:49Z

I think there was some misunderstanding about the issue you raised, and the PR you created.

Getting multiple generated keys for a multi-insert statement is not possible in SQLite, unless you use a RETURNING clause
this project will not add query modification on the fly. Doing so in a safe manner would require a parser, and at the moment this project does not include one.
checking whether a provided query contains some keywords is fine, even without a parser. It could be wrong, but the risk is small.

A possible solution would be to detect whether the query contains a RETURNING keyword, and if that is the case, provide the returned keys from the statement inside getGeneratedKeys. I believe that is what sqlite-jna does.

However, i believe that in your particular use case, you do not control the queries, and as such you would not be able to use a RETURNING clause.

prrvchr · 2024-02-06T11:58:11Z

However, i believe that in your particular use case, you do not control the queries, and as such you would not be able to use a RETURNING clause.

This is exactly my problem, it's LibreOffice Base which controls my driver.
On the other hand, I have just implemented this operating mode in my driver and it works quite well.
Is it complicated to compile sqlite-jdbc?

gotson · 2024-02-07T02:07:01Z

Feel free to use your own fork if you need. I will not merge this PR.

Is it complicated to compile sqlite-jdbc?

It is not, if you read the documentation. What is it you are struggling with ?

prrvchr · 2024-02-07T12:32:12Z

Feel free to use your own fork if you need. I will not merge this PR.

The job of a driver is to follow a standard here JDBC.
Its developer must do everything possible to follow this standard (cache of results, deferred loading, rewriting of queries).
But you seem determined not to modify the queries and therefore not to be able to follow the JDBC specifications .
I repeat, but under JDBC the INSERT, UPDATE and DELETE SQL command are done by the java.sql.Statement.executeUpdate() methode and not java.sql.Statement.execute() which is reserved for obtaining java.sql.ResultSet and obtaining dynamic keys following an INSERT or an UPDATE is done by the java.sql.getGeneratedKeys() method and for JDBC if the getGenratedKeys() method is not present then the management of records in writing (INSERT) can only be done record by record (goodbye performance).

You are free to do what you want, but do not ask the other to produce PRs if in the end they are refused without even being tested.

And I think it would be good if you put a link to my archive with an explanation to make it available to those who need to follow the JDBC specifications.

gotson · 2024-02-08T03:12:25Z

honestly you start to be annoying. I already stated the position of this project, if you are not happy, move along, or fork the project. It's open source after all.

prrvchr added 2 commits February 6, 2024 03:09

getGeneratedKeys implementation

0ad849b

getGeneratedKeys implementation

4efec79

prrvchr mentioned this pull request Feb 6, 2024

getGeneratedKeys() come back #1061

Closed

gotson reviewed Feb 6, 2024

View reviewed changes

prrvchr added 3 commits February 6, 2024 21:25

getGeneratedKeys implementation 2

4824082

getGeneratedKeys implementation 3

3a5dbe8

getGeneratedKeys implementation 4

841aa38

gotson added the wontfix label Feb 7, 2024

gotson closed this Feb 7, 2024

Repository owner locked as too heated and limited conversation to collaborators Feb 8, 2024

Conversation

prrvchr commented Feb 6, 2024

Uh oh!

gotson commented Feb 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gotson commented Feb 6, 2024

Uh oh!

prrvchr commented Feb 6, 2024

Uh oh!

gotson commented Feb 7, 2024

Uh oh!

prrvchr commented Feb 7, 2024

Uh oh!

gotson commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants