Classpath changed sends too many microprofile/propertiesChanged notifications

[angelozerr]

Classpath changed sends too many microprofile/propertiesChanged notifications

Fixes #206

Signed-off-by: azerr azerr@redhat.com

[angelozerr]

Before testing the PR, I suggest you that you see the problem in master.

test 1 : space in pom.xml

• open the rest-client-quickstart project which have 6 java sources files.
• open the application.properties (to compute properties)
• open the pom.xml
• add a space and save it.

In the LanguageSupport for Java Terminal, you will see that there will have 6 microprofile/propertiesChanged events.

With the PR, you should see that there are no events.

test 2 : add dependency in pom.xml

• open the rest-client-quickstart project which have 6 java sources files.
• open the application.properties (to compute properties)
• open the pom.xml
• add a dependency in pom.xml and save it.

In the LanguageSupport for Java Terminal, you will see that there will have 7 microprofile/propertiesChanged events (one with 1,2 type) types which means classpath changed and 6 with 1 type.

With the PR, you should see that there are just one event with 1,2 type.

test3 : change sources

To test this PR, you must change java sources file and save it, and check properties are recomputed correctly.

Please note test are not working, I must find a solution for that otherwise I will disable it.

[xorye]

Both test1 and test2 work great on my machine.

For test3, with the config-quickstart project, sometimes I am not getting any microprofile/propertiesChanged events when changing a config property name in GreetingResource.java.

For example, in GreetingResource.java I would change greeting.message to greeting.messag and then hit save and the microprofile/propertiesChanged would not happen (I computed properties by invoking completion in application.properties beforehand). As a result, when I go back into the application.properties file, there would be no completion for greeting.messag.

Although this does not always happen, I find this issue easy to reproduce on my machine.

[angelozerr]

Although this does not always happen, I find this issue easy to reproduce on my machine.

@xorye I cannot reproduce it -(

[xorye]

@angelozerr I'm not sure if it will help but here's a gif of the issue:

If I understand correctly, every time I edit and save the GreetingResource file, I should see a new microprofile/propertiesChanged command being sent in the Java serer trace.

When I first edit and save the file, I do get "1 of 1" from the server trace search results as expected, but when I edit and save a second time, I still get "1 of 1" instead of "1 of 2". Going back into the application.properties file, I do not get completion for the new config.

[angelozerr]

@snjeza if you have time, could you help me with this PR?

The main idea of this PR is to improve the track of Java sources files. In master I track that with resources changed listener. It causes some trouble (too many events are fired, for instance if I have 5 java files in my project and I update pom.xml it fires 5 events although java files are not updated) so I tried to improve that by using JDT Element listener in https://github.com/redhat-developer/quarkus-ls/pull/235/files#diff-621737e98587a1a0febac12c446e4438R105

Is it a proper mean to do that? It works with my Windows OS but it doesn't worksometimes on Linux OS #235 (comment)

My second question is about tests. My tests fails every time the first time. The test track the JDT element events, but as it is done asynchronously, it's hard for the test to catch the proper event when test is executed. Here the tests

quarkus-ls/microprofile.jdt/com.redhat.microprofile.jdt.test/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManagerTest.java

Line 87 in 21e2bed

public void classpathChanged() throws Exception {

Any feedback are welcome to fix those 2 problems, thanks!

[snjeza]

The main idea of this PR is to improve the track of Java sources files. In master I track that with resources changed listener. It causes some trouble (too many events are fired, for instance if I have 5 java files in my project and I update pom.xml it fires 5 events although java files are not updated

@angelozerr I think, you have to explore delta.getKind() and delta.getFlags() as the following:

diff --git a/microprofile.jdt/com.redhat.microprofile.jdt.core/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManager.java b/microprofile.jdt/com.redhat.microprofile.jdt.core/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManager.java
index 0118f10..0f95660 100644
--- a/microprofile.jdt/com.redhat.microprofile.jdt.core/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManager.java
+++ b/microprofile.jdt/com.redhat.microprofile.jdt.core/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManager.java
@@ -151,6 +151,9 @@ public class MicroProfilePropertiesListenerManager {
 				IFile file = (IFile) resource;
 				if (isJavaFile(file)) {
 					// A Java file has been saved
+					if (delta.getKind() == IResourceDelta.CHANGED && (delta.getFlags() & IResourceDelta.CONTENT) == 0) {
+						return false;
+					}
 					MicroProfilePropertiesChangeEvent event = new MicroProfilePropertiesChangeEvent();
 					event.setType(MicroProfilePropertiesScope.ONLY_SOURCES);
 					event.setProjectURIs(new HashSet<String>());

[snjeza]

My second question is about tests. My tests fails every time the first time. The test track the JDT element events, but as it is done asynchronously, it's hard for the test to catch the proper event when test is executed.

@angelozerr you can try the following patch:

diff --git a/microprofile.jdt/com.redhat.microprofile.jdt.test/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManagerTest.java b/microprofile.jdt/com.redhat.microprofile.jdt.test/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManagerTest.java
index 4e6a2b0..0efa660 100644
--- a/microprofile.jdt/com.redhat.microprofile.jdt.test/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManagerTest.java
+++ b/microprofile.jdt/com.redhat.microprofile.jdt.test/src/main/java/com/redhat/microprofile/jdt/internal/core/MicroProfilePropertiesListenerManagerTest.java
@@ -10,11 +10,12 @@
 package com.redhat.microprofile.jdt.internal.core;
 
 import java.io.File;
-import java.io.IOException;
 import java.util.HashSet;
 import java.util.Set;
 
 import org.eclipse.core.resources.IFolder;
+import org.eclipse.core.resources.IProject;
+import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.jdt.core.IClasspathEntry;
 import org.eclipse.jdt.core.ICompilationUnit;
@@ -25,7 +26,6 @@ import org.eclipse.jdt.core.IType;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import com.google.common.io.MoreFiles;
@@ -68,11 +68,15 @@ public class MicroProfilePropertiesListenerManagerTest {
 
 	private void cleanWorkinkingDir() {
 		try {
+			IProject[] projects = ResourcesPlugin.getWorkspace().getRoot().getProjects();
+			for (IProject project:projects) {
+				project.delete(true, null);
+			}
 			File dir = JavaUtils.getWorkingProjectDirectory();
 			if (dir.exists()) {
 				MoreFiles.deleteRecursively(dir.toPath(), RecursiveDeleteOption.ALLOW_INSECURE);
 			}
-		} catch (IOException e) {
+		} catch (Exception e) {
 			e.printStackTrace();
 		}
 	}
@@ -82,15 +86,16 @@ public class MicroProfilePropertiesListenerManagerTest {
 		MicroProfilePropertiesListenerManager manager = MicroProfilePropertiesListenerManager.getInstance();
 		manager.removeMicroProfilePropertiesChangedListener(projectTracker);
 		cleanWorkinkingDir();
+		projectTracker.getProjects().clear();
 	}
 
-	@Ignore
 	@Test
 	public void classpathChanged() throws Exception {
 		Assert.assertEquals(0, projectTracker.getProjects().size());
 
 		// Create a Java project -> classpath changed
 		IJavaProject project = JavaUtils.createJavaProject("test-classpath-changed", new String[] { "/test.jar" });
+		JobHelpers.waitForJobsToComplete();
 		Assert.assertEquals(1, projectTracker.getProjects().size());
 		Assert.assertEquals(JDTMicroProfileUtils.getProjectURI(project),
 				projectTracker.getProjects().iterator().next());
@@ -100,18 +105,19 @@ public class MicroProfilePropertiesListenerManagerTest {
 
 		// Update classpath -> classpath changed
 		project.setRawClasspath(new IClasspathEntry[] {}, new NullProgressMonitor());
+		JobHelpers.waitForJobsToComplete();
 		Assert.assertEquals(1, projectTracker.getProjects().size());
 		Assert.assertEquals(JDTMicroProfileUtils.getProjectURI(project),
 				projectTracker.getProjects().iterator().next());
 	}
 
-	@Ignore
 	@Test
 	public void javaSourcesChanged() throws Exception {
 		Assert.assertEquals(0, projectTracker.getProjects().size());
 
 		// Create a Java project -> classpath changed
 		IJavaProject javaProject = JavaUtils.createJavaProject("test-java-sources-changed", new String[] {});
+		JobHelpers.waitForJobsToComplete();
 		Assert.assertEquals(1, projectTracker.getProjects().size());
 		Assert.assertEquals(JDTMicroProfileUtils.getProjectURI(javaProject),
 				projectTracker.getProjects().iterator().next());
@@ -167,8 +173,7 @@ public class MicroProfilePropertiesListenerManagerTest {
 				"";
 		ICompilationUnit cu = fragment.createCompilationUnit("GreetingResource.java", str, false, null);
 
-		// The java file has been modified
-		Thread.sleep(200); // wait a moment since IQuarkusPropertiesChangedListener are fired in async mode
+		JobHelpers.waitForJobsToComplete();
 		Assert.assertEquals(1, projectTracker.getProjects().size());
 		Assert.assertEquals(JDTMicroProfileUtils.getProjectURI(javaProject),
 				projectTracker.getProjects().iterator().next());
@@ -179,8 +184,7 @@ public class MicroProfilePropertiesListenerManagerTest {
 		IType type = cu.getType("Test");
 		type.createField("private String age;", null, true, null);
 
-		// The java file has been modified
-		Thread.sleep(200); // wait a moment since IQuarkusPropertiesChangedListener are fired in async mode
+		JobHelpers.waitForJobsToComplete();
 		Assert.assertEquals(1, projectTracker.getProjects().size());
 		Assert.assertEquals(JDTMicroProfileUtils.getProjectURI(javaProject),
 				projectTracker.getProjects().iterator().next());

[angelozerr]

@snjeza I see the ideas, thanks a lot! I will try it.

[angelozerr]

I close this PR for #293