Skip to content

Make easier to use TNS connections #119

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Make easier to use TNS connections #119

wants to merge 1 commit into from

Conversation

felipebz
Copy link

@felipebz felipebz commented Dec 14, 2018 •
edited
Loading

Currently if the Java property oracle.net.tns_admin isn't set, the cli will define it as "ORACLE_HOME/network/admin". This is an attempt to make easier to use TNS connections as discussed in #112. I separated my changes in two commits for easier discussion.

The first commit is a very straightforward change: if there's a TNS_ADMIN environment variable set and it's a valid path, use it to set oracle.net.tns_admin.

The second commit adds a bit more complexity. The idea is:

  • read all subkeys of HKEY_LOCAL_MACHINE\SOFTWARE\Oracle
  • check for HKEY_LOCAL_MACHINE\SOFTWARE\Oracle\<subkey>\TNS_ADMIN and use it if it's valid
  • check for HKEY_LOCAL_MACHINE\SOFTWARE\Oracle\<subkey>\ORACLE_HOME and if "ORACLE_HOME/network/admin" is valid, use it

The code queries the 64-bit view of the Windows registry (HKEY_LOCAL_MACHINE\SOFTWARE\Oracle) and the 32-bit view (HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Oracle).

But this second commit also adds a dependency of JNA, therefore two new .jar would need to be distributed with utPLSQL-cli: jna-5.1.0.jar (1.41 MB) and jna-platform-5.1.0.jar (2.4 MB). The JNA library can be distributed under the Apache License too, so it's safe to use.

(It looks like there are a hackish way to access the Windows registry without any extra dependency, but... well... it's too hackish IMHO)

@pesse
Copy link
Member

pesse commented Dec 18, 2018

Hi @felipebz ,
thanks for your great work - I was in favor of the hackish registry solution first (because I am careful about adding new dependencies), but JNA looks very mature and safe to rely upon.
I wonder if we should separate that functionality from DataSourceProvider and put it into a util in API.
Reason for this would be that we also have the maven-plugIn which might have similar connection issues.

I'd also suggest that we use EnvironmentVariableUtil.getEnvValue() instead of System.getenv() to get the environmental values for ORACLE_HOME and TNS_ADMIN.
The reason is that EnvironmentVariableUtil also uses Java properties if set, which would also allow us to do some basic Unit-Tests (not including registry though).

What do you think?
Best, Sam

@pesse
Copy link
Member

pesse commented Dec 18, 2018

I thought about the JNA again.
Although it looks mature and a dependency to maintain easily, it comes with nearly +4MB required space. CLI need 550KB at the moment, so we would increase the size of the program by nearly 800% just to read from registry in case we are on a windows platform.
Is that worth it for a single functionality or would the more hacky solution work in that case?
What's your opinion?

@felipebz
Copy link
Author

@pesse Thanks for the feedback. I'll make some changes.

Regarding the hacky solution: it works fine in Java 8, but in Java 11 it causes a java.lang.NoSuchMethodException: java.util.prefs.WindowsPreferences.WindowsRegOpenKey(int, [B, int). So it'll work until you need to support Java 11. Anyway, it looks like that even Oracle is using that hacky solution in SQLcl... If you don't see any problem with this, I could change it.

But then I thought about all of this again today and what if we don't add this Windows registry lookup and keep it simple, only looking for the TNS_ADMIN env var?

@pesse
Copy link
Member

pesse commented Feb 6, 2019

Hey @felipebz
are you still willing to work on this?
Getting TNS_ADMIN/ORACLE_HOME from registry is still a very convenient feature for windows users - I'd really like to include it.
However, I'd rather take the "hacky" solution (even though it doesn't work atm in Java 11) than to introduce another dependency. We should wrap it and only implement the things we need (reading) so we can easily switch this part in a future release if necessary.

@felipebz
Copy link
Author

I'm sorry for the lack of news, @pesse. I'll work on it. ;-)

@pesse
Copy link
Member

pesse commented Feb 12, 2019

No pressure, just wanted to know :)

@felipebz felipebz force-pushed the feature/TNS_ADMIN_support branch from cbb4503 to 3dbe4d5 Compare March 18, 2019 23:36
@jgebal
Copy link
Member

jgebal commented Feb 23, 2020

Is this PR still valid @pesse @felipebz

@pesse
Copy link
Member

pesse commented Mar 10, 2020

The PR would work for Java 8, but it would break compatibility with Java 9+

@felipebz felipebz closed this May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felipebz @pesse @jgebal