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

feat: enable setting ipType configuration option for SQL Server connector #936

Merged
merged 8 commits into from
Dec 7, 2022

Conversation

shubha-rajan
Copy link
Contributor

@shubha-rajan shubha-rajan commented Aug 3, 2022

fixes #359
Using a query string format for the socketFactoryConstructorArg allows passing in more configuration arguments without breaking users of previous versions

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Aug 3, 2022

🤖 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@shubha-rajan shubha-rajan marked this pull request as ready for review August 4, 2022 22:04
@shubha-rajan shubha-rajan requested a review from a team August 4, 2022 22:04
Copy link
Collaborator

@hessjcg hessjcg left a 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.

public SocketFactory(String instanceName) {
this.props.setProperty(CoreSocketFactory.CLOUD_SQL_INSTANCE_PROPERTY, instanceName);
public SocketFactory(String socketFactoryConstructorArg) {
String[] s = socketFactoryConstructorArg.split("\\?");
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@kurtisvg kurtisvg Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use ParseURI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use ParseURI?

Unfortunately not - I tried it and the : character in the instance name causes issues, and we can't make users escape the instance name without breaking existing users (see check failures in 53b5936)

@shubha-rajan shubha-rajan marked this pull request as draft August 4, 2022 22:53
@@ -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();
Copy link
Contributor

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("\\?");
Copy link
Contributor

@kurtisvg kurtisvg Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use ParseURI?

@shubha-rajan shubha-rajan marked this pull request as ready for review September 14, 2022 18:04
@shubha-rajan shubha-rajan added kokoro:force-run Add this label to force Kokoro to re-run the tests. tests: run Label to trigger Github Action tests. labels Sep 14, 2022
@github-actions github-actions bot removed the tests: run Label to trigger Github Action tests. label Sep 14, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2022
Copy link
Collaborator

@hessjcg hessjcg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shubha-rajan shubha-rajan enabled auto-merge (squash) December 7, 2022 09:07
@shubha-rajan shubha-rajan merged commit e76518d into main Dec 7, 2022
@shubha-rajan shubha-rajan deleted the ipTypes branch December 7, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for specifying IP Types for SQL Server
4 participants