Adding command for New Database Connection #3358

jhorvath · 2021-12-08T17:06:23Z

Adding command for New Database Connection to java.lsp.server

java/java.lsp.server/nbproject/project.xml

JaroslavTulach

Fine at first round:

properly version the DB module API changes
avoid needless changes in platform.properties
write a test for the new method
try to avoid references to "Micronaut" in this generic extension

JaroslavTulach · 2021-12-09T17:02:01Z

ide/db/src/org/netbeans/api/db/explorer/ConnectionManager.java

+     * @param dbconn
+     * @return List of schemas or null if connection failed
+     */
+    public List<String> getSchemas(DatabaseConnection dbconn) {


This is an API change. It needs @since tag, it needs a note in apichanges.xml.

JaroslavTulach · 2021-12-09T17:02:51Z

ide/db/src/org/netbeans/api/db/explorer/ConnectionManager.java

+     */
+    public List<String> getSchemas(DatabaseConnection dbconn) {
+        if (SwingUtilities.isEventDispatchThread()) {
+            throw new IllegalStateException("This method can not be called on the event dispatch thread."); // NOI18N


cannot is a single word - maybe you want to spell this (unusual?) restriction in the Javadoc of the method.

JaroslavTulach · 2021-12-09T17:03:54Z

ide/db/src/org/netbeans/api/db/explorer/ConnectionManager.java

+     * Retrieves a list of database schemas available for given connection.
+     * 
+     * @param dbconn
+     * @return List of schemas or null if connection failed


I usually use {@code null} to print it in a different font. Shouldn't an exception be raised if the connection fails?

JaroslavTulach · 2021-12-09T17:04:15Z

ide/db/src/org/netbeans/modules/db/explorer/DbSchemasQuery.java

+                    }
+                }
+            }
+        } catch (DatabaseException | SQLException ex) {


Why not propagate the exception to the caller?

java/java.lsp.server/nbcode/nbproject/platform.properties

JaroslavTulach · 2021-12-09T17:06:28Z

java/java.lsp.server/nbcode/nbproject/platform.properties

+    org.openide.options,\
+    org.openide.util.enumerations,\
+    org.openidex.util
+nbjdk.active=JDK_11


Avoid nbjdk.active and nbplatform.active.

JaroslavTulach · 2021-12-09T17:07:47Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java

@@ -858,6 +859,7 @@ protected LanguageClient client() {
    public static final String JAVA_SUPER_IMPLEMENTATION =  "java.super.implementation";
    public static final String JAVA_SOURCE_FOR =  "java.source.for";
    public static final String GRAALVM_PAUSE_SCRIPT =  "graalvm.pause.script";
+    public static final String DB_ADD_CONNECTION =  "micronaut.add.db.connection";


Why is this command prefixed by micronaut? The DB explorer is completely micronaut unrelated, right?

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ShowInputBoxParams.java

JaroslavTulach · 2021-12-09T17:11:34Z

java/java.lsp.server/vscode/package.json

+      {
+                                "command": "micronaut.add.db.connection",
+                                "title": "Add Database Connection",
+ 				"category": "Micronaut"


It'd be better if Micronaut extension were in enterprise/micronaut module, not here.

java/java.lsp.server/vscode/src/nbcode.ts

ide/db/src/org/netbeans/modules/db/explorer/DbSchemasQuery.java

ide/db/src/org/netbeans/api/db/explorer/ConnectionManager.java

JaroslavTulach

The most recent version eliminates the API changes altogether, great! The "Add Database Connection" command looks fine to me. Please squash a bit to eliminate the add/remove of platform.properties entries and the API attempt from the history after merge.

java/java.lsp.server/nbcode/nbproject/platform.properties

java/java.lsp.server/vscode/package.json

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/db/DatabaseCommands.java

matthiasblaesing

Thanks for the updates, looks sane to me.

matthiasblaesing · 2021-12-19T16:54:14Z

@jhorvath Before this is merged:

please ensure the author information is consistent, there is at least one merge commit, that only holds your username and a different email adress. I assume, this is created by github.
after final review, please rebase onto master and squash into one commit (this will most likely fix the author point)
I assume you read the license statement in the files you edited/added and you are in a position to donate the changes to the Apache Foundation and do so. (or you have an ICLA on file?)

jhorvath · 2021-12-19T17:58:51Z

I have read and agree to the license statement.

sdedic · 2021-12-20T08:41:59Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/db/DatabaseCommands.java

+                FileObject jarFO = URLMapper.findFileObject(jars[0]);
+                if (jarFO != null && jarFO.isValid()) {
+                    items.add(
+                            new QuickPickItem(drivers[i].getName(), null, drivers[i].getDisplayName() + " (" + drivers[i].getClassName() + ")", false, i) // NOI18N


Hm; on the second thought, consider to send drivers[i].getName() to identify the selected driver in then clause. Not necessary, unless another vscode instance adds a driver.

sdedic · 2021-12-20T08:44:08Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/db/DatabaseCommands.java

+                                }
+                                client.showInputBox(new ShowInputBoxParams(
+                                        Bundle.MSG_EnterPassword(), "", true)).thenAccept((password) -> { //NOI18N
+                                    if (password == null) {


Is password required for all databases ? E.g. for mysql, password-less logins can be allowed (?)

sdedic · 2021-12-20T08:46:12Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/db/DatabaseCommands.java

+                                        return;
+                                    }
+                                    DatabaseConnection dbconn = DatabaseConnection.create(driver, u, username, null, password, true);
+                                    try {


If you would like to limit that deep nesting, this is the point where the CompletableFuture can yield a DatabaseConnection instance and we can with thenAccept at top level again :)

sdedic · 2021-12-20T08:47:02Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/db/DatabaseCommands.java

+                ResultSet rs = dbMetaData.getSchemas();
+                if (rs != null) {
+                    while (rs.next()) {
+                        schemas.add(rs.getString(1).trim());


is index safer than column name in this case ?

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ShowInputBoxParams.java

java/java.lsp.server/vscode/src/protocol.ts

sdedic · 2021-12-20T08:56:51Z

java/java.lsp.server/vscode/src/extension.ts

@@ -756,7 +756,7 @@ function doActivateWithJDK(specifiedJDK: string | null, context: ExtensionContex
                return selected ? Array.isArray(selected) ? selected : [selected] : undefined;
            });
            c.onRequest(InputBoxRequest.type, async param => {
-                return await window.showInputBox({ prompt: param.prompt, value: param.value });
+                return await window.showInputBox({ prompt: param.prompt, value: param.value, password: param.password });


Bump required spec version in nbcode/integration module.

java/java.lsp.server/nbcode/nbproject/platform.properties

…d java.lsp.server version

…ional

…tion

sdedic · 2021-12-21T08:51:53Z

I would ignore the commit validation on Mac. Failed on a timeout.

sdedic assigned jhorvath Dec 8, 2021

sdedic requested review from dbalek and JaroslavTulach and removed request for dbalek December 8, 2021 17:11

sdedic reviewed Dec 8, 2021

View reviewed changes

java/java.lsp.server/nbproject/project.xml Outdated Show resolved Hide resolved

sdedic added enhancement LSP [ci] enable Language Server Protocol tests labels Dec 8, 2021

JaroslavTulach suggested changes Dec 9, 2021

View reviewed changes

matthiasblaesing requested changes Dec 9, 2021

View reviewed changes

ide/db/src/org/netbeans/modules/db/explorer/DbSchemasQuery.java Outdated Show resolved Hide resolved

ide/db/src/org/netbeans/api/db/explorer/ConnectionManager.java Outdated Show resolved Hide resolved

JaroslavTulach approved these changes Dec 14, 2021

View reviewed changes

java/java.lsp.server/nbcode/nbproject/platform.properties Outdated Show resolved Hide resolved

java/java.lsp.server/nbcode/nbproject/platform.properties Outdated Show resolved Hide resolved

java/java.lsp.server/vscode/package.json Show resolved Hide resolved

jhorvath requested a review from matthiasblaesing December 16, 2021 14:51

matthiasblaesing reviewed Dec 16, 2021

View reviewed changes

jhorvath requested a review from matthiasblaesing December 19, 2021 15:41

matthiasblaesing approved these changes Dec 19, 2021

View reviewed changes

Adding command for New Database Connection

28a654a

jhorvath force-pushed the db-connection-lsp branch from 4d43197 to 28a654a Compare December 20, 2021 08:47

sdedic requested changes Dec 20, 2021

View reviewed changes

jhorvath added 6 commits December 20, 2021 11:34

removed unnecessary dependency org.netbeans.modules.db.core, increase…

a2e6f87

…d java.lsp.server version

remove unnecessary db.dataview module

ecd45b6

Use CodeActionsProvider for new connection command, make password opt…

b3a7e52

…ional

properly register the command

9837030

update org.netbeans.modules.java.lsp.server version in nbcode/integra…

e33bee5

…tion

to be squashed

ec9be0d

sdedic approved these changes Dec 20, 2021

View reviewed changes

to be squashed

1335922

sdedic merged commit 9dc2a2e into apache:master Dec 21, 2021

neilcsmith-net added this to the NB13 milestone Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding command for New Database Connection #3358

Adding command for New Database Connection #3358

jhorvath commented Dec 8, 2021

JaroslavTulach left a comment

JaroslavTulach Dec 9, 2021

JaroslavTulach Dec 9, 2021

JaroslavTulach Dec 9, 2021

JaroslavTulach Dec 9, 2021

JaroslavTulach Dec 9, 2021

JaroslavTulach Dec 9, 2021

JaroslavTulach Dec 9, 2021

JaroslavTulach left a comment

matthiasblaesing left a comment

matthiasblaesing commented Dec 19, 2021

jhorvath commented Dec 19, 2021

sdedic Dec 20, 2021

sdedic Dec 20, 2021

sdedic Dec 20, 2021

sdedic Dec 20, 2021

sdedic Dec 20, 2021

sdedic commented Dec 21, 2021

Adding command for New Database Connection #3358

Adding command for New Database Connection #3358

Conversation

jhorvath commented Dec 8, 2021

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

matthiasblaesing left a comment

Choose a reason for hiding this comment

matthiasblaesing commented Dec 19, 2021

jhorvath commented Dec 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdedic commented Dec 21, 2021