java.getPackageData is broken for some Maven modules

Sheng Chen (@jdneo)

At my current project, all of the Maven module showed the root in the "Java Projects" view:

I did some debugging and found the problem:

vscode-java-dependency/jdtls.ext/com.microsoft.jdtls.ext.core/src/com/microsoft/jdtls/ext/core/PackageCommand.java

Lines 586 to 590 in 2ba3227

    
           // For multi-module scenario, findContainersForLocationURI API may return a container array, 
        
           // put the result from the nearest project in front. 
        
           Arrays.sort(containers, (Comparator<IContainer>) (IContainer a, IContainer b) -> { 
        
               return a.getFullPath().toPortableString().length() - b.getFullPath().toPortableString().length(); 
        
           });

This code originated from here: #425

This works only for some projects. Here is a small project where it doesn't work: mvn-multimodule-no-packagedata.zip

It has the following structure:

pom.xml (parent with artifactId de.myorg.myservice)
  level1/
    pom.xml (with artifactId de.my.org.myservice.level1)

The code above will receive the following containers[]:

F/de.myorg.myservice/level1
P/de.myorg.myservice.level1

Because they have the same path length, sorting will do nothing and the code will call .getProject() on the first folder which returns the root project. My project at work had a lot more nesting (4 levels):

F/de.myorg.myservice/level1/level2/level3
F/de.myorg.myservice.level1/level2/level3
F/de.myorg.myservice/level1.level2/level3
P/de.myorg.myservice.level1.level2.level3

Essentially this code breaks, if the artifactId of the project is the same (as in my case) or longer than the path to the project. This should also break the code:

pom.xml
  level1/
     pom.xml (with artifactId myIncredibleLongArtifactIdWhichDoesNotFollowTheDirectoryLayout)

The code will then look at:

F/de.myorg.myservice/level1
P/myIncredibleLongArtifactIdWhichDoesNotFollowTheDirectoryLayout

The documentation for findContainersForLocationURI says:

	 * If the path maps to a project, the resulting array will contain a
	 * resource of type <code>PROJECT</code>, along with any linked folders that
	 * share the same location. Otherwise the resulting array will contain
	 * folders (type <code>FOLDER</code>).

I then added the following code above the sorting:

        Optional<IContainer> maybeProject = Arrays.stream(containers).filter(container -> container instanceof IProject).findFirst();
        if (maybeProject.isPresent()) {
        	return (IProject) maybeProject.get();
        }

I believe the code should return nothing, if the array does not contain a project. If the method returns only an array of folders, calling .getProject() on the first project (or the one with the shortest path) should always return the root project and displaying files of the root project is not useful in the "Java Projects" view.

Sheng Chen (@jdneo) , I can create a MR with some tests, if you agree with me. Otherwise let me know how it should work.

Frederik Claus (@fvclaus)

Thank you Frederik Claus (@fvclaus) for your detailed analysis!

And yes, the changing looks good to me. Let's do a filter to only pick the IProjects before sorting.

Please go ahead to create a PR if you would like to.👍

I don't think we need the sorting anymore. According to the JavaDoc, the result is either an array of one project and folders or an array of just folders. Do you know of a project where findContainersForLocationURI returns an array with more than one project? It would have to be two projects with the same location URI.

My plan would be to find the one project in the array, if there is no project, then return null and handle it in the caller by logging a warning and returning an empty list. Not finding a project should never occur, because the input to java.getPackageData with kind=PROJECT is the output of java.project.list.

I don't think we need the sorting anymore. According to the JavaDoc, the result is either an array of one project and folders or an array of just folders.

You are right. Let's go as your proposal.

Frederik Claus (@fvclaus)

Hi Frederik Claus (@fvclaus), thank you for your investigation. I think your findings solves #766.

Below is the code I used to fix the issue:

public static IProject getProject(String projectUri) {
        IWorkspaceRoot root = ResourcesPlugin.getWorkspace().getRoot();
        IContainer[] containers = root.findContainersForLocationURI(JDTUtils.toURI(projectUri));

        if (containers.length == 0) {
            return null;
        }

        Optional<IContainer> maybeProject = Arrays.stream(containers).filter(IProject.class::isInstance).findFirst();
        if (maybeProject.isPresent()) {
        	return (IProject) maybeProject.get();
        }

        return null;
    }

Would you like to raise a PR for that? It is not necessary to contain tests in your PR if you have no time adding it. I can take care of it later.

If, you have no time recently, would you mind me to create that PR representing you?

Sheng Chen (@jdneo)

Sheng Chen (@jdneo), I will try to create an MR until the end of this week.

Fixes microsoft#788

Sheng Chen (@jdneo)

Sheng Chen (@jdneo), the MR is still a WIP here: fvclaus#1
I had a lot of issues with the tests:

They assume a clean environment (no user settings), but when I run them locally my user settings get merged with the workspace settings resulting in files being hidden that the tests expects to find. I fixed this by setting all required settings in the settings.json:
They also alter settings during the test execution without cleaning up afterwards breaking the following test executions. I added a cleanup function:
The UI job failed with linter issues. I fixed all the linter issues

Currently, I am stuck at the UI Tests. They are using some labels that do not exist anymore and suffer from the same issue as the unit tests (altering project state without cleanup breaking successive test executions). I made some progress, but haven't finished yet. I have time to work on this issue a few hours every day the next few days.

I will create an MR here, once I am done.

Frederik Claus (@fvclaus)

Thank you Frederik Claus (@fvclaus). It's totally fine without the UI tests if you haven't enough time.

Ideally, I should setup a unit test project (another OSGi bundle), which is dedicated to testing the Java side code. It's a pity that it is not setup which makes the contribution trickier. Sorry for that...

Sheng Chen (@jdneo)

Sheng Chen (@jdneo). No, it's no problem. I don't want to merge this with failing tests. I made some progress today and will very likely finish tomorrow. I worked around a few issues in vscode-extension-tester where the PageObjects don't work with the vscode version we are using for tests (1.77.0):

item.getActionButton is broken:

settingsEditor.switchToPerspective('Workspace') is broken:

menu.getItem is broken:

I also sped up local test execution:

And finally reduced some code duplication.

Currently 7 are passing and 6 are still skipped:

I worked on it some more. Two tests are still failing in the pipeline, but they work locally (so it can't be too difficult to fix):

Thank you! Looking forward to it :)

Fixes java.getPackageData for modules with long name

Navigation Menu

Repository navigation

java.getPackageData is broken for some Maven modules #788

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// For multi-module scenario, findContainersForLocationURI API may return a container array,
	// put the result from the nearest project in front.
	Arrays.sort(containers, (Comparator<IContainer>) (IContainer a, IContainer b) -> {
	return a.getFullPath().toPortableString().length() - b.getFullPath().toPortableString().length();
	});

java.getPackageData is broken for some Maven modules #788

Description

Activity

jdneo commented on Sep 18, 2023

fvclaus commented on Sep 18, 2023

jdneo commented on Sep 18, 2023

jdneo commented on Sep 21, 2023

fvclaus commented on Sep 21, 2023

fvclaus commented on Sep 24, 2023

jdneo commented on Sep 25, 2023

fvclaus commented on Sep 26, 2023

fvclaus commented on Sep 27, 2023

jdneo commented on Sep 28, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions