Skip to content

Don't re-trigger twice Java validation when a Java file is saved. #500

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.

Sign up for GitHub

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

Closed
angelozerr wants to merge 1 commit into from
Closed

Don't re-trigger twice Java validation when a Java file is saved. #500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 48 additions & 1 deletion ...main/java/org/eclipse/lsp4mp/jdt/internal/core/MicroProfilePropertiesListenerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,23 @@
import java.util.logging.Logger;

import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.resources.IResourceChangeEvent;
import org.eclipse.core.resources.IResourceChangeListener;
import org.eclipse.core.resources.IResourceDelta;
import org.eclipse.core.resources.IResourceDeltaVisitor;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IPath;
import org.eclipse.jdt.core.ElementChangedEvent;
import org.eclipse.jdt.core.IElementChangedListener;
import org.eclipse.jdt.core.IJavaElement;
import org.eclipse.jdt.core.IJavaElementDelta;
import org.eclipse.jdt.core.IJavaProject;
import org.eclipse.jdt.core.IPackageFragmentRoot;
import org.eclipse.jdt.core.JavaCore;
import org.eclipse.jdt.core.JavaModelException;
import org.eclipse.lsp4mp.commons.MicroProfilePropertiesChangeEvent;
import org.eclipse.lsp4mp.commons.MicroProfilePropertiesScope;
import org.eclipse.lsp4mp.jdt.core.IMicroProfilePropertiesChangedListener;
Expand Down Expand Up @@ -146,11 +150,17 @@ public boolean visit(IResourceDelta delta) throws CoreException {
if (resource == null) {
return false;
}

// Step 5: Process folders, files, and projects as needed
switch (resource.getType()) {
case IResource.ROOT:
return true;
case IResource.PROJECT:
IProject project = (IProject) resource;
return project.isAccessible() && project.hasNature(JavaCore.NATURE_ID);
case IResource.FOLDER:
return resource.isAccessible();
// Ignore folder which belongs to Java output file location (ex: target/classes)
return resource.isAccessible() && !isInOutput(resource);
case IResource.FILE:
IFile file = (IFile) resource;
if (isJavaFile(file) && isFileContentChanged(delta)) {
Expand All @@ -171,6 +181,43 @@ public boolean visit(IResourceDelta delta) throws CoreException {
return false;
}

/**
* Returns true if the given (folder) resource is in the Java output location
* (ex : /target/classes) and false otherwise (ex: src/main/java).
*
* @param resource the folder resource.
* @return true if the given (folder) resource is in the Java output location
* (ex : /target/classes) and false otherwise (ex: src/main/java).
* @throws JavaModelException
*/
private static boolean isInOutput(IResource resource) throws JavaModelException {

IJavaProject javaProject = JavaCore.create(resource.getProject());
IPath resourcePath = resource.getFullPath();

// Exclude resources in the main output location (e.g.,/ProjectName/bin)
IPath outputLocation = javaProject.getOutputLocation();
IPath outputFullPath = ResourcesPlugin.getWorkspace().getRoot().getFolder(outputLocation).getFullPath();
Copy link
Copy Markdown
Contributor

@rgrunber rgrunber May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't the full path.

image

You could compute the path relative to the workspace, in which case I don't think you need outputFullPath. you can do outputLocation.isPrefixOf(resourcePath).

If you want the absolute path on the filesystem, why not do something like javaProject.getProject().getLocation().append(outputLocation) which should be the absolute path. I've seen it used in some other places.

if (outputFullPath.isPrefixOf(resourcePath)) {
return true;
}

// Exclude resources in custom output locations (if any)
for (IPackageFragmentRoot root : javaProject.getPackageFragmentRoots()) {
if (root.getKind() == IPackageFragmentRoot.K_SOURCE) {
IPath customOutput = root.getRawClasspathEntry().getOutputLocation();
if (customOutput != null) {
IPath customFullPath = ResourcesPlugin.getWorkspace().getRoot().getFolder(customOutput)
.getFullPath();
if (customFullPath.isPrefixOf(resourcePath)) {
return true;
}
}
}
}
return false;
}

private void fireAsyncEvent(MicroProfilePropertiesChangeEvent event) {
// IMPORTANT: The LSP notification 'microprofile/propertiesChanged' must be
// executed
Expand Down
22 changes: 16 additions & 6 deletions ...lipse.lsp4mp.ls/src/main/java/org/eclipse/lsp4mp/ls/java/JavaFileTextDocumentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
import org.eclipse.lsp4mp.ls.commons.TextDocument;
import org.eclipse.lsp4mp.ls.commons.ValidatorDelayer;
import org.eclipse.lsp4mp.ls.commons.client.CommandKind;
import org.eclipse.lsp4mp.ls.commons.client.ExtendedCompletionCapabilities;
import org.eclipse.lsp4mp.ls.java.JavaTextDocuments.JavaTextDocument;
import org.eclipse.lsp4mp.ls.properties.IPropertiesModelProvider;
import org.eclipse.lsp4mp.model.Node;
Expand Down Expand Up @@ -93,7 +92,8 @@ public class JavaFileTextDocumentService extends AbstractTextDocumentService {
private ValidatorDelayer<JavaTextDocument> validatorDelayer;

public JavaFileTextDocumentService(MicroProfileLanguageServer microprofileLanguageServer,
IPropertiesModelProvider propertiesModelProvider, SharedSettings sharedSettings, JavaTextDocuments javaTextDocuments) {
IPropertiesModelProvider propertiesModelProvider, SharedSettings sharedSettings,
JavaTextDocuments javaTextDocuments) {
super(microprofileLanguageServer, sharedSettings);
this.propertiesModelProvider = propertiesModelProvider;
this.documents = javaTextDocuments;
Expand Down Expand Up @@ -125,8 +125,17 @@ public void didClose(DidCloseTextDocumentParams params) {

@Override
public void didSave(DidSaveTextDocumentParams params) {
// Do nothing..

// Since the beginning of the MP LS project,
// saving a Java file triggers validation for all open Java files,
// which can negatively impact performance.
// As there's no comment explaining the reason for this behavior,
// we are disabling this feature.

// validate all opened java files which belong to a MicroProfile project
triggerValidationForAll(null);
// triggerValidationForAll(null);
Copy link
Copy Markdown
Contributor

@rgrunber rgrunber May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this breaks the following behaviour. You have a Java source file that defines configuration properties used in the configuration file. If those are added/removed, it affects some diagnostics :

Without this change

Screencast.From.2025-05-16.12-36-34.mp4

With this change

Screencast.From.2025-05-16.12-43-30.mp4

The diagnostics will not update when saving the config file, but they do update when continuing to type in the source file, or re-opening it. Are we ok with that ?

vscode-java does have a setting that causes this exact behaviour by default (ie. don't update opened document diagnostics when another source file being modified affects it). Mainly for performance gains.

Copy link
Copy Markdown
Contributor Author

@angelozerr angelozerr May 16, 2025

Choose a reason for hiding this comment

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

I remember that I had the similar problem with IJ Quarkus.

I am pretty sure that if you save your application.properties a second time, it will work.

If it that it is because we load in cache properties (for checking validation on Java file) from the /target/application.properties and not from src/main/resources/application.properties.

I think we should load cache from src/main/resources/application.properties.

We will loose the computed variable from maven for instance declared in teh properties file, but I have never seen this usecase.

Copy link
Copy Markdown
Contributor

@rgrunber rgrunber May 16, 2025

Choose a reason for hiding this comment

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

Saving a second time will not fix this. Validation isn't triggered. You either need to type, or re-open the file.

Copy link
Copy Markdown
Contributor Author

@angelozerr angelozerr May 16, 2025

Choose a reason for hiding this comment

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

That's strange it is working for me?

Copy link
Copy Markdown
Contributor

@rgrunber rgrunber May 20, 2025

Choose a reason for hiding this comment

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

Are you making changes and then saving or just hitting "Save" on an unchanged file ? Making changes will update the diagnostics but saving doesn't. Where would the validation come from when triggerValidationForAll is removed from didSave above.

Copy link
Copy Markdown
Contributor Author

@angelozerr angelozerr May 20, 2025

Choose a reason for hiding this comment

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

Are you making changes and then saving or just hitting "Save" on an unchanged file ? Making changes will update the diagnostics but saving doesn't. Where would the validation come from when triggerValidationForAll is removed from didSave above.

Type a space and szve it twice.

I need to investigate more a proper fix.

Copy link
Copy Markdown
Contributor

@rgrunber rgrunber May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Typing a space in the Java source file even without saving the file will update the diagnostics. Look at the video under "With this change" above! You can see just typing updates the diagnostics so that's fine. What I mean is that saving a file that has no changes doesn't update the diagnostics.

Copy link
Copy Markdown
Contributor Author

@angelozerr angelozerr May 20, 2025

Choose a reason for hiding this comment

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

Sorry I mean tyep a space and save application.properried and do that twice.

It should update java diagnostics correctly, right?

Copy link
Copy Markdown
Contributor

@rgrunber rgrunber May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, it does work. Interesting.

Update: I mean for the first save, the MP LS responds with no diagnostics, but on the second save it returns the diagnostic(s). I guess it can't save + return the updates diagnostics at the same time ? I guess that's fine for now.

@angelozerr all that's left is to address my comment at #500 (review)


}

// ------------------------------ Completion ------------------------------
Expand Down Expand Up @@ -167,8 +176,9 @@ public CompletableFuture<Either<List<CompletionItem>, CompletionList>> completio
JavaCursorContextResult cursorContext = completionResult.getCursorContext();

// calculate the snippet completion items based on the context
List<CompletionItem> snippetCompletionItems = documents.getSnippetRegistry().getCompletionItems(document, finalizedCompletionOffset,
canSupportMarkdown, snippetsSupported, (context, model) -> {
List<CompletionItem> snippetCompletionItems = documents.getSnippetRegistry().getCompletionItems(
document, finalizedCompletionOffset, canSupportMarkdown, snippetsSupported,
(context, model) -> {
if (context != null && context instanceof SnippetContextForJava) {
return ((SnippetContextForJava) context)
.isMatch(new JavaSnippetCompletionContext(projectInfo, cursorContext));
Expand Down Expand Up @@ -339,7 +349,7 @@ private void validate(JavaTextDocument javaTextDocument, boolean delay) {
/**
* Validate the given opened Java file.
*
* @param document the opened Java file.
* @param document the opened Java file.
*/
private void triggerValidationFor(JavaTextDocument document) {
document.executeIfInMicroProfileProject((projectinfo, cancelChecker) -> {
Expand Down
Loading