Use configured gson instance for toString

[henryju]

This is important when users have registered custom type adapters

Fixes #768

[pisv]

To illustrate, here is how the console log looks like after running the IntegrationTest with the patch in its current form:

Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageSending
WARNING: Unmatched response message: org.eclipse.lsp4j.jsonrpc.messages.ResponseMessage@5e1f1cb
Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageSending
WARNING: Unmatched response message: org.eclipse.lsp4j.jsonrpc.messages.ResponseMessage@a0b2606
Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageReceiving
WARNING: Unmatched response message: {
  "jsonrpc": "2.0",
  "id": "1",
  "result": {
    "value": "BAR"
  }
}
Nov 08, 2023 3:45:17 PM org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer consumeMessageReceiving
WARNING: Unmatched response message: {
  "jsonrpc": "2.0",
  "id": "1",
  "result": {
    "value": "FOO",
    "customAdapter": "/Users/vladimir/git/lsp4j/org.eclipse.lsp4j.jsonrpc"
  }
}

[henryju]

I fixed most of the feedback, except the point about having no more toString on the various Message classes. I will have another look a bit later, to see if that would be possible to inject the jsonHandler into each of those objects when they are created.

[pisv]

To sum up and hopefully clarify, here is what I'd like to see in this PR.

Thanks to the prompt by @szarnekow and the following changes by @henryju, I have been persuaded that there is a clean non-breaking solution to this problem.

So, one of the main goals should be to preserve all of the existing API elements and augment the existing behaviour rather than trying to replace it.

To put it in a perspective, there were no issues reported about this problem since 2019 when the TracingMessageConsumer was introduced. This seems to indicate that for most of the LSP4J clients the current implementation works just fine.

Therefore, let's keep the current API and implementation to avoid breaking any of the existing clients, and just augment it with the enhanced implementation where it is possible, using the old one as a fallback.

To that end, let's first reinstate all of the removed methods and their behaviour, including the static MessageJsonHandler.toString method, Message.toString, CancelParams.toString, ResponseError.toString.

Let's rename the (new) instance MessageJsonHandler.toString method to e.g. format.

Let's use a setter for passing a MessageJsonHandler to both MessageTracer and TracingMessageConsumer as it is the least invasive approach and works just fine in this case.

When no MessageJsonHandler is set on a TracingMessageConsumer, let's fallback to the existing behaviour.

That would be a clean non-breaking solution I'd like to see.

As a bonus, let's make sure that possible exceptions are properly handled in the existing static MessageJsonHandler.toString method, so that it becomes more resilient if some of the required type adapters are missing.

However, I'd consider this bonus part as optional for this PR.

[henryju]

I won't say I am super convinced by keeping dangerous toString implementations, but I will respect your decision. I have some other things to do today, so I will come back to this next week.

[pisv]

I won't say I am super convinced by keeping dangerous toString implementations

On one hand, at least it will not make things any worse for existing clients.

With your current proposal, clients might start to see something like

WARNING: Unmatched response message: Message{id=1}

instead of

WARNING: Unmatched response message: {
  "jsonrpc": "2.0",
  "id": "1",
  "result": {
    "value": "FOO"
  }
}

I don't think it would be fine for most purposes.

On the other hand, we can try to make the 'dangerous' toString implementations less dangerous:

As a bonus, let's make sure that possible exceptions are properly handled in the existing static MessageJsonHandler.toString method, so that it becomes more resilient if some of the required type adapters are missing.

[pisv]

For the sake of convenience, I have pushed what I'd consider a clean patch against main that reflects the points I have depicted above. (It doesn't include the bonus part.)

If you don't mind, let's use it as a baseline for further discussion and development.

[pisv]

Let's see if I can come up with something along the lines sketched out in https://github.com/eclipse-lsp4j/lsp4j/issues/768#issuecomment-1807164354 to make the 'dangerous' toString methods less dangerous...

[pisv]

@jonahgraham Could you please review this PR with a fresh pair of eyes?

We tried to enhance the existing implementation without any breaking changes by passing a specific MessageJsonHandler to the TracingMessageConsumer and Message so that the static MessageJsonHandler.toString(Object) method is called only as a fallback now.

If all else fails, Message.toString(), CancelParams.toString(), and ResponseError.toString() will fallback to an alternative implementation based on a ToStringBuilder.

As far as I can see, the main potential issue with the current patch is that existing reflective code might be broken by the addition of the Message.jsonHandler field. (There is a more detailed comment above about this issue.) Apart from that, it should cause no issues for existing clients.

[pisv]

Regarding the potential issue with existing reflective code, I now think that it would probably be sufficient to just mention it in the CHANGELOG as a potential breaking change, but I would still appreciate an extra pair of eyes to review this PR.

It would be great if we could merge it before the 0.22.0 release.

[jonahgraham]

I added it to the 0.22.0 milestone. It fell off the bottom of my todo list, I will schedule to look at it sometime before the 0.22.0 release. See conversation about timing in #732

[pisv]

I added it to the 0.22.0 milestone. It fell off the bottom of my todo list, I will schedule to look at it sometime before the 0.22.0 release. See conversation about timing in #732

OK. Thank you!

[jonahgraham]

I'll merge this before I build 0.22.0 next ~year~ week unless I hear concerns back.

@henryju and @pisv thank you for your efforts here.

[jonahgraham]

next year

next *week :-)

[jonahgraham]

Thanks everyone. This is now committed and the next release of LSP4J (expected next week) will contain this improvement.