Detect Simple Java Projects on Import #12041

tsmaeder · 2018-11-23T16:46:06Z

What does this PR do?

This PR detects when a project type has been changed to a "Plain Java Project" and creates a project in jdt.ls in turn.
In order for this to work, it was necessary to send ProjectUpdateEvents from where project configs are updated. Unfortunately, ProjectUpdatedEvent was used for other things than just notifying of changes to project configs, so a redesign of the notifications in this area was necessary.
In this cleanup, notifications from jdt.ls to Che have been reduced to "classpath changed" and "maven project created". When che code changes the classpath (e.g. "Use as source folder"), the change is first done in jdt.ls and only propagated back to che through the "classpath changed" notification. Notifications from jdt.ls are handled in a separate thread in order to free up the message receiver thread in case the notification leads to a request to jdt.ls (this was creating deadlock).

What issues does this PR fix or reference?

#11516

Docs PR

This is a bugfix.

tsmaeder · 2018-11-23T16:50:23Z

ci-test

riuvshin · 2018-11-23T17:04:38Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko

Fixing building project with -DskipTests LGTM

Please wait for IDE code owners approvals before a merge

tolusha · 2018-11-27T09:16:40Z

ci-test

riuvshin · 2018-11-27T10:03:00Z

ci-build

vzhukovs · 2018-11-27T10:02:58Z

...-core-api-project/src/main/java/org/eclipse/che/api/project/server/MutableProjectConfig.java

