xerial / sqlite-jdbc

SQLite JDBC Driver
Apache License 2.0
2.83k stars 619 forks source link

Simplify JDBC classes and interfaces #767

Open gotson opened 2 years ago

gotson commented 2 years ago

While looking at #735 i noticed that we have similar classes in org.sqlite.core, org.sqlite.jdbc3 and org.sqlite.jdbc4.

For example for Statement:

And for PreparedStatement:

That seems like a mess to me:

I'm not sure what's the intended use of the core package, as it would always have dependencies on java.sql anyway.

There are also some classes within org.sqlite like SQLiteConnection or SQLiteDataSource.

I suppose there's some history behind that:

While i am no expert in JDBC, it seems JDBC 4.2 shipped with Java 8.

Given we only support Java 8, we could probably remove the JDBC3 package, and have a single JDBC4 package instead.

I'm not sure what to do with the core package or the classes that are in org.sqlite to be honest.

If anyone wants to chime in, I would be interested to hear your thoughts.

gotson commented 2 years ago

@pyckle I would love to get your opinion on this, as you recently did some changes in those packages/classes.

pyckle commented 2 years ago

Hey. Things are very busy for me for the next few weeks, but I'll take a look at this soon.

Most likely the best way to understand why the code was written like this is the git history, which will take a while to sift through.

gotson commented 2 years ago

Most likely the best way to understand why the code was written like this is the git history, which will take a while to sift through.

that's 10+ years of history 🙃

I plan to have a look at other JDBC drivers, namely H2 and PostgreSQL, for inspiration.

gotson commented 2 years ago

I noticed that the classes are almost never encapsulated. It's quite often that another class will modify a class, via public or protected fields.

For example:

If possible i would like to inch toward proper encapsulation, and when possible immutable objects or properties.

pyckle commented 2 years ago

I tend to agree that they should be put into single classes, as the spec is supposed to be backwards compatible: https://stackoverflow.com/questions/11466367/is-jdbc-4-fully-compliant-with-jdbc-3

I don't see the benefit in separating methods based on which jdbc spec they were introduced in. Also, as you said, as the protected fields are accessed all over the place, there are not encapsulation benefits

gotson commented 2 years ago

I have seen the same split of JDBC classes per version of JDBC in pgsql, it seems that was historical and now they have removed all that and merged it back.

What could make sense is to implement JDBC 4.3 which was added in Java 9. I haven't checked it out so i don't know if it depends on classes that are not in Java 8, in which case we would need to ship a multi-release jar, with a separate implementation for JDBC 4.3.

What i am not sure about is whether we need non-JDBC classes, like CoreStatement, CoreResultSet etc. The library is a JDBC driver firstly, even though it could probably be used without JDBC?

gotson commented 1 year ago

We should probably look at changing the package names too, org.sqlite is way too generic. This was raised in #804.

For instance https://github.com/gwenn/sqlite-jna also uses org.sqlite as the base package.