CHE-8557: No Dto available for FormattingOptions #8784

jonahgraham · 2018-02-14T20:19:25Z

The Dto isn't needed for FormattingOptions as it is really a specialized
Map and the types that contain a FormattingOptions field handle
the field as a Map during JSON serialize/deserialize

Signed-off-by: Jonah Graham jonah@kichwacoders.com

What does this PR do?

Fixes formatting when using Language server

What issues does this PR fix or reference?

#8557

Release Notes

Docs PR

jonahgraham · 2018-02-14T20:20:18Z

@svor Can you review/test please?

codenvy-ci · 2018-02-14T20:21:02Z

Can one of the admins verify this patch?

codenvy-ci · 2018-02-14T20:24:01Z

Can one of the admins verify this patch?

codenvy-ci · 2018-02-14T20:24:05Z

Can one of the admins verify this patch?

svor · 2018-02-15T09:26:12Z

@jonahkichwacoders
Thank you for quick response! These changes fix the first problem which you describe here.
But after that I got the second problem :-)

jonahgraham · 2018-02-15T11:36:15Z

@svor Can you provide me some instructions to test? I don't seem to have any failing tests at the moment and I can't get the error message you are getting. Which language server is it? Thanks!

dkulieshov · 2018-02-15T13:03:59Z

Hi,

@jonahkichwacoders Could you please dispel my concerns about something that I see in this line: https://github.com/eclipse/che/blob/master/ide/che-core-orion-editor/src/main/java/org/eclipse/che/ide/editor/orion/client/OrionEditorBuilder.java#L36

If I understand correctly, we use org.eclipse.che.ide.api.editor.editorconfig.DefaultTextEditorConfiguration for the editor while your changes impact org.eclipse.che.plugin.languageserver.ide.editor.LanguageServerFormatter which is used in cases where we initialize org.eclipse.che.plugin.languageserver.ide.editor.LanguageServerEditorConfiguration. If that's true I guess your changes and the code related to formatting that we're dealing here with is never used. How can you be sure that it works well, do we have tests for that or something that can prove it's consistency?

@svor please correct me if my concerns are in vain.

Thanks.

svor · 2018-02-15T13:58:27Z

@jonahkichwacoders
The json which is sent from the client to the server is like

{
  "options": {
    "insertSpaces": true,
    "tabSize": 4
  },
  "textDocument": {
    "uri": "/console-java-simple/src/main/java/org/eclipse/che/examples/A.java"
  }
}

I've created a test to see the error. This is the patch:

Index: wsagent/che-core-api-languageserver/src/test/java/org/eclipse/che/api/languageserver/dto/DtoConversionTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- wsagent/che-core-api-languageserver/src/test/java/org/eclipse/che/api/languageserver/dto/DtoConversionTest.java	(revision 2b9c7413a3e8555b627399b547a5bb1df02fecbf)
+++ wsagent/che-core-api-languageserver/src/test/java/org/eclipse/che/api/languageserver/dto/DtoConversionTest.java	(revision )
@@ -10,6 +10,8 @@
  */
 package org.eclipse.che.api.languageserver.dto;
 
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
 import java.lang.reflect.Array;
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
@@ -25,7 +27,9 @@
 import org.eclipse.che.api.languageserver.server.dto.DtoServerImpls.WorkspaceEditDto;
 import org.eclipse.che.api.languageserver.shared.model.ExtendedCompletionItem;
 import org.eclipse.che.api.languageserver.shared.model.ExtendedCompletionList;
+import org.eclipse.che.dto.server.DtoFactory;
 import org.eclipse.lsp4j.CompletionItem;
+import org.eclipse.lsp4j.DocumentFormattingParams;
 import org.eclipse.lsp4j.Hover;
 import org.eclipse.lsp4j.MarkedString;
 import org.eclipse.lsp4j.ParameterInformation;
@@ -83,6 +87,20 @@
     Assert.assertTrue(reflectionEquals(originalDto, convertedDto));
   }
 
