-
Notifications
You must be signed in to change notification settings - Fork 912
Improve Add Database Connection dialog for SQLite, DuckDB, BigQuery, and Snowflake #8189
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
Conversation
6bc27e1 to
7ffd540
Compare
7c057f6 to
0b21eb1
Compare
|
I added another commit which recognizes Snowflake JDBC URLs, just since we're already making enhancements to this piece of code. |
0e58cc0 to
c4705c3
Compare
|
I squashed the previous review edits, and also added a separate commit which recognizes Google BigQuery JDBC URLs. |
3894882 to
b8a0fc0
Compare
|
@eirikbakke could you rebase this PR? We might be able to squeeze this in. |
b8a0fc0 to
46d1bf5
Compare
Sure, done! Do you prefer a single squashed commit, or keep the three commits separate as they currently are? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a small problem with h2 (the file part of the URL is not a full path) and nitpick regarding the JdbcUrl constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer a single squashed commit, or keep the three commits separate as they currently are?
I would probably squash it in this case, but do what you prefer.
ide/db/src/org/netbeans/modules/db/explorer/dlg/NewConnectionPanel.java
Outdated
Show resolved
Hide resolved
|
Thanks for review suggestions; I pushed a commit which incorporates them (will squash all commits into one before merging). I also fixed two flaky tests in the same module. ("Fixed" by essentially calling Thread.sleep... a bit ugly, but there was already a convention for doing so in the same test file.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Looks sane to me.
|
this looks ready other than needing squash and rebase. |
…SQLite and DuckDB URL templates. Also validate the selected file in the SQLite and DuckDB cases. Also have the New Connection wizard recognize the Snowflake and Google BigQuery JDBC drivers and JDBC URLs. Add a logging statement for one case involving JDBC connection errors that extend from Error rather than Exception (such as NoClassDefFoundError originating from a JDBC driver).
9ced37c to
bf46f66
Compare
|
Thanks for reviewing! Merged! |

The "Add Database Connection" dialog supports various kinds of database-specific connection fields (host, port, database name etc.) These are automatically substituted into the JDBC driver specific JDBC URL.
This PR adds support for a "File" field, which will be shown if the user is using the JDBC drivers for SQLite or DuckDB. There is a "Browse" button that can be used to pick the file from a file browser. Furthermore, the username and password fields are hidden for these drivers, which are known not to require this kind of authentication.
For SQLite and DuckDB, there's a warning if the selected file is not of the right kind. (Useful because SQLite files do not have any kind of standardized file extension.)
For comparison, this is what the Add Database Connection dialog looked like before, for SQLite:
(There are some proprietary JDBC drivers for SQLite that do allow a password to be used for encryption, but in this rare case the user could use a connection property to pass this information instead.)