MessageJsonHandler::toString doesn't work with custom type adapters

[henryju]

Hi,

I am trying to use types like java.nio.file.Path directly in my RPC messages. For this, we are using a custom type adapter, and registering it when building the launcher:

var launcher = new Launcher.Builder<MyClient>()
      .setLocalService(myServer)
      .setRemoteInterface(MyClient.class)
      .setInput(in)
      .setOutput(out)
      .configureGson(gsonBuilder -> gsonBuilder
        .registerTypeAdapterFactory(new PathTypeAdapter.Factory())
      )
      //.traceMessages(new PrintWriter(System.out, true))  // This is one way to trigger the issue
      .create();

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:

com.google.gson.JsonIOException: Failed making field 'sun.nio.fs.UnixPath#fs' accessible; either increase its visibility or write a custom TypeAdapter for its declaring type.
    at com.google.gson.internal.reflect.ReflectionHelper.makeAccessible(ReflectionHelper.java:38)
    at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.getBoundFields(ReflectiveTypeAdapterFactory.java:287)
    at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.create(ReflectiveTypeAdapterFactory.java:130)
    at com.google.gson.Gson.getAdapter(Gson.java:546)
    at org.eclipse.lsp4j.jsonrpc.json.adapters.CollectionTypeAdapter.write(CollectionTypeAdapter.java:138)
    at org.eclipse.lsp4j.jsonrpc.json.adapters.CollectionTypeAdapter.write(CollectionTypeAdapter.java:40)
    at com.google.gson.internal.bind.TypeAdapterRuntimeTypeWrapper.write(TypeAdapterRuntimeTypeWrapper.java:70)
    at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$1.write(ReflectiveTypeAdapterFactory.java:196)
    at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory$Adapter.write(ReflectiveTypeAdapterFactory.java:366)
    at com.google.gson.Gson.toJson(Gson.java:825)
    at com.google.gson.Gson.toJson(Gson.java:795)
    at com.google.gson.Gson.toJson(Gson.java:742)
    at com.google.gson.Gson.toJson(Gson.java:719)
    at org.eclipse.lsp4j.jsonrpc.json.MessageJsonHandler.toString(MessageJsonHandler.java:161)
    at org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer.consumeMessageSending(TracingMessageConsumer.java:125)
    at org.eclipse.lsp4j.jsonrpc.TracingMessageConsumer.consume(TracingMessageConsumer.java:101)

Any use of MessageJsonHandler.toString is 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?

[pisv]

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 @JsonAdapter annotation, instead of registering it when building the launcher?

[henryju]

That's what I tried until I reached fields of type List<Path>, or Either<Path, SomethingElse>. As far as I know, it is not possible to define the type adapter using annotation in this case.

[pisv]

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 DocumentChangeListAdapter or PrepareRenameResponseAdapter. Just search for implementations of TypeAdapterFactory in LSP4J for much more examples.

[henryju]

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?
    }
}

[pisv]

I guess that basically you would need to write your own MapTypeAdapter along the lines of CollectionTypeAdapter, but reading/writing a JSON object instead of a JSON array, would not you?

[henryju]

I would like to avoid that :) When I see the built-in Map adapter from gson, it seems pretty big.

[pisv]

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.

[jonahgraham]

Any use of MessageJsonHandler.toString is affected.

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.

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?

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?

[pisv]

@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 MessageJsonHandler.toString(Object) method is still called from Message.toString(), ResponseError.toString(), and CancelParams.toString() methods, which makes them susceptible to the same issue.

One way to deal with this problem might be to replace the gson-based implementation of these toString() methods with something more safe, e.g. an implementation based on a ToStringBuilder.

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. (Message.toString() is called from several places in LSP4J for logging.)

Therefore, I think it would be better to use an alternative toString implementation as a fallback when the existing implementation fails, e.g.:

@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 Message when messages are created, e.g.:

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?

[jonahgraham]

depend on seeing JSON-based representation of messages in logs

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.

[pisv]

Thank you for your comments, Jonah! 👍 It looks like we are on the same page here, which is encouraging.