+  @Test
+  public void testDocumentFormattingParamsDeserializer() throws Exception {
+    JsonObject json =
+        (JsonObject)
+            new JsonParser()
+                .parse(
+                    "{\"options\": {\"insertSpaces\": true,\"tabSize\": 4},\"textDocument\": {\"uri\": \"/console-java-simple/src/main/java/org/eclipse/che/examples/A.java\"}}");
+
+    DocumentFormattingParams params =
+        DtoFactory.getInstance().createDtoFromJson(json.toString(), DocumentFormattingParams.class);
+
+    Assert.assertTrue(params.getOptions().isInsertSpaces());
+  }
+
   @Test
   public void testEitherConversion() {
     Either<String, MarkedString> either1 = Either.forLeft("foobar");

jonahgraham · 2018-02-15T14:34:44Z

@dkuleshov my original patch, #8318 was to support using LSP4J 0.4.0 so that Debug Sever Protocol could be supported, part of #5508. I can't comment/don't know about the Orion editor.

@svor Let me look into it.

dkulieshov · 2018-02-15T15:17:53Z

@jonahkichwacoders I see, thank you for the quick response.

By a mistake (and I am ashamed for that) I must have missed that for #8318 - I see that there is a bunch of a new functionality, but I don't see the way we can appropriately test it. That is why I'm eager to know how we can ensure consistency and quality here. Could you please comment if you have any unit or integration tests for that purpose, or perhaps you can advise another way how we can make sure that solutions presented in actual pull request and in #8318 work well?

jonahgraham · 2018-02-15T16:01:41Z

@dkuleshov I too am sorry. I didn't realize this change was not covered by existing tests. The Debug's need for newer LSP4J did not really need other parts of Che updated, but with a single classloader there was no option. (I have been spoiled by years of Eclipse IDE development where independent changes can be truly independent thanks to OSGi.)

So, how to test? I don't know, I guess we are talking about retrofitting tests at the moment.

jonahgraham · 2018-02-15T16:27:07Z

@svor A few things all going wrong together here.

Dto streaming #3103 changed deserializing to stop using the Dto generator and use Gson to deserialize. Problem is Gson and generated Dtos don't create the exact same JSON. This means that calling org.eclipse.che.dto.server.DtoFactory.createDtoFromJson(String, Class<T>) vs org.eclipse.che.dto.server.DtoFactory.createDtoFromJson(JsonElement, Class<T>) uses completely different deserializing. The former uses Gson, the latter uses the Dto's implementation (org.eclipse.che.dto.server.DtoProvider.fromJson(JsonElement)). This is ok for most everything, but stuff like lsp4j has special encoding/decoding for handling fields of Either type

That means this test works (but see problem 2):

  @Test
  public void testHover() throws Exception {
    Hover hover = DtoFactory.getInstance().createDto(Hover.class);
    MarkedString markedString = DtoFactory.getInstance().createDto(MarkedString.class);
    markedString.setLanguage("Language");
    markedString.setValue("Value");
    List<Either<String, MarkedString>> list = new ArrayList<>();
    list.add(Either.forRight(markedString));
    list.add(Either.forLeft("normal String"));
    hover.setContents(list);

    JsonElement json2 = DtoFactory.getInstance().toJsonElement(hover);

//    Hover params =
//        DtoFactory.getInstance().createDtoFromJson(json2.toString(), Hover.class);
    Hover params =
        DtoFactory.getInstance().createDtoFromJson(json2, Hover.class);

    Assert.assertTrue(params.getContents().get(0).isRight());
    Assert.assertEquals("Value", params.getContents().get(0).getRight().getValue());
    Assert.assertEquals("Language", params.getContents().get(0).getRight().getLanguage());
    Assert.assertTrue(params.getContents().get(1).isLeft());
    Assert.assertEquals("normal String", params.getContents().get(1).getLeft());
  }

but this one does not:

  @Test
  public void testHover() throws Exception {
    Hover hover = DtoFactory.getInstance().createDto(Hover.class);
    MarkedString markedString = DtoFactory.getInstance().createDto(MarkedString.class);
    markedString.setLanguage("Language");
    markedString.setValue("Value");
    List<Either<String, MarkedString>> list = new ArrayList<>();
    list.add(Either.forRight(markedString));
    list.add(Either.forLeft("normal String"));
    hover.setContents(list);

    JsonElement json2 = DtoFactory.getInstance().toJsonElement(hover);

    Hover params =
        DtoFactory.getInstance().createDtoFromJson(json2.toString(), Hover.class);
//    Hover params =
//        DtoFactory.getInstance().createDtoFromJson(json2, Hover.class);

    Assert.assertTrue(params.getContents().get(0).isRight());
    Assert.assertEquals("Value", params.getContents().get(0).getRight().getValue());
    Assert.assertEquals("Language", params.getContents().get(0).getRight().getLanguage());
    Assert.assertTrue(params.getContents().get(1).isLeft());
    Assert.assertEquals("normal String", params.getContents().get(1).getLeft());
  }

The fix for this is to configure the Gson object to understand LSP4J serializing/deserializing (by registering the appropriate type adapters, see org.eclipse.lsp4j.jsonrpc.json.MessageJsonHandler.getDefaultGsonBuilder()) or by ensuring the dto generation creates JSON in the format Gson is configured to see.

The org.eclipse.che.api.languageserver.util.EitherUtil.matches(JsonElement, JsonDecision[]) is broken by Update lsp4J lib to newer version. #5292. Some code cleanup was done and it now returns true instead of false when JsonDecision is string but the element is actually an object. There are two copies of this file in the code base, one has the mistake, the other is in the state prior to Update lsp4J lib to newer version. #5292 so does not.

$ find . -name EitherUtil.java | grep org/eclipse/che/api/languageserver/util/EitherUtil.java
./wsagent/che-core-api-languageserver/src/main/java/org/eclipse/che/api/languageserver/util/EitherUtil.java
./plugins/plugin-languageserver/che-plugin-languageserver-ide/src/main/java/org/eclipse/che/api/languageserver/util/EitherUtil.java

The third problem is that we generate some mistaken code cause a class cast exception when passing the json element rather than that json string to DtoFactory.createDtoFromJson. This was caused by CHE-8304: Support DTO generating for new LSP4J constructs #8318.

For problem 1 I am not sure of the general solution, but in this one use case I can provide a fix. For 2 and 3 I'll provide an update soon.

jonahgraham · 2018-02-15T16:29:40Z

For problem 1 I am not sure of the general solution, but in this one use case I can provide a fix. For 2 and 3 I'll provide an update soon.

Actually take that back. I can provide fix for 2 and 3.

The other alternative is to roll back #8318 as that exposed these problems. Once you need the extra functionality you can reapply and fix the inconsistent json handling around lsp4j.

jonahgraham · 2018-02-15T22:39:48Z

Please don't push this! The last commit will cause compiler errors (as opposed to runtime exceptions).

@svor I am not around tomorrow (Friday 16/Feb) so I won't be able to look at this again for a few days. Feel free to revert #8318 (and eclipse-che/che-dependencies#93) until a correct fix for upgrading LSP4J can be obtained.

dkulieshov · 2018-02-16T12:12:25Z

@jonahkichwacoders Understood, thanks. I guess we can't do a lot for current problem, but I thing we eventually will have to develop more convenient and transparent testing workflows to avoid unnecessary problems in the future. However that is obviously out of the scope of this pull request.

tolusha · 2018-02-27T09:10:16Z

@jonahkichwacoders Any news

The Dto isn't needed for FormattingOptions as it is really a specialized Map and the types that contain a FormattingOptions field handle the field as a Map during JSON serialize/deserialize Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

The is necessary to ensure handling types which are of type Map are still instantiated in their real type. Consider DocumentFormattingParams.setOptions() which takes a FormattingOptions class. Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

This is effectively a follow up CHE-3103 which uses Gson directly to serialize/deserialize Json. To support LSP4J's Either types, the either type adapter factory is needed. Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

jonahgraham · 2018-02-27T10:33:54Z

@tolusha Thanks for the ping.

Rebased and pushed my set of fixes. This does not fix all LSP4J dto issues, but fixes all the formatting ones. The issues with hover that was there is still there. I recommend reviewing the use of casts in the dto generated code as the casts are converting compile time errors to runtime ones still. But AFAICT those parts of LSP are not yet in use (e.g. org.eclipse.lsp4j.TextDocumentRegistrationOptions)

tolusha · 2018-02-27T12:41:12Z

@jonahkichwacoders
Thank you, it works

@dkuleshov
@skabashnyuk
Review that patch pls

codenvy-ci · 2018-02-28T08:46:02Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4485/

skabashnyuk · 2018-02-28T11:40:30Z

@tolusha @svor @vparfonov as discussed I can propose to do something like this

@@ -101,12 +102,11 @@ public final class DtoFactory {
   // It helps avoid reflection when need create copy of exited DTO instance.
   private final Map<Class<?>, DtoProvider<?>> dtoImpl2Providers = new ConcurrentHashMap<>();
   private final Gson dtoGson =
-      new GsonBuilder()
-          .registerTypeAdapterFactory(
-              new NullAsEmptyTAF<>(Collection.class, Collections.emptyList()))
-          .registerTypeAdapterFactory(new NullAsEmptyTAF<>(Map.class, Collections.emptyMap()))
-          .registerTypeAdapterFactory(new DtoInterfaceTAF())
-          .create();
+      buildDtoParser(
+          ServiceLoader.load(TypeAdapterFactory.class).iterator(),
+          new NullAsEmptyTAF<>(Collection.class, Collections.emptyList()),
+          new NullAsEmptyTAF<>(Map.class, Collections.emptyMap()),
+          new DtoInterfaceTAF());

   /**
    * Created deep copy of DTO object.
@@ -490,4 +490,19 @@ public final class DtoFactory {
   }

   private DtoFactory() {}
+
+  private static Gson buildDtoParser(
+      Iterator<TypeAdapterFactory> factoryIterator, TypeAdapterFactory... factories) {
+    GsonBuilder builder = new GsonBuilder();
+
+    for (Iterator<TypeAdapterFactory> it = factoryIterator; it.hasNext(); ) {
+      TypeAdapterFactory factory = it.next();
+      builder.registerTypeAdapterFactory(factory);
+    }
+
+    for (TypeAdapterFactory factory : factories) {
+      builder.registerTypeAdapterFactory(factory);
+    }
+    return builder.create();
+  }
 }

wdyt?
In this way we can avoid adding lsp specific dependency to core

tolusha · 2018-02-28T12:15:58Z

+1

svor · 2018-02-28T14:20:21Z

i agree

svor · 2018-03-01T13:24:15Z

@skabashnyuk Are you going to merge your changes to master branch?

skabashnyuk · 2018-03-01T13:25:53Z

No. I hope authors of this pr will test it. it was just a prototype.

svor · 2018-03-01T13:44:48Z

@jonahkichwacoders
Could you please try this way of registering TypeAdapterFactory to avoid adding org.eclipse.lsp4j.jsonrpc dependency?

jonahgraham · 2018-03-05T08:59:16Z

@svor Sure, I can have a look.

jonahgraham · 2018-03-07T20:50:24Z

@svor I have been very busy this week, I will try to get to this in the next few days. Feel free to apply the proposed solution to the branch if this is more urgent than I can get to it.

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

svor · 2018-03-13T10:46:56Z

Hello @jonahkichwacoders
I've made a PR to your branch jonahgraham#1
Please take a look.

Load EitherTypeAdapterFactory for registration

jonahgraham · 2018-03-14T15:41:52Z

Hi @svor, It looks fine to me, I won't have time to run it up today though.

svor · 2018-03-14T15:52:14Z

Thanks @jonahkichwacoders
@skabashnyuk Can you take another look at this PR
@dkuleshov @vparfonov @evidolob Please review these changes

tolusha · 2018-03-15T08:09:45Z

ci-test

tsmaeder

lgtm

codenvy-ci · 2018-03-15T14:24:39Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8784
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

svor · 2018-03-15T16:03:38Z

@musienko-maxim Can you help to analyse test report please?

musienko-maxim · 2018-03-16T10:02:31Z

The selenium session did not find new bugs

svor · 2018-03-16T10:08:42Z

@jonahkichwacoders so we are ready to merge. Maybe you want to add something?

jonahgraham · 2018-03-16T12:30:17Z

I'm good. Thanks.

jonahgraham requested review from dkulieshov and evidolob as code owners February 14, 2018 20:19

svor mentioned this pull request Feb 19, 2018

Replace old java formatter to jdt.ls formatter #8829

Merged

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Feb 23, 2018

jonahgraham added 5 commits February 27, 2018 09:12

CHE-8557: Test cases for Either deserializing

4aaad10

Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

CHE-8557: Fix Either handling when deserializing on the server

e03ac57

Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

CHE-8557: Register LSP4J's Either adapter factory

007a354

This is effectively a follow up CHE-3103 which uses Gson directly to serialize/deserialize Json. To support LSP4J's Either types, the either type adapter factory is needed. Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

jonahgraham force-pushed the CHE-8557 branch from 01d6006 to 007a354 Compare February 27, 2018 10:22

jonahgraham requested a review from skabashnyuk as a code owner February 27, 2018 10:22

load EitherTypeAdapterFactory for registration

32a9741

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

Merge pull request #1 from svor/che-8557

3a924ce

Load EitherTypeAdapterFactory for registration

svor requested a review from vparfonov March 14, 2018 15:49

svor approved these changes Mar 14, 2018

View reviewed changes

skabashnyuk approved these changes Mar 15, 2018

View reviewed changes

vparfonov approved these changes Mar 15, 2018

View reviewed changes

evidolob approved these changes Mar 15, 2018

View reviewed changes

dkulieshov approved these changes Mar 15, 2018

View reviewed changes

tsmaeder approved these changes Mar 15, 2018

View reviewed changes

svor merged commit c69cf51 into eclipse-che:master Mar 16, 2018

benoitf added this to the 6.3.0 milestone Mar 16, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 16, 2018

CHE-8557: No Dto available for FormattingOptions #8784

CHE-8557: No Dto available for FormattingOptions #8784

Conversation

jonahgraham commented Feb 14, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

jonahgraham commented Feb 14, 2018

codenvy-ci commented Feb 14, 2018

codenvy-ci commented Feb 14, 2018

codenvy-ci commented Feb 14, 2018

svor commented Feb 15, 2018

jonahgraham commented Feb 15, 2018

dkulieshov commented Feb 15, 2018

svor commented Feb 15, 2018 • edited Loading

jonahgraham commented Feb 15, 2018

dkulieshov commented Feb 15, 2018

jonahgraham commented Feb 15, 2018

jonahgraham commented Feb 15, 2018

jonahgraham commented Feb 15, 2018

jonahgraham commented Feb 15, 2018

dkulieshov commented Feb 16, 2018 • edited Loading

tolusha commented Feb 27, 2018

jonahgraham commented Feb 27, 2018

tolusha commented Feb 27, 2018

codenvy-ci commented Feb 28, 2018

skabashnyuk commented Feb 28, 2018

tolusha commented Feb 28, 2018

svor commented Feb 28, 2018 • edited Loading

svor commented Mar 1, 2018

skabashnyuk commented Mar 1, 2018

svor commented Mar 1, 2018

jonahgraham commented Mar 5, 2018

jonahgraham commented Mar 7, 2018

svor commented Mar 13, 2018

jonahgraham commented Mar 14, 2018

svor commented Mar 14, 2018 • edited Loading

tolusha commented Mar 15, 2018

tsmaeder left a comment

Choose a reason for hiding this comment

codenvy-ci commented Mar 15, 2018

svor commented Mar 15, 2018

musienko-maxim commented Mar 16, 2018

svor commented Mar 16, 2018 • edited Loading

jonahgraham commented Mar 16, 2018

svor commented Feb 15, 2018 •

edited

Loading

dkulieshov commented Feb 16, 2018 •

edited

Loading

svor commented Feb 28, 2018 •

edited

Loading

svor commented Mar 14, 2018 •

edited

Loading

svor commented Mar 16, 2018 •

edited

Loading