Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: add CommitStats to Connection API #608
feat!: add CommitStats to Connection API #608
Changes from all commits
3872199
def8768
5372dae
34fbda6
8f61c2a
47b57ea
a9014bc
1063ced
2f56c29
041b34d
df7273a
8299054
9b710a7
9a2e555
57a10a8
88c2d83
043555d
8c15641
919cf02
97ec917
afeb0fd
8524526
06b3e22
1c17d16
664b87d
6573a0f
0b448c3
83039fb
57ea714
74f5951
1b077e4
ea061f8
51fb048
66c5f88
bd9bf11
b351d08
da0e434
1d17973
a2531cb
c01152d
bf0a7bd
48ef21b
1f33c42
7d45ace
fcb9eda
682b849
0e341a9
7540c31
f86255e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't see why both getCommitResponseOrNull and getCommitResponse exist. Why not just getCommitResponse that returns null if there's no unit of work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two methods serve two different APIs that behave differently:
Connection
interface contains several methods that return the current state of the connection or of the last statement. The chosen pattern for that API is to throw an exception if theConnection
is not in a valid state for the method to be called. I don't think we should deviate from that pattern for one method. This also somewhat reflects the standard in JDBC, where most methods also throw an exception if theConnection
is not in a valid state for the method. See for example the commit() method. (Although this is not 100% true in all cases, as some other JDBC methods have a different contract, and allows you to call it in what seems to be an illogical state. Calling setAutoCommit is for example allowed during a transaction, and will automatically commit the transaction if the auto commit mode).null
when a value is requested that is not available at that moment. This SQL parser uses thegetCommitResponseOrNull
and other similar methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it useful to check the connection state here? What breaks if this field is set or read on a closed connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also follows the standard of the other methods in this class, as well as the default behavior in JDBC, where all methods throw an exception if it is called on a closed connection. See for example getAutoCommit().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a code smell suggests that perhaps getCommitResponse does not belong in the superclass/interface in the first place since it doesn't apply to all subclasses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a package private class (extending from a package private interface), so it is not something that client applications will deal with. Client applications only interact with the Connection interface. (And client applications in this case is only intended to be frameworks /drivers that implement a declarative connection-type interface, such as JDBC.)
These declarative style APIs often mean that certain methods are only applicable in certain states, and this is what is also reflected in these classes. The method that client applications will interact with is Connection#getCommitResponse(). This method will delegate the actual response to the at that moment active
UnitOfWork
on theConnection
. The differentUnitOfWork
implementations will respond based on what they support, which may also be that they do not support it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered returning an empty object or null instead?
If clients really won't see this, fine. However it's not enough that this class is non-public. They could still invoke this method if they get an instance of it, even while only knowing its supertype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning null or an empty object here would only move the logic to the
ConnectionImpl
class, which should throw an exception if it is not in a valid state to returnCommitStats
at that moment (for example because a DDL batch is active). This logic is therefore delegated to the concrete implementations ofUnitOfWork
.This class and its super types are all package private and or not accessible from outside the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doubles my suspicion that this doesn't belong in the superclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.