-
Notifications
You must be signed in to change notification settings - Fork 120
feat: enable setting ipType configuration option for SQL Server connector #936
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
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
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.
Good start. Stay away from parsing URLs without a library. Also, more tests and meaner tests.
jdbc/sqlserver/src/main/java/com/google/cloud/sql/sqlserver/SocketFactory.java
Show resolved
Hide resolved
public SocketFactory(String instanceName) { | ||
this.props.setProperty(CoreSocketFactory.CLOUD_SQL_INSTANCE_PROPERTY, instanceName); | ||
public SocketFactory(String socketFactoryConstructorArg) { | ||
String[] s = socketFactoryConstructorArg.split("\\?"); |
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.
You may want to use String.indexOf('?') instead of String.split(). Customer may include more than one '?' characters and you don't really want to split on all '?', just the first one.
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.
What would be the use case for more than one ?
character? Wouldn't that be a malformed string (which we should probably catch with an error). so far the only allowed args are true/false for enableIamAuth
and public/private for ipType.
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.
Can we use ParseURI?
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.
jdbc/sqlserver/src/test/java/com/google/cloud/sql/sqlserver/JdbcSqlServerUnitTests.java
Show resolved
Hide resolved
@@ -26,7 +26,8 @@ | |||
public class SocketFactory extends javax.net.SocketFactory { | |||
|
|||
private static final Logger logger = Logger.getLogger(SocketFactory.class.getName()); | |||
private Properties props = new Properties(); | |||
// props are protected, not private, so that they can be accessed from unit tests | |||
protected Properties props = new Properties(); |
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.
public SocketFactory(String instanceName) { | ||
this.props.setProperty(CoreSocketFactory.CLOUD_SQL_INSTANCE_PROPERTY, instanceName); | ||
public SocketFactory(String socketFactoryConstructorArg) { | ||
String[] s = socketFactoryConstructorArg.split("\\?"); |
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.
Can we use ParseURI?
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.
LGTM
fixes #359
Using a query string format for the socketFactoryConstructorArg allows passing in more configuration arguments without breaking users of previous versions