Bug #130: Support for JSON-RPC-like Debug Server Protocol's RPC #131

jonahgraham · 2017-11-13T18:27:20Z

This provides the supporting RPC implementation for VSCode's Debug Protocol
as defined https://github.com/Microsoft/vscode-debugadapter-node/blob/master/debugProtocol.json

See the class comment on DebugMessageTypeAdapterFactory for details on
mapping between Debug Server Protocol and JSON-RPC 2.0.

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

eclipsewebmaster · 2017-11-13T18:27:24Z

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=130

mickaelistria · 2017-11-16T10:34:47Z

@svenefftinge @spoenemann @akosyakov Is there anything I can do to help on this topic? I'm eager to see progress there and can allocate most of my time.

svenefftinge · 2017-11-17T09:50:37Z

@jonahkichwacoders thanks for the PR.
Could we vote you in as a committer and maintainer of this part?

spoenemann

Thank you for this proposal! The code seems to me of very high quality. I would support your nomination as a committer :-)

Are you planning to also implement service interfaces with corresponding data structures in the style of org.eclipse.lsp4j.services.LanguageServer? Or have you already done that?

spoenemann · 2017-11-17T10:25:12Z

ide/build.gradle

@@ -30,6 +38,7 @@ oomphIde {
 	addProjectFolder('../org.eclipse.lsp4j')
 	addProjectFolder('../org.eclipse.lsp4j.generator')
 	addProjectFolder('../org.eclipse.lsp4j.jsonrpc')
+	addProjectFolder('../org.eclipse.dsp4j.jsonrpc')


I'm not sure about the namespace org.eclipse.dsp4j, since theoretically that could cause name clashes with other Eclipse projects. On the other hand, I like the symmetry with org.eclipse.lsp4j. What do others think about this?

An alternative project name could be org.eclipse.lsp4j.jsonrpc.debug

OK, depending on decision here will affect name of the interfaces containing plug-in. I was originally planning on org.eclipse.dsp4j, but perhaps org.eclipse.lsp4j.debug makes more sense.

Yes, IIRC there even is some rule at eclipse for java packages to use the project name as prefix.

spoenemann · 2017-11-17T10:29:47Z

...jsonrpc/src/main/java/org/eclipse/dsp4j/jsonrpc/adapters/DebugMessageTypeAdapterFactory.java

+			return result;
+		}
+
+		protected Object parseParams(JsonReader in, String method) throws IOException {


We should make MessageTypeAdapterFactory extensible in order to avoid duplicating code here. I'll prepare a PR for that.

OK, there is certainly some duplication between the two.

Merged #135.

I updated the debug code to to use this new code. I also slightly refactored the MessageTypeAdapterFactory to access some of its logic from the subclass.

spoenemann · 2017-11-17T10:33:10Z

...ipse.dsp4j.jsonrpc/src/main/java/org/eclipse/dsp4j/jsonrpc/json/DebugMessageJsonHandler.java

+		super(supportedMethods, configureGson);
+	}
+
+	public GsonBuilder getDefaultGsonBuilder() {


This additional class could be avoided by passing a Consumer<GsonBuilder> to the default MessageJsonHandler. When multiple type adapter factories are configured for the same type, the last one should win.

OK, the advantage of your approach too is that the other type factories don't need to be duplicated. A lot of the tests instantiate DebugMessageJsonHandler directly, so I'll probably want a method somewhere that constructs a MessageJsonHandler correctly for the tests in the same way as it is constructed for by the DebugLauncher. Perhaps a method in the DebugLauncher itself.

I am having trouble writing a sensible method in DebugLauncher, at the moment it looks like the sample below, which seems uglier than having the extra class, ideas welcome.

The reason for the complication is the circular dependency between the MessageJsonHandler and the DebugMessageTypeAdapterFactory.

My next update will have the DebugMessageJsonHandler still, but rather than repeating everything in getDefaultGsonBuilder I'll call super's one and then add on the DebugMessageTypeAdapterFactory.

If you have a thought on something I overlooked, I would be pleased for the feedback.

static MessageJsonHandler createMessageHandler(Map<String, JsonRpcMethod> supportedMethods, Consumer<GsonBuilder> configureGson) { DebugMessageTypeAdapterFactory adapterFactory[] = new DebugMessageTypeAdapterFactory[] {null}; Consumer<GsonBuilder> consumer = (builder) -> { builder.registerTypeAdapterFactory(adapterFactory[0]); configureGson.accept(builder); }; MessageJsonHandler messageJsonHandler = new MessageJsonHandler(supportedMethods, consumer); adapterFactory[0] = new DebugMessageTypeAdapterFactory(messageJsonHandler); return messageJsonHandler; }

Hmmm, we could change the configureGson parameter in MessageJsonHandler to BiConsumer<GsonBuilder, MessageJsonHandler>, but that's a breaking change. @akosyakov, @svenefftinge do you think this is worth it? Would help for cases when someone wants to customize parsing / serialization of messages.

We could also add an additional constructor and deprecate the existing one. That would prevent it being a breaking change.

Right, we don't even need to deprecate the old one.

I have not made this change yet. The current version, and the new constructor we discussed suffers from a problem. It passes a reference to this from the constructor when the object is not fully created, so creating new API that required and actually enforced passing around an incompletely constructed object seems like a bad idea. If you would like me to pursue this further (e.g. add an initialize() method for example) I can raise and issue and work on that separately?

spoenemann · 2017-11-17T10:36:25Z

...j.jsonrpc/src/test/java/org/eclipse/dsp4j/jsonrpc/test/json/DebugMessageJsonHandlerTest.java

+	}
+
+	@Test
+	public void testParamsParsing_AllOrders() {


This parsing test that tries all iterations would also be nice for the MessageJsonHandlerTest in org.eclipse.lsp4j.jsonrpc.

OK, I will contribute it back to both places. I also realised on revisiting that the idea was to remove the _02/_03/etc versions of the methods, so I will do that too.

I have added the _AllOrders method to MessageJsonHandlerTest and extended it to do all the types of messages.

spoenemann · 2017-11-17T11:00:38Z

@eclipsewebmaster

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=130

That makes no sense. Has probably been triggered by Jonahs mention of Bug #130. So better use the term "issue" in this context :-D

jonahgraham · 2017-11-17T12:52:16Z

Could we vote you in as a committer and maintainer of this part?

Yes.

jonahgraham · 2017-11-17T12:55:29Z

Thank you for this proposal! The code seems to me of very high quality. I would support your nomination as a committer :-)

