Feature/dynamic credential #274
Conversation
There seems to be a copy paste artifact that seems unnecessary Signed-off-by: calebcodesgud <calwoy1@gmail.com>
Signed-off-by: Caleb Woy calwoy1@gmail.com [resolves r2dbc#273] Signed-off-by: calebcodesgud <calwoy1@gmail.com>
calebcodesgud
force-pushed
the
feature/dynamicCredential
branch
from
df1bc15 to
de9c40a
Compare
|
|
||
| /** Factory for creating {@link Credential} objects */ | ||
| public class CredentialFactory { | ||
|
|
There was a problem hiding this comment.
I'd suggest adding a private constructor that does nothing:
private CredentialFactory() { }Typically, we don't want to instantiate factories, as their methods are all static.
You may want to declare the class itself as final. This indicates that the class is not designed for inheritance.
There was a problem hiding this comment.
agreed, resolved with commit 9ce228d
| * UserPasswordCredentials used to authenticate with a database. | ||
| */ | ||
| public interface UserPasswordCredential extends Credential { | ||
| String user(); |
There was a problem hiding this comment.
Even though it's kind of obvious what the method does, every public API should have a JavaDoc IMHO. The JavaDoc offers a concrete specification of the method's behavior, and specifications are super important for SPIs.
There was a problem hiding this comment.
Agreed, resolved with commit 2b09f64
| @@ -0,0 +1,23 @@ | |||
| package io.r2dbc.spi; | |||
|
|
|||
| public class UserPasswordCredentialImpl implements UserPasswordCredential { | |||
There was a problem hiding this comment.
I think this class can be a package-private (ie: no "public" modifier). Code that is external to the package should use the factory method.
There was a problem hiding this comment.
Agreed, resolved with commit 2b09f64
|
@Michael-A-McMahon @mp911de Do we need anything else on this to consider merging? |
|
@calebcodesgud Thanks for addressing my review comments. I think it all looks good now. Sorry for not responding sooner. |
|
I have this one on my radar for a merge. We currently have no immediate plans for a future release. We're collecting issues and a release could happen in 2-3 years. |
|
Closing this, as it was solve by @Michael-A-McMahon in 137. |
|
Just to be sure: My fix is only for the Oracle driver. But I'll note that my fix hardly does anything which is specific to Oracle Database, although it does reference things which only exist in the Oracle R2DBC code base. Other drivers could maybe add code which is similar to this: (Edit: Clarifying that there are Oracle R2DBC specific things in this implementation) |
Added this pull request implementing @Michael-A-McMahon 's suggested changes for #273 .
Only difference vs original suggestions on #273 is using a class for
UserPasswordCredentialImplrather than arecord, so as not to tie the project to java 14.Still need to add tests for this, was having trouble getting r2dbc-spi-test module running in my local env...