From a24f7eaca23da0272743cea435ac6f1ee24b82d0 Mon Sep 17 00:00:00 2001 From: Shawn Hurley Date: Mon, 22 Jul 2024 13:08:42 -0400 Subject: [PATCH] :bug: use the location from the inital call (#668) I assume we used to find the class definition, and then had to find those references, with the updated call I can only assume we are getting the actual usage and the refenece is now finding the places that this is being called. --------- Signed-off-by: Shawn Hurley --- engine/conditions_test.go | 100 +++++++++--------- engine/engine.go | 2 +- .../pkg/java_external_provider/dependency.go | 4 +- .../java_external_provider/dependency_test.go | 2 +- .../pkg/java_external_provider/filter.go | 82 +------------- .../pkg/java_external_provider/provider.go | 4 +- .../java_external_provider/service_client.go | 2 +- .../pkg/java_external_provider/util.go | 22 +--- .../pkg/java_external_provider/util_test.go | 3 +- lsp/protocol/enums.go | 2 +- lsp/protocol/tsdocument_changes.go | 2 +- parser/rule_parser.go | 10 +- parser/rule_parser_test.go | 2 +- provider/grpc/dependency_resolver_client.go | 4 + provider/internal/builtin/provider.go | 6 -- 15 files changed, 77 insertions(+), 170 deletions(-) diff --git a/engine/conditions_test.go b/engine/conditions_test.go index d206572a..e1f3ccc9 100644 --- a/engine/conditions_test.go +++ b/engine/conditions_test.go @@ -15,19 +15,19 @@ func Test_sortConditionEntries(t *testing.T) { { title: "correctly sorted conditions should stay sorted", entries: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { As: "b", From: "a", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { As: "b", From: "a", }, @@ -35,19 +35,19 @@ func Test_sortConditionEntries(t *testing.T) { }, { title: "incorrectly sorted conditions should be sorted", entries: []ConditionEntry{ - ConditionEntry{ + { As: "b", From: "a", }, - ConditionEntry{ + { As: "a", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { As: "b", From: "a", }, @@ -55,43 +55,43 @@ func Test_sortConditionEntries(t *testing.T) { }, { title: "incorrectly sorted conditions that branch should be sorted", entries: []ConditionEntry{ - ConditionEntry{ + { As: "b", From: "a", }, - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { As: "c", From: "b", }, - ConditionEntry{ + { As: "e", From: "d", }, - ConditionEntry{ + { As: "d", From: "b", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { As: "b", From: "a", }, - ConditionEntry{ + { As: "c", From: "b", }, - ConditionEntry{ + { As: "d", From: "b", }, - ConditionEntry{ + { As: "e", From: "d", }, @@ -99,127 +99,127 @@ func Test_sortConditionEntries(t *testing.T) { }, { title: "longer chains should sort properly", entries: []ConditionEntry{ - ConditionEntry{ + { From: "e", As: "f", }, - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { From: "d", As: "e", }, - ConditionEntry{ + { From: "a", As: "b", }, - ConditionEntry{ + { From: "b", As: "c", }, - ConditionEntry{ + { From: "c", As: "d", }, - ConditionEntry{ + { From: "f", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { From: "a", As: "b", }, - ConditionEntry{ + { From: "b", As: "c", }, - ConditionEntry{ + { From: "c", As: "d", }, - ConditionEntry{ + { From: "d", As: "e", }, - ConditionEntry{ + { From: "e", As: "f", }, - ConditionEntry{ + { From: "f", }, }, }, { title: "completely reversed chains should sort properly", entries: []ConditionEntry{ - ConditionEntry{ + { From: "c", }, - ConditionEntry{ + { From: "b", As: "c", }, - ConditionEntry{ + { From: "a", As: "b", }, - ConditionEntry{ + { As: "a", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { From: "a", As: "b", }, - ConditionEntry{ + { From: "b", As: "c", }, - ConditionEntry{ + { From: "c", }, }, }, { title: "unused As should not cause error", entries: []ConditionEntry{ - ConditionEntry{ + { From: "c", As: "d", }, - ConditionEntry{ + { From: "b", As: "c", }, - ConditionEntry{ + { From: "a", As: "b", }, - ConditionEntry{ + { As: "a", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, - ConditionEntry{ + { From: "a", As: "b", }, - ConditionEntry{ + { From: "b", As: "c", }, - ConditionEntry{ + { From: "c", As: "d", }, @@ -227,12 +227,12 @@ func Test_sortConditionEntries(t *testing.T) { }, { title: "length 1 lists should not cause error", entries: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, }, expected: []ConditionEntry{ - ConditionEntry{ + { As: "a", }, }, diff --git a/engine/engine.go b/engine/engine.go index 89070600..bc9cd276 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -604,7 +604,7 @@ func (r *ruleEngine) createViolation(ctx context.Context, conditionResponse Cond }, nil } -func (r *ruleEngine) getCodeLocation(ctx context.Context, m IncidentContext, rule Rule) (codeSnip string, err error) { +func (r *ruleEngine) getCodeLocation(_ context.Context, m IncidentContext, rule Rule) (codeSnip string, err error) { if m.CodeLocation == nil { r.logger.V(6).Info("unable to get the code snip", "URI", m.FileURI) return "", nil diff --git a/external-providers/java-external-provider/pkg/java_external_provider/dependency.go b/external-providers/java-external-provider/pkg/java_external_provider/dependency.go index 4171036b..b891b6d7 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/dependency.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/dependency.go @@ -287,7 +287,7 @@ func (p *javaServiceClient) GetDependenciesDAG(ctx context.Context) (map[uri.URI } } -func (p *javaServiceClient) getDependenciesForMaven(ctx context.Context) (map[uri.URI][]provider.DepDAGItem, error) { +func (p *javaServiceClient) getDependenciesForMaven(_ context.Context) (map[uri.URI][]provider.DepDAGItem, error) { localRepoPath := getMavenLocalRepoPath(p.mvnSettingsFile) path := p.findPom() @@ -338,7 +338,7 @@ func (p *javaServiceClient) getDependenciesForMaven(ctx context.Context) (map[ur // getDependenciesForGradle invokes the Gradle wrapper to get the dependency tree and returns all project dependencies // TODO: what if no wrapper? -func (p *javaServiceClient) getDependenciesForGradle(ctx context.Context) (map[uri.URI][]provider.DepDAGItem, error) { +func (p *javaServiceClient) getDependenciesForGradle(_ context.Context) (map[uri.URI][]provider.DepDAGItem, error) { subprojects, err := p.getGradleSubprojects() if err != nil { return nil, err diff --git a/external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go b/external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go index 33957093..e68b5466 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go @@ -524,7 +524,6 @@ func Test_parseMavenDepLines(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { lines := strings.Split(tt.mavenOutput, "\n") - deps := []provider.DepDAGItem{} var err error p := javaServiceClient{ log: testr.New(t), @@ -540,6 +539,7 @@ func Test_parseMavenDepLines(t *testing.T) { } // we are not testing dep init here, so ignore error p.depInit() + var deps []provider.DepDAGItem if deps, err = p.parseMavenDepLines(lines[1:], "testdata", "pom.xml"); (err != nil) != tt.wantErr { t.Errorf("parseMavenDepLines() error = %v, wantErr %v", err, tt.wantErr) } diff --git a/external-providers/java-external-provider/pkg/java_external_provider/filter.go b/external-providers/java-external-provider/pkg/java_external_provider/filter.go index 3d0d690d..2dbd7d73 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/filter.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/filter.go @@ -2,7 +2,6 @@ package java import ( "bufio" - "context" "fmt" "net/url" "os" @@ -63,22 +62,6 @@ func (p *javaServiceClient) filterTypesInheritance(symbols []protocol.WorkspaceS return incidents, nil } -func (p *javaServiceClient) filterTypeReferences(ctx context.Context, symbols []protocol.WorkspaceSymbol) ([]provider.IncidentContext, error) { - incidents := []provider.IncidentContext{} - for _, symbol := range symbols { - references := p.GetAllReferences(ctx, symbol) - - for _, ref := range references { - incident, err := p.convertSymbolRefToIncidentContext(symbol, ref) - if err != nil { - return nil, err - } - incidents = append(incidents, incident) - } - } - return incidents, nil -} - func (p *javaServiceClient) filterDefault(symbols []protocol.WorkspaceSymbol) ([]provider.IncidentContext, error) { incidents := []provider.IncidentContext{} for _, symbol := range symbols { @@ -108,22 +91,6 @@ func (p *javaServiceClient) filterMethodSymbols(symbols []protocol.WorkspaceSymb } -func (p *javaServiceClient) filterConstructorSymbols(ctx context.Context, symbols []protocol.WorkspaceSymbol) ([]provider.IncidentContext, error) { - - incidents := []provider.IncidentContext{} - for _, symbol := range symbols { - references := p.GetAllReferences(ctx, symbol) - for _, ref := range references { - incident, err := p.convertSymbolRefToIncidentContext(symbol, ref) - if err != nil { - return nil, err - } - incidents = append(incidents, incident) - } - } - return incidents, nil -} - func (p *javaServiceClient) convertToIncidentContext(symbol protocol.WorkspaceSymbol) (provider.IncidentContext, error) { var locationURI protocol.DocumentURI var locationRange protocol.Range @@ -177,48 +144,6 @@ func (p *javaServiceClient) convertToIncidentContext(symbol protocol.WorkspaceSy return incident, nil } -func (p *javaServiceClient) convertSymbolRefToIncidentContext(symbol protocol.WorkspaceSymbol, ref protocol.Location) (provider.IncidentContext, error) { - n, u, err := p.getURI(ref.URI) - if err != nil { - return provider.IncidentContext{}, err - } - - incident := provider.IncidentContext{ - FileURI: u, - Variables: map[string]interface{}{ - KIND_EXTRA_KEY: symbolKindToString(symbol.Kind), - SYMBOL_NAME_KEY: symbol.Name, - FILE_KEY: string(u), - "package": n, - }, - } - - // based on original URI we got, we can tell if this incident appeared in a dep - if strings.HasPrefix(ref.URI, JDT_CLASS_FILE_URI_PREFIX) { - incident.IsDependencyIncident = true - } - - if ref.Range.Start.Line == 0 && ref.Range.Start.Character == 0 && ref.Range.End.Line == 0 && ref.Range.End.Character == 0 { - return incident, nil - } - - incident.CodeLocation = &provider.Location{ - StartPosition: provider.Position{ - Line: float64(ref.Range.Start.Line), - Character: float64(ref.Range.Start.Character), - }, - EndPosition: provider.Position{ - Line: float64(ref.Range.End.Line), - Character: float64(ref.Range.End.Character), - }, - } - lineNumber := int(ref.Range.Start.Line) + 1 - incident.LineNumber = &lineNumber - - return incident, nil - -} - func (p *javaServiceClient) getURI(refURI string) (string, uri.URI, error) { if !strings.HasPrefix(refURI, JDT_CLASS_FILE_URI_PREFIX) { u, err := uri.Parse(refURI) @@ -226,11 +151,11 @@ func (p *javaServiceClient) getURI(refURI string) (string, uri.URI, error) { return "", uri.URI(""), err } file, err := os.Open(u.Filename()) - defer file.Close() if err != nil { p.log.V(4).Info("unable to get package name", "err", err) return "", u, nil } + defer file.Close() scanner := bufio.NewScanner(file) name := "" for scanner.Scan() { @@ -304,9 +229,6 @@ func (p *javaServiceClient) getURI(refURI string) (string, uri.URI, error) { } if !d.IsDir() && d.Name() == jarFile { sourcesFile = path - if err != nil { - return fmt.Errorf("found error traversing files: %w", err) - } return nil } return nil @@ -331,11 +253,11 @@ func (p *javaServiceClient) getURI(refURI string) (string, uri.URI, error) { ui := uri.New(javaFileAbsolutePath) file, err := os.Open(ui.Filename()) - defer file.Close() if err != nil { p.log.V(4).Info("unable to get package name", "err", err) return "", ui, nil } + defer file.Close() n := "" scanner := bufio.NewScanner(file) for scanner.Scan() { diff --git a/external-providers/java-external-provider/pkg/java_external_provider/provider.go b/external-providers/java-external-provider/pkg/java_external_provider/provider.go index 4519fbd7..4d67743b 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/provider.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/provider.go @@ -279,6 +279,7 @@ func (p *javaProvider) Init(ctx context.Context, log logr.Logger, config provide fmt.Sprintf("%s.%s", strings.Join(mvnCoordinatesParts[1:3], "-"), strings.ToLower(mvnCoordinatesParts[3]))) } if _, err := os.Stat(downloadedPath); err != nil { + cancelFunc() return nil, fmt.Errorf("failed to download maven artifact to path %s - %w", downloadedPath, err) } config.Location = downloadedPath @@ -389,6 +390,7 @@ func (p *javaProvider) Init(ctx context.Context, log logr.Logger, config provide svcClient.initialization(ctx) err = svcClient.depInit() if err != nil { + cancelFunc() return nil, err } // Will only set up log follow one time @@ -416,7 +418,7 @@ func (p *javaProvider) Init(ctx context.Context, log logr.Logger, config provide return &svcClient, returnErr } -func resolveSourcesJarsForGradle(ctx context.Context, log logr.Logger, location string, mvnSettings string, svc *javaServiceClient) error { +func resolveSourcesJarsForGradle(ctx context.Context, log logr.Logger, location string, _ string, svc *javaServiceClient) error { ctx, span := tracing.StartNewSpan(ctx, "resolve-sources") defer span.End() diff --git a/external-providers/java-external-provider/pkg/java_external_provider/service_client.go b/external-providers/java-external-provider/pkg/java_external_provider/service_client.go index b26f7b45..ff01ca02 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/service_client.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/service_client.go @@ -72,7 +72,7 @@ func (p *javaServiceClient) Evaluate(ctx context.Context, cap string, conditionI case 2: incidents, err = p.filterMethodSymbols(symbols) case 3: - incidents, err = p.filterConstructorSymbols(ctx, symbols) + incidents, err = p.filterDefault(symbols) case 4: incidents, err = p.filterDefault(symbols) case 7: diff --git a/external-providers/java-external-provider/pkg/java_external_provider/util.go b/external-providers/java-external-provider/pkg/java_external_provider/util.go index 1952e824..dcff629a 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/util.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/util.go @@ -20,8 +20,6 @@ import ( "text/template" "github.com/go-logr/logr" - "github.com/konveyor/analyzer-lsp/engine/labels" - "github.com/konveyor/analyzer-lsp/provider" "github.com/konveyor/analyzer-lsp/tracing" "go.opentelemetry.io/otel/attribute" ) @@ -76,20 +74,6 @@ func (a alwaysDecompileFilter) shouldDecompile(j javaArtifact) bool { return bool(a) } -type excludeOpenSourceDecompileFilter map[string]*depLabelItem - -func (o excludeOpenSourceDecompileFilter) shouldDecompile(j javaArtifact) bool { - matchWith := fmt.Sprintf("%s.%s", j.GroupId, j.ArtifactId) - for _, r := range o { - if r.r.MatchString(matchWith) { - if _, ok := r.labels[labels.AsString(provider.DepSourceLabel, javaDepSourceOpenSource)]; ok { - return false - } - } - } - return true -} - type decompileJob struct { inputPath string outputPath string @@ -414,7 +398,7 @@ func explode(ctx context.Context, log logr.Logger, archivePath, projectPath stri return destDir, decompileJobs, dependencies, nil } -func createJavaProject(ctx context.Context, dir string, dependencies []javaArtifact) error { +func createJavaProject(_ context.Context, dir string, dependencies []javaArtifact) error { tmpl := template.Must(template.New("javaProjectPom").Parse(javaProjectPom)) err := os.MkdirAll(filepath.Join(dir, "src", "main", "java"), 0755) @@ -491,7 +475,7 @@ func AppendToFile(src string, dst string) error { } // toDependency returns javaArtifact constructed for a jar -func toDependency(ctx context.Context, jarFile string) (javaArtifact, error) { +func toDependency(_ context.Context, jarFile string) (javaArtifact, error) { // attempt to lookup java artifact in maven dep, err := constructArtifactFromSHA(jarFile) if err == nil { @@ -612,7 +596,7 @@ func constructArtifactFromSHA(jarFile string) (javaArtifact, error) { return dep, fmt.Errorf("failed to construct artifact from maven lookup") } -func toFilePathDependency(ctx context.Context, filePath string) (javaArtifact, error) { +func toFilePathDependency(_ context.Context, filePath string) (javaArtifact, error) { dep := javaArtifact{} // Move up one level to the artifact. we are assuming that we get the full class file here. // For instance the dir /org/springframework/boot/loader/jar/Something.class. diff --git a/external-providers/java-external-provider/pkg/java_external_provider/util_test.go b/external-providers/java-external-provider/pkg/java_external_provider/util_test.go index 30df477a..68d347cc 100644 --- a/external-providers/java-external-provider/pkg/java_external_provider/util_test.go +++ b/external-providers/java-external-provider/pkg/java_external_provider/util_test.go @@ -2,6 +2,7 @@ package java import ( "bytes" + "context" "fmt" "os" "path/filepath" @@ -27,7 +28,7 @@ func TestRenderPom(t *testing.T) { } // Call the function with the temporary directory and sample dependencies - err := createJavaProject(nil, tmpDir, dependencies) + err := createJavaProject(context.Background(), tmpDir, dependencies) if err != nil { t.Fatalf("createJavaProject returned an error: %v", err) } diff --git a/lsp/protocol/enums.go b/lsp/protocol/enums.go index 82398e22..cc25da08 100644 --- a/lsp/protocol/enums.go +++ b/lsp/protocol/enums.go @@ -117,7 +117,7 @@ func init() { namesTextDocumentSaveReason[int(FocusOut)] = "FocusOut" } -func formatEnum(f fmt.State, c rune, i int, names []string, unknown string) { +func formatEnum(f fmt.State, _ rune, i int, names []string, unknown string) { s := "" if i >= 0 && i < len(names) { s = names[i] diff --git a/lsp/protocol/tsdocument_changes.go b/lsp/protocol/tsdocument_changes.go index 2c7a524e..ac731fd3 100644 --- a/lsp/protocol/tsdocument_changes.go +++ b/lsp/protocol/tsdocument_changes.go @@ -38,5 +38,5 @@ func (d *DocumentChanges) MarshalJSON() ([]byte, error) { } else if d.RenameFile != nil { return json.Marshal(d.RenameFile) } - return nil, fmt.Errorf("Empty DocumentChanges union value") + return nil, fmt.Errorf("empty DocumentChanges union value") } diff --git a/parser/rule_parser.go b/parser/rule_parser.go index 82aeaef7..078fb7fa 100644 --- a/parser/rule_parser.go +++ b/parser/rule_parser.go @@ -763,16 +763,16 @@ func (r *RuleParser) getConditionForProvider(langProvider, capability string, va fullCondition, ok := value.(map[interface{}]interface{}) if !ok { - return nil, nil, fmt.Errorf("Unable to parse dependency condition for %s", langProvider) + return nil, nil, fmt.Errorf("unable to parse dependency condition for %s", langProvider) } for k, v := range fullCondition { key, ok := k.(string) if !ok { - return nil, nil, fmt.Errorf("Unable to parse dependency condition for %s", langProvider) + return nil, nil, fmt.Errorf("unable to parse dependency condition for %s", langProvider) } value, ok := v.(string) if !ok { - return nil, nil, fmt.Errorf("Unable to parse dependency condition for %s", langProvider) + return nil, nil, fmt.Errorf("unable to parse dependency condition for %s", langProvider) } switch key { case "name": @@ -792,11 +792,11 @@ func (r *RuleParser) getConditionForProvider(langProvider, capability string, va } if depCondition.Name == "" { - return nil, nil, fmt.Errorf("Unable to parse dependency condition for %s (name is required)", langProvider) + return nil, nil, fmt.Errorf("unable to parse dependency condition for %s (name is required)", langProvider) } if depCondition.Upperbound == "" && depCondition.Lowerbound == "" { - return nil, nil, fmt.Errorf("Unable to parse dependency condition for %s (one of upperbound or lowerbound is required)", langProvider) + return nil, nil, fmt.Errorf("unable to parse dependency condition for %s (one of upperbound or lowerbound is required)", langProvider) } return &depCondition, client, nil diff --git a/parser/rule_parser_test.go b/parser/rule_parser_test.go index 1f312dd9..ea840166 100644 --- a/parser/rule_parser_test.go +++ b/parser/rule_parser_test.go @@ -823,7 +823,7 @@ func TestLoadRules(t *testing.T) { t.Errorf("Got err: %v expected: should have error: %v or message: %v", err, tc.ShouldErr, tc.ErrorMessage) return } - if err == nil && tc.ShouldErr { + if tc.ShouldErr { t.Errorf("expected error but not none") return } diff --git a/provider/grpc/dependency_resolver_client.go b/provider/grpc/dependency_resolver_client.go index 4b1e5abb..0343a94f 100644 --- a/provider/grpc/dependency_resolver_client.go +++ b/provider/grpc/dependency_resolver_client.go @@ -35,6 +35,10 @@ func (d *dependencyLocationResolverClient) GetLocation(ctx context.Context, dep }, DepFile: depFile, }) + if err != nil { + // Igonore the error so that some failures just continue processing + return engine.Location{}, nil + } if res == nil || res.Location == nil { return engine.Location{}, nil } diff --git a/provider/internal/builtin/provider.go b/provider/internal/builtin/provider.go index 7e821a1b..654e5736 100644 --- a/provider/internal/builtin/provider.go +++ b/provider/internal/builtin/provider.go @@ -12,10 +12,6 @@ import ( const TAGS_FILE_INIT_OPTION = "tagsFile" -var ( - filePathsDescription = "file pattern to search" -) - var capabilities = []provider.Capability{} type builtinCondition struct { @@ -57,7 +53,6 @@ type jsonCondition struct { } type builtinProvider struct { - ctx context.Context log logr.Logger config provider.Config @@ -185,5 +180,4 @@ func (p *builtinProvider) Evaluate(ctx context.Context, cap string, conditionInf } func (p *builtinProvider) Stop() { - return }