Thanks.

Are you planning to also implement service interfaces with corresponding data structures in the style of org.eclipse.lsp4j.services.LanguageServer? Or have you already done that?

I am planning to create a PR in the coming days with the equivalent for the debug protocol. It is generated from the debug protocol json schema. Originally I was going to put it in a plug-in called org.eclipse.dsp4j, but based on your other comments, perhaps org.eclipse.lsp4j.debug would make more sense.

jonahgraham · 2017-11-17T13:04:02Z

So better use the term "issue" in this context :-D

OK.

spoenemann · 2017-11-17T14:48:59Z

It seems like the word "bug" is not necessary for triggering the Bugzilla bot, see eclipse/kapua#131

…ng of JSON The DSP implementation created some new tests for checking every order of fields in a JSON message. This commit adapts the same test for LSP messages. Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

jonahgraham · 2017-11-23T16:13:44Z

@spoenemann I have updated the code with all your feedback and it is again ready for review.

This update has the next part too, the service protocol files. As the DSP has a Json Schema that defines it, the DebugProtocol.java, IDebugProtocolClient and IDebugProtocolServer are auto-generated using a generator derived from the typescript generator used by vscode. Please see the instructions for the generator in the root of the org.eclipse.lsp4j.debug plug-in. I don't know if the licensing of the generator allows it to live in the lsp4j code base. I also don't know if continuing to generate it automatically as the protocol evolves is the best plan either, so perhaps the generator is only useful to bootstrap the project?

spoenemann

Excellent!

As long as the main source for the DSP is a JSON schema, it totally makes sense to generate the Java code from it. Ideally, the generator would be in this project (org.eclipse.lsp4j.generator), but probably we cannot just copy the code from Microsoft/vscode-debugadapter-node or a derived version of it. We can leave that problem for later and first make this PR ready for merge.

spoenemann · 2017-11-24T08:49:16Z

org.eclipse.lsp4j.debug/README.md

+```
+$ git clone git@github.com:jonahkichwacoders/vscode-debugadapter-node.git
+$ git checkout dsp
+$ npm install -g typescript


typescript is already in the devDependencies of the package.json, so it's not necessary to install it globally. The line tsc -p ./src/ # convert typescript should be added as a script in the package.json

spoenemann · 2017-11-24T08:54:57Z

org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.java

+	/**
+	 * <p>
+	 * Version of debugProtocol.json this class was derived from.
+	 * </p>


All these <p>...</p> create quite a lot of noise. I would prefer to omit them.

I can add some additional logic to the generator to only add the <p> between paragraphs and omit them on single paragraph comments.

spoenemann · 2017-11-24T09:00:52Z

org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.java

+	 * completed, by executing a debugger statement etc.
+	 * </p>
+	 */
+	public static class StoppedEventArguments {


The class structure should not be nested, but flat, in order to be consistent with the LSP implementation.

Probably it would be better to generate a DebugProtocol.xtend with @JsonRpcData tags and that gets us even closer. It would be a 2 stage generation process (first the typescript one then the o.e.lsp4j.generator one), but then the DebugProtocol.xtend would be much readable (and shorter). WDYT?

Two-stage generation sounds a bit scary at first. But a nice side effect is that all the derived members (getters, setters, equals, hashCode) are consistent in both protocols. And I like the idea to have a more readable version of the Java data structures.

spoenemann · 2017-11-24T09:03:59Z

org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.java

+		 * Possible values include - but not limited to those defined in {@link Reason}
+		 * </p>
+		 */
+		public String reason;


The fields should not be public, but accessible via getters, in order to be consistent with the LSP implementation.

spoenemann · 2017-11-24T09:21:16Z

...eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/services/IDebugProtocolClient.java

+	 * </ul>
+	 */
+	@JsonNotification
+	default void initialized() {


Why these default implementations? I'd remove them.

These are warnings always seen when running gradle build Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

This provides the supporting RPC implementation for VSCode's Debug Protocol as defined https://github.com/Microsoft/vscode-debugadapter-node/blob/master/debugProtocol.json See the class comment on DebugMessageTypeAdapter for details on mapping between Debug Server Protocol and JSON-RPC 2.0. See DebugProtocol/IDebugProtocolClient/IDebugProtocolServer for the Java Service Interfaces and supporting types. Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

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

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

jonahgraham · 2017-11-28T13:20:12Z

@spoenemann Its ready, I have hopefully addressed all your concerns adequately. With the generation of DebugProtocol.xtend and using the existing generator as the second-pass generator makes it much easier to read DebugProtocol.xtend and the generated javadoc still looks fine.

spoenemann

Looks good to me, thanks again!

A further improvement would be to generate @NonNull annotations from the required properties in the schema, but I'll create a separate issue for that.

spoenemann · 2017-11-29T08:19:51Z

org.eclipse.lsp4j.debug/src/main/java/org/eclipse/lsp4j/debug/DebugProtocol.xtend

+ * <p>
+ * Auto-generated from debugProtocol.json schema version 1.24.0. Do not edit manually.
+ */
+class DebugProtcol {


I think this class could be removed now.

is deprecated and going to be removed soon. Fixes eclipse-lsp4j#131 Signed-off-by: azerr <azerr@redhat.com>

jonahgraham mentioned this pull request Nov 13, 2017

Support Debug Server Protocol #130

Closed

spoenemann reviewed Nov 17, 2017

View reviewed changes

spoenemann mentioned this pull request Nov 20, 2017

Made Gson type adapters better extensible #135

Merged

jonahgraham force-pushed the bug130_dsp branch 2 times, most recently from 3c90c26 to e374cf0 Compare November 23, 2017 14:31

jonahgraham mentioned this pull request Nov 23, 2017

No response message/wrong response message if RemoteEndpoint's exceptionHandler returns null #136

Closed

spoenemann reviewed Nov 24, 2017

View reviewed changes

jonahgraham added 4 commits November 28, 2017 13:05

Fix minor Javadoc warnings

ddbdb25

These are warnings always seen when running gradle build Signed-off-by: Jonah Graham <jonah@kichwacoders.com>

Issue eclipse-lsp4j#136: Test demonstrating misbehaving Exception Han…

5615657

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

Issue eclipse-lsp4j#136: Fix for missing response error object

33fe5de

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

jonahgraham force-pushed the bug130_dsp branch from 7670fce to 33fe5de Compare November 28, 2017 13:06

spoenemann approved these changes Nov 29, 2017

View reviewed changes

spoenemann merged commit 0cacee9 into eclipse-lsp4j:master Nov 29, 2017

adietish pushed a commit to adietish/lsp4j that referenced this pull request Aug 20, 2024

fix: com.intellij.diagnostic.PluginException: ActionUpdateThread.OLD_EDT

03fab78

is deprecated and going to be removed soon. Fixes eclipse-lsp4j#131 Signed-off-by: azerr <azerr@redhat.com>

Bug #130: Support for JSON-RPC-like Debug Server Protocol's RPC #131

Bug #130: Support for JSON-RPC-like Debug Server Protocol's RPC #131

Conversation

jonahgraham commented Nov 13, 2017

eclipsewebmaster commented Nov 13, 2017

mickaelistria commented Nov 16, 2017

svenefftinge commented Nov 17, 2017

spoenemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoenemann commented Nov 17, 2017

jonahgraham commented Nov 17, 2017 • edited Loading

jonahgraham commented Nov 17, 2017

jonahgraham commented Nov 17, 2017

spoenemann commented Nov 17, 2017

jonahgraham commented Nov 23, 2017

spoenemann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Nov 28, 2017

spoenemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Nov 17, 2017 •

edited

Loading

spoenemann left a comment •

edited

Loading