Skip to content

fix: Use a Gson singleton which knows about the types from LSP4J#1414

Merged
rubenporras merged 1 commit intoeclipse-lsp4e:mainfrom
FlorianKroiss:shared-configured-gson-instance
Nov 24, 2025
Merged

fix: Use a Gson singleton which knows about the types from LSP4J#1414
rubenporras merged 1 commit intoeclipse-lsp4e:mainfrom
FlorianKroiss:shared-configured-gson-instance

Conversation

@FlorianKroiss
Copy link
Copy Markdown
Contributor

@FlorianKroiss FlorianKroiss commented Nov 21, 2025

Gson instances are thread safe, so we should create only a single instance.
Gson needs to know how to deserialize the Either type, which seems to happen when configuring the EitherTypeAdapter from LSP4J

Fixes #1413

@FlorianKroiss
Copy link
Copy Markdown
Contributor Author

Should we move the Gson instance somewhere else so we can test deserialization?

@FlorianKroiss FlorianKroiss marked this pull request as draft November 21, 2025 13:15
@rubenporras
Copy link
Copy Markdown
Contributor

Should we move the Gson instance somewhere else so we can test deserialization?

For me, yes, we can move it to an internal package.

@angelozerr
Copy link
Copy Markdown
Contributor

angelozerr commented Nov 21, 2025

@FlorianKroiss your PR will not cover all usecases (like other registration option like completion, etc). See my comment at #1413 (comment)

@FlorianKroiss FlorianKroiss force-pushed the shared-configured-gson-instance branch from 68016df to 678f4d6 Compare November 22, 2025 18:52
@FlorianKroiss FlorianKroiss changed the title fix: Use a single Gson instance which knows about the Either type fix: Use a Gson singleton which knows about the types from LSP4J Nov 22, 2025
@FlorianKroiss
Copy link
Copy Markdown
Contributor Author

@angelozerr Thanks for your insights. I used the same trick as you to obtain a properly configured Gson instance.

@FlorianKroiss FlorianKroiss force-pushed the shared-configured-gson-instance branch from 678f4d6 to 8de4581 Compare November 22, 2025 19:03
@angelozerr
Copy link
Copy Markdown
Contributor

@angelozerr Thanks for your insights. I used the same trick as you to obtain a properly configured Gson instance.

You are welcome!

@FlorianKroiss FlorianKroiss marked this pull request as ready for review November 22, 2025 19:58
*/
public class JsonUtil {

private static final Gson LSP4J_GSON = Objects.requireNonNull(new MessageJsonHandler(Map.of()).getGson());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it not be simpler to declare the static variable public and remove the getter?

Copy link
Copy Markdown
Contributor

@rubenporras rubenporras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@FlorianKroiss FlorianKroiss force-pushed the shared-configured-gson-instance branch from 8de4581 to 837e71f Compare November 24, 2025 07:36
@FlorianKroiss FlorianKroiss force-pushed the shared-configured-gson-instance branch from 837e71f to 4512943 Compare November 24, 2025 09:15
@rubenporras rubenporras merged commit 03e08c6 into eclipse-lsp4e:main Nov 24, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Registration for workspace/didChangeWatchedFiles fails to deserialize JSON payload

3 participants