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

Simplify JDBC classes and interfaces #767

Open
gotson opened this issue Aug 16, 2022 · 7 comments
Open

Simplify JDBC classes and interfaces #767

gotson opened this issue Aug 16, 2022 · 7 comments
Labels
review wanted This needs more eyes

Comments

@gotson
Copy link
Collaborator

gotson commented Aug 16, 2022

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 gotson added the help wanted Contributions are welcome label Aug 16, 2022
@gotson
Copy link
Collaborator Author

gotson commented Aug 16, 2022

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

@gotson gotson added review wanted This needs more eyes and removed help wanted Contributions are welcome labels Aug 16, 2022
@pyckle
Copy link
Contributor

pyckle commented Aug 16, 2022

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

gotson commented Aug 17, 2022

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

gotson commented Oct 27, 2022

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.

@pyckle
Copy link
Contributor

pyckle commented Oct 27, 2022

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

gotson commented Oct 28, 2022

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

gotson commented Nov 7, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review wanted This needs more eyes
Projects
None yet
Development

No branches or pull requests

2 participants