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

Add sqlcl support for oracle #135

Open
walkabout21 opened this issue May 23, 2023 · 13 comments
Open

Add sqlcl support for oracle #135

walkabout21 opened this issue May 23, 2023 · 13 comments

Comments

@walkabout21
Copy link

feature request

Include sqlcl as an adapter option for oracle databases. It has more modern features compared to sqlplus and is standalone instead of requiring a separate oracle client to run.

@walkabout21
Copy link
Author

@tpope I have a working implementation of a sqlcl adapter but I was wondering about naming convention before I submit a PR. Using an underscore like oracle_sqlcl breaks the adapter lookup and obviously replacing the oracle adapter with one that uses sqlcl would be a bad idea. Not sure how to handle this since your naming convention is all systems based instead of tool based.

@tpope
Copy link
Owner

tpope commented May 30, 2023

You should be able to name the file oracle_sqlcl.vim and refer to it via oracle-sqlcl://. This is a technical answer; I'm not sure I'm on board with actually supporting sqlcl this way.

@walkabout21
Copy link
Author

I'm open to suggestions. It seemed the most straightforward way, especially if it needed to be implemented later in a downstream plugin like dadbod-ui.

@tpope
Copy link
Owner

tpope commented May 30, 2023

I don't know; if possible I'd like the existing adapter to support both. Feel free to show me what you have and I can assess.

@walkabout21
Copy link
Author

The connect strings and base commands are the same so I guess the oracle adapter could just test for the sqlcl binary and then default to sqlplus if not found. That makes a decision on the hierarchy though. I will play around with it and see what breaks.

@walkabout21
Copy link
Author

Unfortunately the runtime isn't tested until after the adapter is picked up in adapter.vim, so checking the runtime at the oracle adapter and switching the cmd doesn't seem feasible. The only real change is switching the dbext ora default to 'sql' instead of 'sqlplus' so it's not a major code change. I guess I could just alias 'sql' as 'sqlplus' in my .bashrc and get the same result.

@tpope
Copy link
Owner

tpope commented May 31, 2023

I don't understand what you mean by "the runtime isn't tested".

@walkabout21
Copy link
Author

Admittedly I am not a vimscript dev, but my understanding of the code is that the command is set by calling the functions in oracle.vim, the executable is checked at runtime in the db#connect function in db.vim, so it would not be feasible to catch "executable not found" when db is run and pass that error back to the functions in the oracle.vim context to change the default cmd.

@walkabout21
Copy link
Author

Atleast not without writing specific Oracle exceptions into db#connect which would be ugly.

@walkabout21
Copy link
Author

I guess I can do an initial executable() check in the function that sets the default cmd. I'll try that.

@walkabout21
Copy link
Author

adding an executable() check for 'sql' to the db#adapter#oracle#interactive function like the following works

function! db#adapter#oracle#interactive(url) abort
  let url = db#url#parse(a:url)
  let sql_bin = 'sqlplus'
  if executable('sql')
    let sql_bin = 'sql'
  endif
  return [get(g:, 'dbext_default_ORA_bin', sql_bin ), '-L',
        \ get(url, 'user', 'system') . '/' . get(url, 'password', 'oracle') .
        \ '@' . s:conn(url)]
endfunction

But it does break the dadbod-ui table lookup @kristijanhusak
I'm not sure why other than sqlcl output is formatted differently than sqlplus output.

@kristijanhusak
Copy link
Contributor

@walkabout21 If this gets solved here we can open up an issue in dadbod-ui to solve it there also.

@walkabout21
Copy link
Author

@tpope added PR for this change. Checked docs but there is no reference to the default binaries so I did not see any need for updates. Left SQLPLUS as the default since missing executable error is handled in a different context than the adapter.

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

No branches or pull requests

3 participants