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.

Sign up for GitHub

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

Jump to bottom

Add missing type adapters #669

Merged
merged 4 commits into from
Oct 6, 2022
Merged

Add missing type adapters #669

merged 4 commits into from
Oct 6, 2022

Conversation

pisv
Copy link
Contributor

@pisv pisv commented Sep 30, 2022

Adds type adapters for DocumentDiagnosticReport and WorkspaceDocumentDiagnosticReport.

Fixes #668.

@pisv
Add missing type adapters
0cc9eaa 
Adds type adapters for `DocumentDiagnosticReport` and
`WorkspaceDocumentDiagnosticReport`.

Fixes eclipse-lsp4j#668.
@pisv pisv added this to the 0.16.0 milestone Sep 30, 2022
@pisv
Make stricter tests
987fe50 
Use MessageJsonHandler to obtain a Gson instance in tests
@pisv
Use Collections.emptyMap() instead of Map.of()
c4b4a33 
Need to remember that LSP4J is still at Java 8
@pisv
Copy link
Contributor Author

pisv commented Oct 1, 2022 •
edited
Loading

Unfortunately, the fix didn't work properly when testing the snapshot build in my local setting. JsonParseException: Ambiguous Either type: token BEGIN_OBJECT matches both alternatives is still thrown when reading a response to textDocument/diagnostic or workspace/diagnostic requests.

The reason is that EitherTypeAdapter, which is registered in MessageJsonHandler.getDefaultGsonBuilder(), take precedence over DocumentDiagnosticReportTypeAdapter and WorkspaceDocumentDiagnosticReportTypeAdapter registered via @JsonAdapter annotation. As stated in the annotation contract, "Field annotations take precedence over GsonBuilder-registered type adapters, which in turn take precedence over annotated types."

I have changed the DocumentDiagnosticReportTypeAdapterTest and WorkspaceDocumentDiagnosticReportTypeAdapterTest to use the Gson instance obtained from MessageJsonHandler rather than a default Gson instance. Now they started to fail with the same error as observed in my local setting.

I'm going to try a different approach, which is based on using @ResponseJsonAdapter annotation on the corresponding request methods instead of @JsonAdapter on the corresponding types. Hopefully, this can be done soon.

@pisv pisv marked this pull request as draft October 1, 2022 14:30
@pisv pisv marked this pull request as ready for review October 1, 2022 16:18
@pisv
Copy link
Contributor Author

pisv commented Oct 1, 2022 •
edited
Loading

I have verified that the current fix works fine in my local setup. All tests are now green too. The PR is ready for review.

@pisv pisv merged commit 0a3e64a into eclipse-lsp4j:main Oct 6, 2022
@pisv pisv deleted the GH-668 branch October 6, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KamasamaK KamasamaK KamasamaK approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.

JsonParseException: Ambiguous Either type when reading response to textDocument/diagnostic
2 participants
@pisv @KamasamaK