Repository navigation
jdbc: add a proper support for a result set holdability - #134
Conversation
Totktonada
left a comment
Totktonada
left a comment
There was a problem hiding this comment.
Thank you! The patch looks good for me, I just have a question around JDBCBridge and want to kindly push you to describe the changes a bit deeper in the commit message.
It seems that you are partially support Wrapper interface, so, please mention #73 in a commit message. It also seems that you are added some initial support of SQLState codes, so it also worth to touch #119. It would be good to (briefly at least) describe all changes made in a commit in a commit message. Except Wrapper and SQLState I would also mention that here you added proper checks whether a connection is closed for bunch of methods in SQLConnection, SQLStatement, SQLPreparedStatement and SQLResultSet (don't I miss something?).
It would be good to briefly describe changes around JDBCBridge (afaiu, it is a kind of unification return values type?) — at least for me they are not clear. It is not about your patch, but, to be honest, I didn't got the purpose of this class. It looks more as set of helper methods to handle dql/dml queries and I don't see a point why this logic should be separated from its usage. Don't sure, but maybe it should be refactored or properly commented in the future (of course not in the scope of this patch).
I suggest to split non-dependent changes into separate commits where it is possible. It is not much effort during development, but eases writing descriptive commit messages and eases further reading. Often it worth to split out changes that are not change user-visible behaviour from ones that does. Say, fix of many linter warnings is a good candidate to be factored out to a separate commit: it allows a reader to pay more attention to real changes (however if there are ony few, it does not worth). I'll not push you to do it here, just note for next patchsets.
Re commit message: Try to fit a header into 50 symbols if possible (and body to 72 symbols). See also Developer guidelines (it mostly applicable to tarantool-java too).
Also don't mind to ping me if'll not update the JDBC status wiki page after the merge.
18ba934 to
5a869e2
Compare
2f5c548 to
590936c
Compare
Right now we only can support HOLD_CURSORS_OVER_COMMIT due to lack of Tarantool transaction support and, possibly, cursors. So, in terms of HOLD_CURSORS_OVER_COMMIT we always load a result set completely and it can be used as long as it is opened. Implement the holdability support (next HS) for SQLDatabaseMetaData in part of getting a default holdability and checking proper support. Implement HS for SQLConnection in part of producing new statements and prepared statements (but excluding CallableStatements due to lack of implementation). Implement HS for SQL(Prepared)Statement as well as for SQLResultSet which is produced by the statements. Some part of this feature requires the implementation of java.sql.Wrapper for SQLConnection and SQL(Prepared)Statement. So now they fully implement the interface. Add missed checks for an open status of JDBC components which are required by the specification. Some methods start returning appropriate SQLException subclasses when corresponding errors occur. Add SQLStates enumeration to help to produce the errors with the standard SQL states. Finally, JDBCBridge is no longer aware of the SQLResultSet class. Only Statement implementations are responsible for building of new result sets according to the specification design. Next plan is to completely avoid JDBCBridge logic and transfer it to an inner helper inside the Statement implementations. Closes: #87 Affects: #73, #119
590936c to
268eafa
Compare
added support for HOLD_CURSORS_OVER_COMMIT as a default resultSet holdability.
Closes: #87.