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

Log user friendly error if client does not support _java.reloadBundles.command #2370

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

mfussenegger
Copy link
Contributor

The result of executeClientCommand could be a map in the LSP
ResponseError format.
Previously this resulted in a ClassCastException which got logged:

class com.google.gson.internal.LinkedTreeMap cannot be cast to class java.util.ArrayList (com.google.gson.internal.LinkedTreeMap

This gave users the impression that there's a bug.
This changes the logic to instead log the message returned by the
client.

In neovim for example it results in:

Command _java.reloadBundles.command not supported on client

…s.command

The result of `executeClientCommand` could be a map in the LSP
`ResponseError` format.
Previously this resulted in a ClassCastException which got logged:

	class com.google.gson.internal.LinkedTreeMap cannot be cast to class java.util.ArrayList (com.google.gson.internal.LinkedTreeMap

This gave users the impression that there's a bug.
This changes the logic to instead log the message returned by the
client.

In neovim for example it results in:

	Command _java.reloadBundles.command not supported on client
@rgrunber
Copy link
Contributor

rgrunber commented Dec 8, 2022

I actually ran into this when testing another client based on lsp4e so I know this is an issue. Normally we implement some kind of custom client-side setting under the extendedClientCapabilities property we can use to check if the client supports the command. In this case, we don't do that which seems wrong. @jdneo , this should be added in right ?

See https://github.com/eclipse/eclipse.jdt.ls/blob/73fb18c9f500d4a07548030ffc76fe89ed2c217e/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/ClientPreferences.java#L259 for an example.

So maybe something like :

if (preferenceManager.getClientPreferences().isReloadBundlesSupported()) {
    List<String> bundlesToRefresh = (ArrayList<String>) JavaLanguageServerPlugin.getInstance().getClientConnection().executeClientCommand("_java.reloadBundles.command");
}

@rgrunber rgrunber added the bug label Dec 8, 2022
@rgrunber rgrunber added this to the Early January 2023 milestone Dec 8, 2022
@mfussenegger
Copy link
Contributor Author

I think even if gated behind a capability check it would still make sense to check the result type. Clients could respond with a failure even if they support it, due to some runtime error and then you'd have a meaningful error in the log instead of the ClassCastException

@rgrunber
Copy link
Contributor

rgrunber commented Dec 8, 2022

Right. We do the same thing with https://github.com/eclipse/eclipse.jdt.ls/blob/73fb18c9f500d4a07548030ffc76fe89ed2c217e/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/OrganizeImportsHandler.java#L226 so using Object is probably the right approach if we don't have a capability for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants