konveyor / analyzer-lsp

Add-on that is focused on providing analysis based on the Language Server Protocol.
Apache License 2.0
12 stars 45 forks source link

Re-consider java dependencies fallback feature #740

Open aufi opened 1 week ago

aufi commented 1 week ago

Background

Java external provider uses Maven (or Gradle) to get dependencies tree for the analyzed application. If this maven command fails, there is a dependencies lookup fallback feature included in the analyzer.

Main advantage of the fallback is to provide some results even when primary method with Maven failed. However, that often leads to having different analysis results (issues/incidents), tags or dependencies. This is is currently not obvious to user and can lead to incosistent analysis results.

Why I got to writing this issue

After debugging application analysis-related bugs (summer/fall 2024), analysis issues that used fallback were much harder to reproduce and understand, than "regular" analyzer failures.

It looked that even wrong analysis results, caused e.g. by missing deps, were accepted in past e.g. by QE. This can affect also users, who aim to run applications after migration, so successful maven commands should be critical for them.

Why maven/gradle should matter more than fallback

The maven dependencies tree get might fail for multiple reasons. Inaccessible maven repos (with or without settings.xml file), missing packages there or invalid content in pom.xml files. Failure here typically means, that the analyzed application could not successfully run (before, so also after the migration).

Fallback methods provide some data for depedencies, but since maven failure could mean broken poms, there is no reliable way saying it provider correct data. Just some mostly relevant data, so analysis can proceed and show some results. This could be misleading for users.

Proposed solution

I'm not sure the fallback methods could be simply deleted, but there should be an information in the analysis, that maven failed, so some fallback was used. This is currently in the java provider log, but having implications on analysis results, this information should be much more visible.

Alternatives

Related to https://github.com/konveyor/analyzer-lsp/issues/692

konveyor-ci-bot[bot] commented 1 week ago

This issue is currently awaiting triage. If contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance. The triage/accepted label can be added by org members.

jmle commented 6 days ago

I agree that having different results is an issue, but the fallback mechanism is mostly thought for analysis done without internet connection, which is an important use-case in Konveyor. Maybe we can give some information to the user saying that the dependencies have been calculated without the use of Maven, or that errors happened during analysis, or that the project is not buildable for whatever reasons?

aufi commented 1 day ago

Thanks for feedback @jmle, sounds reasonable to me.

I'm thinking about way providing the information about Maven failure. Producing a real issue saying that Maven has failed, using dependencies fallback and pointing to a file with the Maven output, looks to be the ideal solution I'd expect.

Technically it could be a different question, maybe creating a file with Maven error output by analyzer and having a new rule detecting&reporting this file. But this is just one option, let's discuss other possible options too.

shawn-hurley commented 1 day ago

I don't have many good ideas/options besides adding a warning of sorts to the analyzer output and piping that through the GetDepenendcies, as that is currently a black box for the engine, IIRC.

I think that it would make sense to enhance the GetDependency GRPC endpoints to have an optional warnings field (if there are errors, we assume the error response, I think). Here we add these maven issues, and for these rules that look at deps, or when incidents are filtered out based on deps, we will add to the ruleset a warnings field that communicates this.

The next step is to add this information to both the Hub/UI and the static report. I think both should have a similar yellow box type of icon we are doing for errors today that lets the user know the warning.

@aufi thoughts?