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.

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

Provide JSON-RPC service for Language Service Communications #7840

Merged
merged 13 commits into from
Dec 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions wsagent/che-core-api-languageserver/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,22 @@
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-languageserver-shared</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-model</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-project</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-workspace</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-api-workspace-shared</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-commons-lang</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.eclipse.che.api.languageserver.registry.LanguageServerRegistryImpl;
import org.eclipse.che.api.languageserver.registry.ServerInitializer;
import org.eclipse.che.api.languageserver.registry.ServerInitializerImpl;
import org.eclipse.che.api.languageserver.remote.LsRemoteModule;
import org.eclipse.che.api.languageserver.service.LanguageRegistryService;
import org.eclipse.che.api.languageserver.service.LanguageServerInitializationHandler;
import org.eclipse.che.api.languageserver.service.TextDocumentService;
Expand All @@ -32,6 +33,8 @@ public class LanguageServerModule extends AbstractModule {

@Override
protected void configure() {
install(new LsRemoteModule());

bind(LanguageServerRegistry.class).to(LanguageServerRegistryImpl.class);
bind(ServerInitializer.class).to(ServerInitializerImpl.class);
bind(LanguageRegistryService.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,13 @@ public interface LanguageServerLauncher {

/** Indicates if language server is installed and is ready to be started. */
boolean isAbleToLaunch();

/**
* Denotes if the language server will be launched in a local environment or remote (i.e. if
* <code>isLocal</code> returns <code>true</code> than the server is launched in the local
* physical/virtual machine, otherwise means that the server is launched in the remote machine)
*/
default boolean isLocal() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@
*/
package org.eclipse.che.api.languageserver.registry;

import static javax.ws.rs.core.UriBuilder.fromUri;
import static org.eclipse.che.api.fs.server.WsPathUtils.absolutize;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -24,15 +27,22 @@
import java.util.stream.Collectors;
import javax.annotation.PreDestroy;
import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Provider;
import javax.inject.Singleton;
import org.eclipse.che.api.core.ApiException;
import org.eclipse.che.api.core.model.workspace.Workspace;
import org.eclipse.che.api.core.notification.EventService;
import org.eclipse.che.api.core.rest.HttpJsonRequestFactory;
import org.eclipse.che.api.languageserver.exception.LanguageServerException;
import org.eclipse.che.api.languageserver.launcher.LanguageServerLauncher;
import org.eclipse.che.api.languageserver.remote.RemoteLsLauncherProvider;
import org.eclipse.che.api.languageserver.service.LanguageServiceUtils;
import org.eclipse.che.api.languageserver.shared.model.LanguageDescription;
import org.eclipse.che.api.project.server.ProjectManager;
import org.eclipse.che.api.project.server.impl.RegisteredProject;
import org.eclipse.che.api.workspace.server.WorkspaceService;
import org.eclipse.che.api.workspace.shared.dto.WorkspaceDto;
import org.eclipse.lsp4j.MessageParams;
import org.eclipse.lsp4j.MessageType;
import org.eclipse.lsp4j.ServerCapabilities;
Expand All @@ -44,6 +54,11 @@
public class LanguageServerRegistryImpl implements LanguageServerRegistry {

private static final Logger LOG = LoggerFactory.getLogger(LanguageServerRegistryImpl.class);

private final String workspaceId;
private final String apiEndpoint;
private final HttpJsonRequestFactory httpJsonRequestFactory;
private final Set<RemoteLsLauncherProvider> launcherProviders;
private final List<LanguageDescription> languages;
private final List<LanguageServerLauncher> launchers;
private final AtomicInteger serverId = new AtomicInteger();
Expand All @@ -57,15 +72,24 @@ public class LanguageServerRegistryImpl implements LanguageServerRegistry {
private final ServerInitializer initializer;
private EventService eventService;
private CheLanguageClientFactory clientFactory;
private Workspace workspace;

@Inject
public LanguageServerRegistryImpl(
@Named("env.CHE_WORKSPACE_ID") String workspaceId,
@Named("che.api") String apiEndpoint,
HttpJsonRequestFactory httpJsonRequestFactory,
Set<RemoteLsLauncherProvider> launcherProviders,
Set<LanguageServerLauncher> languageServerLaunchers,
Set<LanguageDescription> languages,
Provider<ProjectManager> projectManagerProvider,
ServerInitializer initializer,
EventService eventService,
CheLanguageClientFactory clientFactory) {
this.workspaceId = workspaceId;
this.apiEndpoint = apiEndpoint;
this.httpJsonRequestFactory = httpJsonRequestFactory;
this.launcherProviders = launcherProviders;
this.languages = new ArrayList<>(languages);
this.launchers = new ArrayList<>(languageServerLaunchers);
this.projectManagerProvider = projectManagerProvider;
Expand Down Expand Up @@ -189,8 +213,16 @@ private List<LanguageServerLauncher> findLaunchers(String projectPath, String fi
if (language == null) {
return Collections.emptyList();
}
List<LanguageServerLauncher> combinedLaunchers = new LinkedList<>(launchers);
Workspace workspace = getWorkspaceConfiguration();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in this area of Che, but by looking at a method argument fileUri I suppose that this method can be called a lot of times because of different files. If this is true then looks like this code might call Workspace API a lot of times. Wouldn't it be better to cache GET WS API calls results?

if (workspace != null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on when this may happen and what the behavior of the wsagent would be in such a case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you're asking. Please take a look at this code fragment:

try {
  return httpJsonRequestFactory.fromUrl(href).useGetMethod().request().asDto(WorkspaceDto.class);
} catch (IOException | ApiException e) {
  return null;
}

I must admit that at first glance it is not obvious where null comes from, but if you could look a bit deeper you could see that this line

httpJsonRequestFactory.fromUrl(href).useGetMethod().request().asDto(WorkspaceDto.class);

could throw exceptions and that would result in null.

Speaking about the consequences, I can assume that such situation is not something that we expect to happen, but in case it happens according to these condition

workspace != null

we will skip the next block of code

for (RemoteLsLauncherProvider launcherProvider : launcherProviders) {
   combinedLaunchers.addAll(launcherProvider.getAll(workspace));
}

In case you are interested in those exceptions, I would recommend you to read javadoc of org.eclipse.che.api.core.rest.HttpJsonRequest#request

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for the explanation

for (RemoteLsLauncherProvider launcherProvider : launcherProviders) {
combinedLaunchers.addAll(launcherProvider.getAll(workspace));
}
}

List<LanguageServerLauncher> result = new ArrayList<>();
for (LanguageServerLauncher launcher : launchers) {
for (LanguageServerLauncher launcher : combinedLaunchers) {
if (launcher.isAbleToLaunch()) {
int score = matchScore(launcher.getDescription(), fileUri, language.getLanguageId());
if (score > 0) {
Expand Down Expand Up @@ -345,4 +377,25 @@ public InitializedLanguageServer getServer(String id) {
}
return null;
}

private Workspace getWorkspaceConfiguration() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDE has Workspace API client (WorkspaceServiceClient). Can you reuse it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to, but unfortunately no. It is interesting for me to know what, from your point of view, allows us to assume that this client code can be used on the server?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that we have a client for WS API, but forgot that it is a client-side only thing

if (workspace != null) {
return workspace;
}

String href =
fromUri(apiEndpoint)
.path(WorkspaceService.class)
.path(WorkspaceService.class, "getByKey")
.queryParam("includeInternalServers", true)
.build(workspaceId)
.toString();
try {
return workspace =
httpJsonRequestFactory.fromUrl(href).useGetMethod().request().asDto(WorkspaceDto.class);
} catch (IOException | ApiException e) {
LOG.error("Did not manage to get workspace configuration: {}", workspaceId, e);
return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public void removeObserver(ServerInitializerObserver observer) {
public CompletableFuture<Pair<LanguageServer, InitializeResult>> initialize(
LanguageServerLauncher launcher, LanguageClient client, String projectPath)
throws LanguageServerException {
InitializeParams initializeParams = prepareInitializeParams(projectPath);
InitializeParams initializeParams = prepareInitializeParams(launcher, projectPath);
String launcherId = launcher.getDescription().getId();
CompletableFuture<Pair<LanguageServer, InitializeResult>> result =
new CompletableFuture<Pair<LanguageServer, InitializeResult>>();
Expand Down Expand Up @@ -142,9 +142,16 @@ protected void registerCallbacks(LanguageServer server, LanguageServerLauncher l
}
}

private InitializeParams prepareInitializeParams(String projectPath) {
private InitializeParams prepareInitializeParams(
LanguageServerLauncher launcher, String projectPath) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at that code I think that you've struggled to fit a new feature in a non-conforming architecture of components. Have you thought about refactoring of this subsystem to more agnostic to local LS launching?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right, there was some struggle, but tell me this, how on earth did you guess that I sometimes really think of making this subsystem more agnostic to local LS launching? Unfortunately it is not in the nearest plans yet.

InitializeParams initializeParams = new InitializeParams();
initializeParams.setProcessId(PROCESS_ID);

if (launcher.isLocal()) {
initializeParams.setProcessId(PROCESS_ID);
} else {
initializeParams.setProcessId(null);
}

initializeParams.setRootPath(LanguageServiceUtils.removeUriScheme(projectPath));
initializeParams.setRootUri(projectPath);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright (c) 2012-2017 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.api.languageserver.remote;

import java.util.Map;
import javax.inject.Singleton;

/** Detects if machine server attributes indicates that we are dealing with language server. */
@Singleton
class LsConfigurationDetector {
/**
* Tests attributes for a language server indicator
*
* @param attributes map with machine server attributes
* @return true if language server is detected, false otherwise
*/
boolean isDetected(Map<String, String> attributes) {
return "ls".equals(attributes.get("type"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have this "magic" constant described anywhere in our javadocs?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't have it, I'd say it's an internal implementation and it is not as important as, for example, the fact that this constant is not described in the documentation, because as far as I know the documentation is under construction at the moment, so I guess we'll have to wait a bit until it is ready. Could you satisfy my curiosity, tell me please why are you taking into account "ls" constant and not "type" that stands next, or others, is it somehow special for you?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pointed out that value. And yes, just type does look a bit simple for me since attributes might contain different attributes for different ports of the application. But this is not a question to you, personally, or this PR since I know that it was discussed by maintainers.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright (c) 2012-2017 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.api.languageserver.remote;

import static java.util.Collections.emptyList;
import static java.util.Collections.unmodifiableList;

import com.google.gson.JsonArray;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import javax.inject.Inject;
import javax.inject.Singleton;
import org.eclipse.che.api.languageserver.registry.DocumentFilter;
import org.eclipse.che.api.languageserver.registry.LanguageServerDescription;

/**
* This class is responsible for language server description extraction out of server attributes
* map. It is expected that there will be specific attribute named <code>config</code> that will
* contain serialized json data that represents all language server configuration data. Structure of
* json corresponds to {@link LanguageServerDescription} class with all aggregated classes
*/
@Singleton
class LsConfigurationExtractor {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks much more complex than parsing to an object. Have you considered creating a model object?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had some consideration for creating a model object.

private final JsonParser jsonParser;

@Inject
LsConfigurationExtractor(JsonParser jsonParser) {
this.jsonParser = jsonParser;
}

LanguageServerDescription extract(Map<String, String> attributes) {
String config = attributes.get("config");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed before I would opt for a more FQN like name because the current one is not very descriptive.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we look at configuration fragment:

{
   "environments":{
      "default":{
         "warnings":[

         ],
         "machines":{
            "typescript-ls-machine":{
               "env":{

               },
               "installers":[
                  "org.eclipse.che.exec",
                  "org.eclipse.che.terminal"
               ],
               "servers":{
                  "ls":{
                     "attributes":{
                        "internal":"true",
                        "type":"ls",
                        "config":"{\"id\":\"org.eclipse.che.plugin.web.typescript\", \"documentFilters\":[  {  \"languageId\":\"typescript\",  \"pathRegex\":\".*\\\\.(ts|tsx)\"} ]}"
                     },
                     "protocol":"tcp",
                     "port":"4417"
                  }
               },
               "volumes":{
                  "projects":{
                     "path":"/projects"
                  }
               },
               "attributes":{
                  "memoryLimitBytes":"1073741824"
               }
            }
         }
      }
   }
}

we see that environments have machines which have servers which have attributes so from this perspective it looks descriptive enough and the logical FQN is something like default->typescript-ls-machine->ls->config. On the other hand in the code in the decription it is mentioned: "class is responsible for language server description extraction out of server attributes map" so in underlined code line attributes.get("config") it should be rather obvious that keyword cofig is related to server's attributes while server is related to machin and so on. Again I'm wondering do you think that it is just config not descriptive or internal and type are not descriptive as well?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was concerned about attributes only since they do not have a fixed structure. And yes, I was thinking the same about internal and type.

JsonObject configJsonObject = jsonParser.parse(config).getAsJsonObject();
String id = getId(configJsonObject);
List<String> languageIds = getLanguageIds(configJsonObject);
List<String> fileWatchPatterns = getFileWatchPatterns(configJsonObject);
List<DocumentFilter> documentFilters = getDocumentFilters(configJsonObject);

return new LanguageServerDescription(id, languageIds, documentFilters, fileWatchPatterns);
}

private String getId(JsonObject jsonObject) {
return !jsonObject.has("id") ? null : jsonObject.get("id").getAsString();
}

private List<String> getLanguageIds(JsonObject jsonObject) {
if (!jsonObject.has("languageIds")) {
return emptyList();
}

JsonArray languageIdsJsonArray = jsonObject.get("languageIds").getAsJsonArray();
int size = languageIdsJsonArray.size();
List<String> languageIds = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
String languageId = languageIdsJsonArray.get(i).getAsString();
languageIds.add(languageId);
}

return unmodifiableList(languageIds);
}

private List<String> getFileWatchPatterns(JsonObject jsonObject) {
if (!jsonObject.has("fileWatchPatterns")) {
return emptyList();
}

JsonArray fileWatchPatternsJsonArray = jsonObject.get("fileWatchPatterns").getAsJsonArray();
int size = fileWatchPatternsJsonArray.size();
List<String> fileWatchPatterns = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
String fileWatchPattern = fileWatchPatternsJsonArray.get(i).getAsString();
fileWatchPatterns.add(fileWatchPattern);
}

return unmodifiableList(fileWatchPatterns);
}

private List<DocumentFilter> getDocumentFilters(JsonObject jsonObject) {
if (!jsonObject.has("documentFilters")) {
return emptyList();
}
JsonArray documentFiltersJsonArray = jsonObject.get("documentFilters").getAsJsonArray();

int size = documentFiltersJsonArray.size();
List<DocumentFilter> documentFilters = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
JsonObject documentFilterJsonObject = documentFiltersJsonArray.get(i).getAsJsonObject();

String pathRegex;
if (documentFilterJsonObject.has("pathRegex")) {
pathRegex = documentFilterJsonObject.get("pathRegex").getAsString();
} else {
pathRegex = null;
}

String languageId;
if (documentFilterJsonObject.has("languageId")) {
languageId = documentFilterJsonObject.get("languageId").getAsString();
} else {
languageId = null;
}

String schema;
if (documentFilterJsonObject.has("scheme")) {
schema = documentFilterJsonObject.get("scheme").getAsString();
} else {
schema = null;
}

DocumentFilter documentFilter = new DocumentFilter(languageId, pathRegex, schema);
documentFilters.add(documentFilter);
}

return unmodifiableList(documentFilters);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (c) 2012-2017 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.api.languageserver.remote;

import com.google.inject.AbstractModule;
import com.google.inject.multibindings.Multibinder;

public class LsRemoteModule extends AbstractModule {

@Override
protected void configure() {
Multibinder.newSetBinder(binder(), RemoteLsLauncherProvider.class)
.addBinding()
.to(SocketLsLauncherProvider.class);
}
}
Loading