tpope / vim-dadbod

dadbod.vim: Modern database interface for Vim
https://www.vim.org/scripts/script.php?script_id=5665
3.75k stars 132 forks source link

Add sqlcl support for oracle #135

Open walkabout21 opened 1 year ago

walkabout21 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

walkabout21 commented 1 year ago

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 commented 1 year ago

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

walkabout21 commented 1 year ago

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

walkabout21 commented 1 year ago

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 commented 1 year ago

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

walkabout21 commented 1 year ago

@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.