MessageJsonHandler::toString doesn't work with custom type adapters #768
Comments
|
As a workaround, would it be manageable for you to specify the type adapter factory for the appropriate fields of your RPC messages using the |
|
That's what I tried until I reached fields of type |
|
It is more of a hassle, but I think it should be possible. You'll just need to implement special type adapters for those cases. There are examples in LSP4J itself, such as |
|
Thanks, that sound an interesting workaround for any collection. By chance, would you know how to implement the same for a Map? I am not sure how to get access to the built-in Map adapter from gson, and "configure" it in a way to use my custom Path adapter for the value. I am looking for something like: // On the DTO
@JsonAdapter(MapOfPathAdapter.class)
private Map<String, Path> mapOfPaths;
public class MapOfPathAdapter implements TypeAdapterFactory {
private static final TypeToken<Path> ELEMENT_TYPE
= new TypeToken<Path>() {};
@SuppressWarnings("unchecked")
@Override
public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
TypeAdapter<Path> valueTypeAdapter = new PathAdapter<>(gson, ELEMENT_TYPE);
return (TypeAdapter<T>) new MapTypeAdapter<>(gson, ELEMENT_TYPE.getType(), valueTypeAdapter, HashMap::new); // How to get the default Map type adapter but "force" it to handle values with a different type adapter?
}
} |
|
I guess that basically you would need to write your own |
|
I would like to avoid that :) When I see the built-in Map adapter from gson, it seems pretty big. |
|
I'm afraid I don't know any other way. Looks like much of the complexity of the gson Map adapter stems from the need to handle complex keys. If keys are strings, the implementation would be much simpler: just read/write a JSON object. |
In the first instance I think a toString that can throw an exception is bad practice and we should not be allowing that exception out of toString. I hope no one is depending on such behaviour.
It looks like it was either an optimization to use a generic gson, or perhaps a desire to see the world without custom adapters affecting things while tracing and debugging? My first instinct would be to consider a patch that uses the correct gson instance. But perhaps the reason it is currently written this way is that the "correct" gson instance isn't available to the tracing code, so it may be more than a trivial change? |
This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768
This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768
This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768
This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768
|
@jonahgraham It is more than a trivial change, indeed. It seems that in #772 we have been able to find a way to pass the "correct" gson instance to the tracing code without breaking changes. However, the static One way to deal with this problem might be to replace the gson-based implementation of these However, this might be a breaking change, since some of the existing clients might have come to depend on seeing JSON-based representation of messages in logs. ( Therefore, I think it would be better to use an alternative @Override
public String toString() {
try {
return MessageJsonHandler.toString(this);
} catch (JsonIOException e) {
return toStringFallback();
}
}
private String toStringFallback() {
ToStringBuilder builder = new ToStringBuilder(this);
builder.addAllFields();
return builder.toString();
}This implementation might be enhanced further, if we provide a way to inject the "correct" gson instance to a transient field of the private transient MessageJsonHandler jsonHandler;
public setJsonHandler(MessageJsonHandler jsonHandler) {
this.jsonHandler = jsonHandler;
}
@Override
public String toString() {
try {
return jsonHandler != null ? jsonHandler.format(this) : MessageJsonHandler.toString(this);
} catch (JsonIOException e) {
return toStringFallback();
}
}However, I'm not quite sure about it. WDYT? |
I don't think we should consider the representation of messages in the logs to be part of the contract of LSP4J - but I do think that if possible we should keep the string representation json. The toStringFallback seems like a good fallback solution though, even if it isn't in json. (By definition no one today that would get toStringFallback can be depending on current output since current output causes exception for those consumers) I do like the idea of the full handler being stored in a transient property. On a quick inspection it looks like the places messages are created do have access to the write instance, so it looks possible. |
|
Thank you for your comments, Jonah! 👍 It looks like we are on the same page here, which is encouraging. |
This is important when users have registered custom type adapters Fixes eclipse-lsp4j#768
This is important when users have registered custom type adapters Fixes #768 Co-authored-by: Vladimir Piskarev <[email protected]>
Hi,
I am trying to use types like
java.nio.file.Pathdirectly in my RPC messages. For this, we are using a custom type adapter, and registering it when building the launcher:the problem is with some code in lsp4j that will try to serialize/deserialize messages with a separate instance of gson, that has not been configured with the custom type adapters.
It leads to errors like:
Any use of
MessageJsonHandler.toStringis affected.I wonder if there could be a way to either reuse the same configured gson instance everywhere, or maybe allow to also configure the one used for
toString?The text was updated successfully, but these errors were encountered: