sqlite-jdbc icon indicating copy to clipboard operation
sqlite-jdbc copied to clipboard

Simplify JDBC classes and interfaces

Open gotson opened this issue 3 years ago • 7 comments

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:

  • abstract class CoreStatement
  • abstract class JDBC3Statement extends CoreStatement
  • class JDBC4Statement extends JDBC3Statement implements Statement

And for PreparedStatement:

  • abstract class CorePreparedStatement extends JDBC4Statement
  • abstract class JDBC3PreparedStatement extends CorePreparedStatement
  • class JDBC4PreparedStatement extends JDBC3PreparedStatement implements PreparedStatement, ParameterMetaData

That seems like a mess to me:

  • JDBC3Statement implements some methods from Statement while it doesn't implement the interface
  • CorePreparedStatement in core package depends on jdbc4 package

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:

  • JDBC 4 was introduced in version 3.8.2 along with Java 6
  • moved to Java 8 in version 3.32.3.3

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 avatar Aug 16 '22 06:08 gotson

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

gotson avatar Aug 16 '22 09:08 gotson

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.

pyckle avatar Aug 16 '22 23:08 pyckle

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 avatar Aug 17 '22 02:08 gotson

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:

  • when preparing a statement, the DB class is modifying the Statement object
  • the ResultSet has a reference to the Statement, and uses it to access the Statement's pointer

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

gotson avatar Oct 27 '22 07:10 gotson

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

pyckle avatar Oct 27 '22 19:10 pyckle

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 avatar Oct 28 '22 01:10 gotson

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.

gotson avatar Nov 07 '22 08:11 gotson