Question about IDidChangeConfigurationHandler

[gep13]

I am trying to implement the IDidChangeConfigurationHandler interface, for use within a Language Server that we are creating here:

https://github.com/gep13/chocolatey-vscode

For validating the Chocolatey Nuspec file when editing within VSCode. In general, things have been going very well with this implementation thanks to some input from @mholo65. The problem stems from the fact that ideally, I would like to have configuration in the following format:

However, when I try to run the Language Server with this configuration, I get the following error:

Unhandled Exception: Newtonsoft.Json.JsonSerializationException: Unexpected token when deserializing object: String. Path 'params.settings.nuspec.suppressedRules[0]', line 1, position 123.
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateDictionary(IDictionary dictionary, JsonReader reader, JsonDictionaryContract contract, JsonProperty containerProperty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.Linq.JToken.ToObject(Type objectType, JsonSerializer jsonSerializer)
   at OmniSharp.Extensions.LanguageServer.Server.LspRequestRouter.FindDescriptor(String method, JToken params) in /_/src/Server/LspRequestRouter.cs:line 77
   at OmniSharp.Extensions.JsonRpc.InputHandler.HandleRequest(String request) in /_/src/JsonRpc/InputHandler.cs:line 198
   at OmniSharp.Extensions.JsonRpc.InputHandler.ProcessInputStream() in /_/src/JsonRpc/InputHandler.cs:line 120
   at System.Threading.Thread.ThreadMain_ThreadStart()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

Digging into the tests, it looks like it is expected that the JSON values are only boolean, number and string, rather than the more complex JSON types:

csharp-language-server-protocol/test/Lsp.Tests/Models/DidChangeConfigurationParamsTests.cs

Lines 18 to 23 in 0832add

	var model = new DidChangeConfigurationParams() {
	Settings = new Dictionary<string, BooleanNumberString>() {
	{ "abc", 1 },
	{ "def", "a" },
	{ "ghi", true },
	}

However, the specification for the Language Server Protocol suggests that it should be of type any:

https://microsoft.github.io/language-server-protocol/specification#workspace_didChangeConfiguration

Is there a reason for not implementing the settings object directly as a JObject, or something similar?

I could work around this using a configuration similar to the following:

However, I was hoping to expand to having a configuration similar to the following extension:

https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker

/cc @tintoy @david-driscoll

[tintoy]

FWIW, I wound up working around that problem by defining my own custom handler that does use JToken :)

https://github.com/tintoy/msbuild-project-tools-server/blob/master/src/LanguageServer.Engine/CustomProtocol/DidChangeConfigurationSettings.cs

...and implementation:

https://github.com/tintoy/msbuild-project-tools-server/blob/dbc74e841eb660826088e9cf94edb63f01631dd7/src/LanguageServer.Engine/Handlers/ConfigurationHandler.cs

...and the Configuration itself:

https://github.com/tintoy/msbuild-project-tools-server/blob/8489b1c7713caa96b6f54cb1f4edbdae50fb93b6/src/LanguageServer.Engine/Configuration.cs

...and how the Configuration is updated from the incoming JSON:

https://github.com/tintoy/msbuild-project-tools-server/blob/16257e0cf27ed22b2ba4eb9aecc0103e2b5dd3df/src/LanguageServer.Engine/CustomProtocol/ProtocolExtensions.cs#L61-L138

[gep13]

@tintoy thanks for getting back to me!!

That looks like exactly what I want/need, if there are no objections, I will unashamedly borrow that 😄

Do you know if there is a reason that the OOTB implementation doesn't provide something like this?

[tintoy]

You’re welcome to - it’s MIT licensed :)

I suspect the answer is historical reasons. Not sure exactly what they are though.

[gep13]

@tintoy said...
You’re welcome to - it’s MIT licensed :)

I did check that before, and I am very grateful that this is how it is licensed. Correct/grateful attribution will be added to the project though.

[gep13]

@tintoy do you know if something has changed in this area...

Your project is using 0.7.9 of OmniSharp and mine is using 0.11.3.

[tintoy]

Yeah probably - I had to make significant extensions to the LSP library for my extension (required functionality wasn’t present at the time) so I haven’t had time to upgrade yet (newer versions are so different from older ones that it’s pradtically a rewrite in some areas).

Basic principle still applies I believe, but interface names might be slightly different :)

[tintoy]

Take a look at the interface definition for the configuration handler in the LSP library - maybe use that as the basis for your new handler?

[gep13]

@tintoy I have "something" compiling 😄 just away to test it just now. Seems like INotificationHandler is now IJsonRpcNotificationHandler.

[tintoy]

I think this is what you're looking for...

https://github.com/OmniSharp/csharp-language-server-protocol/blob/master/src/Protocol/Workspace/Server/IDidChangeConfigurationHandler.cs

[tintoy]

Just define your own parameters model and use that instead.

[gep13]

Going to have to play with this later, as I have other things that I need to be getting on with 😄 Thanks for your help so far though.

I have something that compiles, and doesn't seem to generate any runtime errors, but changing the configuration settings isn't causing anything to be triggered. Will play again later.

[gep13]

@tintoy scratch that...

I have it working! Or at least, I think I do 😄

I will attempt to finalize a PR for my repo, and if you have some time, I might ask that you and @mholo65 have a look at it to make sure I am not doing anything stupid 🐱

[gep13]

@tintoy I have a WIP PR here, where I have the basics in place:

https://github.com/gep13/chocolatey-vscode/pull/135

Would you have some time for a review at some point? Thanks!

[david-driscoll]

definitely still a bug to be fixed... I'm going to try and fix this tonight or this weekend.