Exception raised if workspace/configuration request from server to client does not get passed a section

[bzoracler]

When retrieving configuration settings through workspace/configuration, the language server specifications allows leaving section unspecified:

export interface ConfigurationItem {
	/**
	 * The scope to get the configuration section for.
	 */
	scopeUri?: URI;

	/**
	 * The configuration section asked for.
	 */
	section?: string;
}

However, if section is not provided, LSP4IJ raises an exception¹.

I think a default behaviour should be implemented when section is not specified - such as (1) returning the entire configuration object, which is my preference, or (2) returning null.

com.redhat.devtools.lsp4ij.client.LanguageClientImpl.findSettings

    protected Object findSettings(String section) {
        var config = createSettings();
+       if (section == null) {
+           return config;
+       }
        if (config instanceof JsonObject json) {
            String[] sections = section.split("[.]");
            return findSettings(sections, json);
        }
        return null;
    }

---

1. Stack trace:
   

   lsp4ij/src/main/java/com/redhat/devtools/lsp4ij/client/LanguageClientImpl.java

   Line 227 in 6a561e0

   String[] sections = section.split("[.]");

   Internal error: java.lang.NullPointerException: Cannot invoke "String.split(String)" because "section" is null
   
   java.util.concurrent.CompletionException: java.lang.NullPointerException: Cannot invoke "String.split(String)" because "section" is null
      at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315)
      at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320)
      at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1770)
      at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1760)
      at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:507)
      at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1491)
      at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:2073)
      at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2035)
      at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:187)
   Caused by: java.lang.NullPointerException: Cannot invoke "String.split(String)" because "section" is null
      at com.redhat.devtools.lsp4ij.client.LanguageClientImpl.findSettings(LanguageClientImpl.java:227)
      at com.redhat.devtools.lsp4ij.client.LanguageClientImpl.lambda$configuration$9(LanguageClientImpl.java:203)
      at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
      ... 6 more
   

[angelozerr]

Could you create a PR please

[angelozerr]

I think a default behaviour should be implemented when section is not specified - such as (1) returning the entire configuration object, which is my preference, or (2) returning null.

It would be nice to clarify that. Could you create an issue at https://github.com/microsoft/vscode-languageserver-node/issues please.

[bzoracler]

TL;DR the LSP specs leave the behaviour undefined, so the decision is up to LSP4IJ on what to do upon a null section request (but definitely not raise an exception). LSP4IJ must handle a null section, and IMO it should:

1. Decide on a default behaviour if null is provided as the section;
2. Advertise this as the default behaviour, informing server implementations that they can expect this behaviour if they request null;
3. Have this default behaviour overridable if someone wants to make an LSP4IJ plugin.

---

From the specification (emphases mine):

A ConfigurationItem consists of the configuration section to ask for and an additional scope URI. The configuration section asked for is defined by the server and doesn’t necessarily need to correspond to the configuration store used by the client. So a server might ask for a configuration cpp.formatterOptions but the client stores the configuration in an XML store layout differently. It is up to the client to do the necessary conversion.

I interpret this as: The server must advertise what section it's going to request (such as null), and a compatible client must handle it. So, if the server requests a null section, it's up to the client what to return to the server; what the server/client should do for any particular configuration section (including null) isn't specified by the language server protocol.

In light of this, whether null is a sensible request from the server or not depends on the client; for generic clients which can configure one of multiple servers using a shared configuration file (e.g. VS Code's settings.json), it doesn't make sense for null to return the entire configuration as it would contain irrelevant items. LSP4IJ's user-defined Langauge Server UI isolates each language server with their own configuration settings, so returning the entire configuration may be a sensible interpretation if the server asks for null. However, since the server must advertise what it asks for, if the server advertises that it asks for null, it may not be generically compatible with other clients, but that's the server's implementation problem, not the client's.

See also some discussion about the uncertainty of what section means in microsoft/language-server-protocol#972.

[angelozerr]

I think a default behaviour should be implemented when section is not specified - such as (1) returning the entire configuration object, which is my preference, or (2) returning null.

Why it is your preferred behavior? Me I think it should be better to return null. @fbricon have you some opinion about that?

[bzoracler]

I have a weak preference for sending the entire configuration object, because I believe that configuration requests usually request a statically-defined section, so a null section request would be deliberate.

If a null section is deliberately requested, it doesn't seem useful to return null. Since there isn't a LSP standard for returning a whole configuration object upon requesting a section, this seems like an opportunity to both (1) make a null request useful, and (2) provide a way to retrieve a whole configuration object.

[angelozerr]

Thanks for your reply. Do you know the behaviour of vscode?

Could you create a PR this week, because we will release 0.6.0 next week?

[bzoracler]

Do you know the behaviour of vscode?

I didn't run it, but based on https://github.com/microsoft/vscode-languageserver-node/blob/62b3198a95ac9af13e04a27248b28951064546f9/client/src/common/configuration.ts#L75-L83, it looks like passing a falsey value to section is interpreted by the VSCode main client implementation to retrieve the entire configuration.

I guess my initial intuition about VSCode wasn't correct:

it doesn't make sense for null to return the entire configuration as it would contain irrelevant items.

[angelozerr]

If I understand your PR have the same behaviour than vscode, right?

[bzoracler]

If I understand your PR have the same behaviour than vscode, right?

Yes, that's my understanding of the TypeScript source. However, I didn't have a chance to set up a VSCode language client/server project to confirm this behaviour.