+
+/** @author Vlad Zhukovskiy */
+@Beta
+public class MutableProjectConfig implements ProjectConfig {


I'm not an author of this class in your PR. Besides this class, you leave this one without changes. I'd propose to move MutableProjectConfig near ProjectConfig and not having two different implementations.

I did not want to move the class to "shared" because it's not part of the protocolo between wsmaster and the IDE. Removed you as an author.

As I see, file located is in wsagent module, so whats the problem to move implementation to the che-core-api-project-shared (which also is used by the client)? This will allow us not having two duplicated implementation where one of it is odd. You'll be able to use this class from your code and the client will be able to use it in own purposes.

@tsmaeder What the problem move it to the shared? Project API is part of wsagent and not related to the wsmaster communications protocol

riuvshin · 2018-11-27T10:11:18Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

vzhukovs · 2018-11-27T10:54:57Z

...nt/src/main/java/org/eclipse/che/ide/ext/java/client/service/CustomNotificationReceiver.java

-                    .getProject(Path.valueOf(project))
-                    .thenPromise(
-                        projectConfigDto -> {
-                          return projectService


Fields projectService and promises become unused in this class and can be removed as well.

dkulieshov

I see that some possibly erroneous parts was just moved (not added) however this must not be taken as a justification of adding of a bogus code. At least you can file corresponding issues, for example like you did with improper equals() method: #12040.

In general, the idea looks just fine for me, however I would like to see what @vinokurig and @vparfonov (who has more expertise in this area) will say.

dkulieshov · 2018-11-27T09:58:07Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/resources/tree/ResourceNode.java

@@ -227,7 +227,8 @@ public boolean supportGoInto() {
  @Override
  public boolean equals(Object o) {
    if (this == o) return true;
-    if (!(o instanceof ResourceNode)) return false;
+    if (o == null) return false;


According to Eclipse Che Coding Guidelines we are to follow Google Java Style Guide. Please see braces section.

I'm just doing a minimal fix for the bug here, not rewriting the method.

Then firstly, why are you fixing the bug here while there already is a separate issue for that #12040?
And secondly, if I remember well, the scale of a fix can't be a reason for a violation of coding guidelines.

I'm also support that idea, that these changes should be extracted as separated PR according to #12040 if you really think, that there is really a bug.

The fix in equals is needed for fixing this particular issue. If we fix the equals bug, we need to fix it everywhere, which does not work out, priority-wise (we have not scheduled that bug yet).

dkulieshov · 2018-11-27T10:06:28Z

...va-server/src/main/java/org/eclipse/che/plugin/java/languageserver/ProjectsSynchronizer.java

      }
    } catch (JsonSyntaxException | InterruptedException | ExecutionException | TimeoutException e) {
      LOG.error("Error getting maven projects", e);
    }
  }

+  public void ensureMavenProject(String mavenProjectPath) {
+    Optional<RegisteredProject> project = projectManager.get(mavenProjectPath);


You might consider using org.eclipse.che.api.project.server.ProjectManager#isRegistered if you only need to check if project is already registered.

...ver/src/main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerLauncher.java

dkulieshov · 2018-11-27T10:11:25Z

...ver/src/main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerLauncher.java

+                        LOG.info("updating projectconfig for {}", projectPath);
+                        eventService.publish(new ProjectClassPathChangedEvent(project.getPath()));
+                        projectManager.update(project);
+                      } catch (ForbiddenException


This probably can be shortened to ApiException, no?

Again, APIException is too broad.

dkulieshov · 2018-11-27T10:11:51Z

...ver/src/main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerLauncher.java

+          List<Object> fixedPathList = new ArrayList<>(arguments.size());
+          for (Object uri : arguments) {
+            String uriString = convertToJson(uri).getAsString();
+            String projectPath = removePrefixUri(uriString);


This may lead to errors depending on workspace configuration, please see RootDirPathProvider.

As mentioned above, not part of this PR.

Since you have encountered this problem in your work can you please file an issue to keep track of this bug?

dkulieshov · 2018-11-27T10:20:01Z

...ver/src/main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerLauncher.java

    this.processorJsonRpcCommunication = processorJsonRpcCommunication;
    this.executeCliendCommandTransmitter = executeCliendCommandTransmitter;
    this.notifyTransmitter = notifyTransmitter;
    this.eventService = eventService;
    this.projectManager = projectManager;
    this.projectSynchronizer = projectSynchronizer;
+    this.javaService = javaService;
+    this.notificationHandler = Executors.newSingleThreadExecutor();


We probably should configure at least thread name format and exception handler, please see an example

dkulieshov · 2018-11-27T10:20:58Z

...ver/src/main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerLauncher.java

@@ -75,6 +76,9 @@
  private final ProjectManager projectManager;
  private final ProjectsSynchronizer projectSynchronizer;
  private final AtomicBoolean isStarted = new AtomicBoolean(false);
+  private final JavaLanguageServerExtensionService javaService;
+
+  private ExecutorService notificationHandler;


Shouldn't this be final?

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

...va-plain-server/src/main/java/org/eclipse/che/plugin/java/plain/server/ProjectsListener.java

...ore-api-project/src/main/java/org/eclipse/che/api/project/server/impl/ProjectServiceApi.java

tsmaeder · 2018-11-27T11:57:44Z

build-check

tsmaeder · 2018-11-27T11:58:11Z

ci-test

tsmaeder · 2018-11-27T11:58:20Z

ci-build

riuvshin · 2018-11-27T12:20:05Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

tsmaeder · 2018-11-27T13:05:57Z

@dkuleshov

At least you can file corresponding issues

Like this one?

vinokurig · 2018-11-27T13:12:13Z

...main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerExtensionService.java

@@ -939,7 +938,6 @@ private void updateProjectsWithProblems(List<String> addedProjectsUri) {
                if (!projectConfig.getProblems().isEmpty()) {
                  try {
                    projectManager.update(projectConfig);
-                    eventService.publish(new ProjectUpdatedEvent(projectPath));


AFAIK this is needed to re-update the project if the project is created before the java language servers started. @tolusha can you confirm it?

No, we update all projects once the language server has finished starting.

Sorry, not to re-update the project, but to redraw the project in the project explorer tree.

Anyway it causes a bug

Can you say what scenario you see going wrong?

I am not the author of this part of code, so I can't provide scenario when this line will be called. I mean that if we have projectManager.update(projectConfig); and eventService.publish(new ProjectUpdatedEvent(projectPath)); AFAIK is used to notify the client.
Let me rephrase my question: are you sure that we don't need this line?

Or to be more precise: my testing indicates that we don't need this line.

tsmaeder · 2018-12-13T10:25:54Z

ci-test

riuvshin · 2018-12-13T20:54:45Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

skabashnyuk · 2018-12-14T07:50:57Z

...ver/src/main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerLauncher.java

+    this.notificationHandler =
+        Executors.newSingleThreadScheduledExecutor(
+            new ThreadFactoryBuilder()
+                .setNameFormat("Jdtls Notification Handler")


Suggested change

.setNameFormat("Jdtls Notification Handler")

.setNameFormat("Jdtls Notification Handler-%d")

vparfonov · 2019-01-11T09:43:52Z

ci-test

che-bot · 2019-01-11T09:50:26Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

tsmaeder · 2019-01-15T10:14:30Z

ci-test

che-bot · 2019-01-15T14:05:35Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

tsmaeder · 2019-01-15T15:18:17Z

ci-test

che-bot · 2019-01-15T15:23:10Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

tsmaeder · 2019-01-15T15:33:49Z

ci-test

che-bot · 2019-01-15T19:35:11Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12041
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder requested review from svor and tolusha November 23, 2018 16:46

tsmaeder requested review from dkulieshov, sleshchenko and vparfonov as code owners November 23, 2018 16:46

tsmaeder mentioned this pull request Nov 23, 2018

Clean up project related events eclipse-che/che-ls-jdt#90

Closed

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 Nov 23, 2018

sleshchenko approved these changes Nov 26, 2018

View reviewed changes

tsmaeder force-pushed the 11516_detect_javaprojects branch from 0b384f4 to cb9a111 Compare November 26, 2018 09:04

tsmaeder mentioned this pull request Nov 26, 2018

Languages Sprint 158 (K-3) #11903

Closed

15 tasks

dkulieshov requested a review from vinokurig November 27, 2018 09:46

vzhukovs requested changes Nov 27, 2018

View reviewed changes

vzhukovs reviewed Nov 27, 2018

View reviewed changes

dkulieshov suggested changes Nov 27, 2018

View reviewed changes

tsmaeder mentioned this pull request Nov 27, 2018

Clean up project related events eclipse-che/che-ls-jdt#91

Merged

vinokurig reviewed Nov 27, 2018

View reviewed changes

tsmaeder mentioned this pull request Dec 6, 2018

Languages Sprint 159 (L-1) #12135

Closed

19 tasks

tsmaeder force-pushed the 11516_detect_javaprojects branch from 77fd7c4 to eca85ac Compare December 13, 2018 08:43

skabashnyuk reviewed Dec 14, 2018

View reviewed changes

tsmaeder mentioned this pull request Jan 8, 2019

Languages Sprint 160 (L-2) #12352

Closed

16 tasks

tsmaeder force-pushed the 11516_detect_javaprojects branch from 26e099e to 95b41c0 Compare January 15, 2019 10:13

dkulieshov approved these changes Jan 15, 2019

View reviewed changes

vparfonov approved these changes Jan 15, 2019

View reviewed changes

vzhukovs approved these changes Jan 15, 2019

View reviewed changes

tsmaeder mentioned this pull request Jan 15, 2019

Languages Sprint 161 (L-3) #12427

Closed

30 tasks

tsmaeder added 7 commits January 16, 2019 10:52

Project does not build with -DskipTests

7665aa5

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Send project updated events

beffabe

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Handle project updated and rework project sync accordingly

2be8095

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Handle pom changed to update editor title

8ead54a

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Use snapshot version again after release

3147f1a

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Use "shutdownNow()" to not process any queued notifications

e40a17f

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Remove author from MutableProjectConfig

7b2807f

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder force-pushed the 11516_detect_javaprojects branch from 95b41c0 to 7b2807f Compare January 16, 2019 13:18

tsmaeder merged commit e405591 into eclipse-che:master Jan 16, 2019

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 17, 2019

che-bot added this to the 6.18.0 milestone Jan 17, 2019

This was referenced Jan 19, 2019

Selenium: Add/Delete 'try-catch' according to known issues #12473

Merged

[JDT LS] The plain java project is not detected after importing from github #11516

Closed

tsmaeder mentioned this pull request Feb 8, 2019

Sprint 162 (L-4) #12635

Closed

20 tasks

	.setNameFormat("Jdtls Notification Handler")
	.setNameFormat("Jdtls Notification Handler-%d")

Detect Simple Java Projects on Import #12041

Detect Simple Java Projects on Import #12041

Conversation

tsmaeder commented Nov 23, 2018

What does this PR do?

What issues does this PR fix or reference?

Docs PR

tsmaeder commented Nov 23, 2018

riuvshin commented Nov 23, 2018

sleshchenko left a comment

Choose a reason for hiding this comment

tolusha commented Nov 27, 2018

riuvshin commented Nov 27, 2018

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

riuvshin commented Nov 27, 2018

Choose a reason for hiding this comment

dkulieshov 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

tsmaeder commented Nov 27, 2018

tsmaeder commented Nov 27, 2018

tsmaeder commented Nov 27, 2018

riuvshin commented Nov 27, 2018

tsmaeder commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig Nov 28, 2018 • 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

tsmaeder commented Dec 13, 2018

riuvshin commented Dec 13, 2018

Choose a reason for hiding this comment

vparfonov commented Jan 11, 2019

che-bot commented Jan 11, 2019

tsmaeder commented Jan 15, 2019

che-bot commented Jan 15, 2019

tsmaeder commented Jan 15, 2019

che-bot commented Jan 15, 2019

tsmaeder commented Jan 15, 2019

che-bot commented Jan 15, 2019

vinokurig Nov 28, 2018 •

edited

Loading