Skip to content

Fix connection string pattern for service names containing "_" or "." #15

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

Closed

Conversation

dmvolodin
Copy link

Service name in connect string may contains "_" or "."(for domain specify)

Copy link
Member

@jgebal jgebal left a comment

Choose a reason for hiding this comment

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

The same issue is with password regex.
I wonder if we should bother at all.
Maybe we should have 3 separate parameters and don't worry about splitting one parameter into 3 variables.

@viniciusam
Copy link
Member

Refactored how the connection string is parsed, see #16.

@jgebal
Copy link
Member

jgebal commented Jun 18, 2017

I like the refactoring. No need to use regex to parse and split connection string.

@dmvolodin
Copy link
Author

Great!
By the way, as i can see, we can not pass SID inside connect string.
It may be very simple with 3(4) separate parameters.
Something like this:

public String getConnectionUrl() throws Exception {
        if (getSid() != null && getServiceName() != null) {
            throw new Exception("SID and ServiceName can not be set both");
        }
        if (getSid() != null) {
            return String.format("jdbc:oracle:thin:@%s:%d:%s", getHost(), getPort(), getSid());
        } else {
            return String.format("jdbc:oracle:thin:@//%s:%d/%s", getHost(), getPort(), getServiceName());
        }
    }

@viniciusam
Copy link
Member

Thanks for your contribution @dmvolodin. I will merge the refactoring changes and create an issue for the SID support.

@viniciusam viniciusam closed this Jun 19, 2017
@dmvolodin dmvolodin deleted the bugfix/connect_string_pattern branch June 28, 2017 16:07
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.

3 participants