🐛 Do not use SHA lookup and use artifact list instead #852
Conversation
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
jmle
added
the
do-not-merge/hold
jmle
changed the title
|
""" WalkthroughThis update refactors the Java external provider's dependency labeling and artifact identification logic. Dependency label initialization is decoupled from service client instances and made pure. Artifact identification now analyzes JAR internal structure and directory patterns, replacing SHA-based lookups. Several function and method signatures are updated to accept and propagate dependency label maps and loggers. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider
participant ServiceClient
participant Logger
participant Util
Provider->>Util: initOpenSourceDepLabels(Logger, config)
Util-->>Provider: depLabels map
Provider->>ServiceClient: SetDepLabels(depLabels)
Provider->>Util: decompileJava(Logger, fernflower, archivePath, m2Repo, cleanBin, depLabels)
Util->>Util: explode(Logger, archivePath, projectPath, m2Repo, depLabels)
Util->>Util: toDependency(Logger, depLabels, jarFile)
Util->>Util: constructArtifactFromPom(Logger, jarFile)
alt If not found in POM
Util->>Util: constructArtifactFromStructure(Logger, jarFile, depLabels)
Util->>Util: inferGroupName(jarFile)
end
Util-->>Provider: explodedPath, projectPath, error
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
283-347: Consider removing unused constructArtifactFromSHA function.Since the PR objective is to not use SHA lookup and the function is no longer called, it should be removed to reduce code maintenance burden.
#!/bin/bash # Description: Verify that constructArtifactFromSHA is not used anywhere in the codebase # Search for any references to constructArtifactFromSHA rg "constructArtifactFromSHA" --type go
🧹 Nitpick comments (2)
external-providers/java-external-provider/pkg/java_external_provider/archive.go (1)
199-411: Consider breaking down the explode function for better maintainability.The
explodefunction is quite long (200+ lines) and handles multiple responsibilities. Consider extracting the file type handling logic into separate functions.Would you like me to help refactor this function into smaller, more focused functions?
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
158-164: Remove commented-out code instead of leaving it in the codebase.Since the PR objective is to not use SHA lookup, the commented code should be removed entirely rather than left as comments.
- // (!) would it be good to keep using SHA lookup and still have the index lookup? - // 1. try to find properties from pom - // attempt to lookup java artifact in maven - //dep, err := constructArtifactFromSHA(jarFile) - //if err == nil { - // return dep, nil - //} // if we fail to lookup on maven, construct it from pom
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
external-providers/java-external-provider/pkg/java_external_provider/archive.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency.go(4 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(13 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/util.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
external-providers/java-external-provider/pkg/java_external_provider/archive.go
1-1: : # github.com/konveyor/analyzer-lsp/external-providers/java-external-provider/pkg/java_external_provider [github.com/konveyor/analyzer-lsp/external-providers/java-external-provider/pkg/java_external_provider.test]
pkg/java_external_provider/dependency_test.go:541:6: p.depInit undefined (type javaServiceClient has no field or method depInit)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
🔇 Additional comments (6)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (2)
885-937: Good refactoring to pure functions!The conversion of
initOpenSourceDepLabelsandinitExcludeDepLabelsfrom instance methods to standalone functions improves testability and separation of concerns.
941-969: Excellent improvement to functional programming style.The
loadDepLabelItemsfunction now returns the map instead of modifying state via side effects, which is a cleaner, more functional approach.external-providers/java-external-provider/pkg/java_external_provider/provider.go (2)
349-355: Well-structured initialization sequence.Moving dependency label initialization early in the provider initialization process and setting them on the client is a good architectural decision.
Also applies to: 530-532
585-701: Clean conversion to instance methods.Converting
resolveSourcesJarsForMavenandresolveSourcesJarsForGradleto instance methods reduces parameter passing and improves encapsulation.Also applies to: 815-883
external-providers/java-external-provider/pkg/java_external_provider/archive.go (1)
24-94: Well-implemented concurrent decompilation with proper synchronization.The worker pool pattern with channels and WaitGroup is correctly implemented. Good job on context cancellation support and tracing integration.
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
221-282: Clever approach to infer JAR publicity from internal structure!The logic to determine if a JAR is public by analyzing its internal package structure and matching against known patterns is innovative. The
inferGroupNamefunction correctly implements the common Java convention where package names match group IDs.
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/archive.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
external-providers/java-external-provider/pkg/java_external_provider/archive.go (1)
259-265: Fix potential resource leak with deferred close in loop.The deferred close statements inside the loop will cause resource leaks as they won't execute until the entire function returns, potentially keeping many files open simultaneously.
Apply the previously suggested fix:
- defer dstFile.Close() archiveFile, err := f.Open() if err != nil { + dstFile.Close() return "", decompileJobs, dependencies, err } - defer archiveFile.Close() if _, err := io.Copy(dstFile, archiveFile); err != nil { + dstFile.Close() + archiveFile.Close() return "", decompileJobs, dependencies, err } + dstFile.Close() + archiveFile.Close()
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
309-374: Verify complete removal of SHA-based lookup.The SHA-based lookup code is commented out but should be completely removed if it's no longer needed, as keeping large blocks of commented code can clutter the codebase.
Consider removing the commented code block entirely if the new structure-based approach is proven to work effectively:
-//func constructArtifactFromSHA(jarFile string) (javaArtifact, error) { -// dep := javaArtifact{} -// // ... (entire commented block) -//}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
external-providers/java-external-provider/pkg/java_external_provider/archive.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency.go(6 hunks)external-providers/java-external-provider/pkg/java_external_provider/util.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- external-providers/java-external-provider/pkg/java_external_provider/dependency.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
external-providers/java-external-provider/pkg/java_external_provider/archive.go
1-1: : # github.com/konveyor/analyzer-lsp/external-providers/java-external-provider/pkg/java_external_provider [github.com/konveyor/analyzer-lsp/external-providers/java-external-provider/pkg/java_external_provider.test]
pkg/java_external_provider/dependency_test.go:541:6: p.depInit undefined (type javaServiceClient has no field or method depInit)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
🔇 Additional comments (3)
external-providers/java-external-provider/pkg/java_external_provider/archive.go (1)
180-195: Improve resource management in explodeSimple.The function has good error handling but could benefit from consistent resource cleanup patterns.
The explicit close calls after
io.Copyare well-handled, avoiding the defer-in-loop issue present elsewhere in the file.external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
155-172: LGTM on removing SHA lookup approach.The refactoring to use artifact structure analysis instead of SHA-based Maven lookup aligns well with the PR objectives and should be more reliable for dependency identification.
247-307: Robust implementation of group name inference.The
inferGroupNamefunction provides a solid approach to extract group names from JAR structure by finding the longest common prefix of package paths. The handling of edge cases (inner classes, metadata paths, etc.) is comprehensive.
external-providers/java-external-provider/pkg/java_external_provider/archive.go
Outdated
Show resolved
Hide resolved
| if _, err := io.Copy(dstFile, archiveFile); err != nil { | ||
| return "", decompileJobs, dependencies, err | ||
| } | ||
| seenDirArtificat := map[string]interface{}{} |
There was a problem hiding this comment.
Fix variable name typo and scope issue.
The variable seenDirArtificat has a typo and is declared inside the switch statement, causing it to be reinitialized on each iteration.
+ seenDirArtifact := map[string]interface{}{}
for _, f := range archive.File {
// ... existing code ...
- seenDirArtificat := map[string]interface{}{}
switch {
// ... existing cases ...
- if _, ok := seenDirArtificat[filepath.Dir(f.Name)]; !ok {
+ if _, ok := seenDirArtifact[filepath.Dir(f.Name)]; !ok {
dep, err := toFilePathDependency(ctx, f.Name)
if err != nil {
log.V(8).Error(err, "error getting dependcy for path", "path", destPath)
continue
}
dependencies = append(dependencies, dep)
- seenDirArtificat[filepath.Dir(f.Name)] = nil
+ seenDirArtifact[filepath.Dir(f.Name)] = nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| seenDirArtificat := map[string]interface{}{} | |
| // Track which directories we've already added dependencies for | |
| seenDirArtifact := map[string]interface{}{} | |
| for _, f := range archive.File { | |
| // ... existing code before switch ... | |
| switch { | |
| // ... existing cases ... | |
| // only add a dependency once per directory | |
| if _, ok := seenDirArtifact[filepath.Dir(f.Name)]; !ok { | |
| dep, err := toFilePathDependency(ctx, f.Name) | |
| if err != nil { | |
| log.V(8).Error(err, "error getting dependency for path", "path", destPath) | |
| continue | |
| } | |
| dependencies = append(dependencies, dep) | |
| seenDirArtifact[filepath.Dir(f.Name)] = nil | |
| } | |
| } | |
| // ... existing code after switch ... | |
| } |
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/archive.go
at line 270, rename the variable from `seenDirArtificat` to `seenDirArtifact` to
fix the typo. Also, move the declaration of this map outside the switch
statement to ensure it is not reinitialized on each iteration, preserving its
state across iterations.
| java := filepath.Join(os.Getenv("JAVA_HOME"), "bin", "java") | ||
| // -mpm (max processing method) is required to keep decomp time low | ||
| cmd := exec.CommandContext( | ||
| jobCtx, java, "-jar", fernflower, "-mpm=30", job.inputPath, outputPathDir) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for JAVA_HOME environment variable.
The code assumes JAVA_HOME is set but doesn't validate this, which could lead to runtime errors if the environment variable is missing.
+ javaHome := os.Getenv("JAVA_HOME")
+ if javaHome == "" {
+ log.V(3).Error(fmt.Errorf("JAVA_HOME not set"), "JAVA_HOME environment variable is required")
+ continue
+ }
// multiple java versions may be installed - chose $JAVA_HOME one
- java := filepath.Join(os.Getenv("JAVA_HOME"), "bin", "java")
+ java := filepath.Join(javaHome, "bin", "java")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| java := filepath.Join(os.Getenv("JAVA_HOME"), "bin", "java") | |
| // -mpm (max processing method) is required to keep decomp time low | |
| cmd := exec.CommandContext( | |
| jobCtx, java, "-jar", fernflower, "-mpm=30", job.inputPath, outputPathDir) | |
| javaHome := os.Getenv("JAVA_HOME") | |
| if javaHome == "" { | |
| log.V(3).Error(fmt.Errorf("JAVA_HOME not set"), "JAVA_HOME environment variable is required") | |
| continue | |
| } | |
| // multiple java versions may be installed - chose $JAVA_HOME one | |
| java := filepath.Join(javaHome, "bin", "java") | |
| // -mpm (max processing method) is required to keep decomp time low | |
| cmd := exec.CommandContext( | |
| jobCtx, java, "-jar", fernflower, "-mpm=30", job.inputPath, outputPathDir) |
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/archive.go
around lines 56 to 59, the code uses the JAVA_HOME environment variable without
checking if it is set, which can cause runtime errors. Add validation to check
if JAVA_HOME is empty or unset before using it; if it is not set, return an
appropriate error or handle the case gracefully to prevent failures when
constructing the java executable path.
external-providers/java-external-provider/pkg/java_external_provider/util.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
216-243: Past logging issue has been resolved, but consider variable naming clarity.The premature logging issue from the previous review has been fixed - the log statement now correctly appears after the dependency label check.
However, consider improving variable naming for clarity since
groupIdis modified during the loop:func constructArtifactFromStructure(log logr.Logger, jarFile string, depToLabels map[string]*depLabelItem) (javaArtifact, error) { log.V(10).Info(fmt.Sprintf("trying to infer if %s is a public dependency", jarFile)) - groupId, err := inferGroupName(jarFile) + originalGroupId, err := inferGroupName(jarFile) if err != nil { return javaArtifact{}, err } - artifact := javaArtifact{GroupId: groupId} + artifact := javaArtifact{GroupId: originalGroupId} // check the inferred groupId against list of public groups // if groupId is not found, remove last segment. repeat if not found until no segments are left. - sgmts := strings.Split(groupId, ".") + groupId := originalGroupId + sgmts := strings.Split(groupId, ".")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
🔇 Additional comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
245-308: Well-implemented group name inference algorithm.The function correctly implements a longest common prefix algorithm to infer group names from JAR internal structure. Good filtering logic excludes irrelevant files (inner classes, metadata, top-level classes) and proper error handling is in place.
| //func constructArtifactFromSHA(jarFile string) (javaArtifact, error) { | ||
| // dep := javaArtifact{} | ||
| // // we look up the jar in maven | ||
| // file, err := os.Open(jarFile) | ||
| // if err != nil { | ||
| // return dep, err | ||
| // } | ||
| // defer file.Close() | ||
| // | ||
| // hash := sha1.New() | ||
| // _, err = io.Copy(hash, file) | ||
| // if err != nil { | ||
| // return dep, err | ||
| // } | ||
| // | ||
| // sha1sum := hex.EncodeToString(hash.Sum(nil)) | ||
| // | ||
| // // Make an HTTPS request to search.maven.org | ||
| // searchURL := fmt.Sprintf("https://search.maven.org/solrsearch/select?q=1:%s&rows=20&wt=json", sha1sum) | ||
| // resp, err := http.Get(searchURL) | ||
| // if err != nil { | ||
| // return dep, err | ||
| // } | ||
| // defer resp.Body.Close() | ||
| // | ||
| // // Read and parse the JSON response | ||
| // body, err := io.ReadAll(resp.Body) | ||
| // if err != nil { | ||
| // return dep, err | ||
| // } | ||
| // | ||
| // var data map[string]interface{} | ||
| // err = json.Unmarshal(body, &data) | ||
| // if err != nil { | ||
| // return dep, err | ||
| // } | ||
| // | ||
| // // Check if a single result is found | ||
| // response, ok := data["response"].(map[string]interface{}) | ||
| // if !ok { | ||
| // return dep, err | ||
| // } | ||
| // | ||
| // numFound, ok := response["numFound"].(float64) | ||
| // if !ok { | ||
| // return dep, err | ||
| // } | ||
| // | ||
| // if numFound == 1 { | ||
| // jarInfo := response["docs"].([]interface{})[0].(map[string]interface{}) | ||
| // dep.GroupId = jarInfo["g"].(string) | ||
| // dep.ArtifactId = jarInfo["a"].(string) | ||
| // dep.Version = jarInfo["v"].(string) | ||
| // dep.sha1 = sha1sum | ||
| // dep.foundOnline = true | ||
| // return dep, nil | ||
| // } else if numFound > 1 { | ||
| // dep, err = constructArtifactFromPom(jarFile) | ||
| // if err == nil { | ||
| // dep.foundOnline = true | ||
| // return dep, nil | ||
| // } | ||
| // } | ||
| // return dep, fmt.Errorf("failed to construct artifact from maven lookup") | ||
| //} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented code instead of leaving it in the codebase.
The commented-out constructArtifactFromSHA function should be removed entirely rather than left as commented code. This aligns with the PR objective and keeps the codebase clean.
-//func constructArtifactFromSHA(jarFile string) (javaArtifact, error) {
-// dep := javaArtifact{}
-// // we look up the jar in maven
-// file, err := os.Open(jarFile)
-// if err != nil {
-// return dep, err
-// }
-// defer file.Close()
-//
-// hash := sha1.New()
-// _, err = io.Copy(hash, file)
-// if err != nil {
-// return dep, err
-// }
-//
-// sha1sum := hex.EncodeToString(hash.Sum(nil))
-//
-// // Make an HTTPS request to search.maven.org
-// searchURL := fmt.Sprintf("https://search.maven.org/solrsearch/select?q=1:%s&rows=20&wt=json", sha1sum)
-// resp, err := http.Get(searchURL)
-// if err != nil {
-// return dep, err
-// }
-// defer resp.Body.Close()
-//
-// // Read and parse the JSON response
-// body, err := io.ReadAll(resp.Body)
-// if err != nil {
-// return dep, err
-// }
-//
-// var data map[string]interface{}
-// err = json.Unmarshal(body, &data)
-// if err != nil {
-// return dep, err
-// }
-//
-// // Check if a single result is found
-// response, ok := data["response"].(map[string]interface{})
-// if !ok {
-// return dep, err
-// }
-//
-// numFound, ok := response["numFound"].(float64)
-// if !ok {
-// return dep, err
-// }
-//
-// if numFound == 1 {
-// jarInfo := response["docs"].([]interface{})[0].(map[string]interface{})
-// dep.GroupId = jarInfo["g"].(string)
-// dep.ArtifactId = jarInfo["a"].(string)
-// dep.Version = jarInfo["v"].(string)
-// dep.sha1 = sha1sum
-// dep.foundOnline = true
-// return dep, nil
-// } else if numFound > 1 {
-// dep, err = constructArtifactFromPom(jarFile)
-// if err == nil {
-// dep.foundOnline = true
-// return dep, nil
-// }
-// }
-// return dep, fmt.Errorf("failed to construct artifact from maven lookup")
-//}
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //func constructArtifactFromSHA(jarFile string) (javaArtifact, error) { | |
| // dep := javaArtifact{} | |
| // // we look up the jar in maven | |
| // file, err := os.Open(jarFile) | |
| // if err != nil { | |
| // return dep, err | |
| // } | |
| // defer file.Close() | |
| // | |
| // hash := sha1.New() | |
| // _, err = io.Copy(hash, file) | |
| // if err != nil { | |
| // return dep, err | |
| // } | |
| // | |
| // sha1sum := hex.EncodeToString(hash.Sum(nil)) | |
| // | |
| // // Make an HTTPS request to search.maven.org | |
| // searchURL := fmt.Sprintf("https://search.maven.org/solrsearch/select?q=1:%s&rows=20&wt=json", sha1sum) | |
| // resp, err := http.Get(searchURL) | |
| // if err != nil { | |
| // return dep, err | |
| // } | |
| // defer resp.Body.Close() | |
| // | |
| // // Read and parse the JSON response | |
| // body, err := io.ReadAll(resp.Body) | |
| // if err != nil { | |
| // return dep, err | |
| // } | |
| // | |
| // var data map[string]interface{} | |
| // err = json.Unmarshal(body, &data) | |
| // if err != nil { | |
| // return dep, err | |
| // } | |
| // | |
| // // Check if a single result is found | |
| // response, ok := data["response"].(map[string]interface{}) | |
| // if !ok { | |
| // return dep, err | |
| // } | |
| // | |
| // numFound, ok := response["numFound"].(float64) | |
| // if !ok { | |
| // return dep, err | |
| // } | |
| // | |
| // if numFound == 1 { | |
| // jarInfo := response["docs"].([]interface{})[0].(map[string]interface{}) | |
| // dep.GroupId = jarInfo["g"].(string) | |
| // dep.ArtifactId = jarInfo["a"].(string) | |
| // dep.Version = jarInfo["v"].(string) | |
| // dep.sha1 = sha1sum | |
| // dep.foundOnline = true | |
| // return dep, nil | |
| // } else if numFound > 1 { | |
| // dep, err = constructArtifactFromPom(jarFile) | |
| // if err == nil { | |
| // dep.foundOnline = true | |
| // return dep, nil | |
| // } | |
| // } | |
| // return dep, fmt.Errorf("failed to construct artifact from maven lookup") | |
| //} |
🤖 Prompt for AI Agents
In external-providers/java-external-provider/pkg/java_external_provider/util.go
between lines 310 and 375, remove the entire commented-out
constructArtifactFromSHA function instead of leaving it commented in the
codebase. This will clean up the code and align with the PR objective.
external-providers/java-external-provider/pkg/java_external_provider/util.go
Show resolved
Hide resolved
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
657-722: Remove commented code instead of leaving it in the codebase.The commented-out
constructArtifactFromSHAfunction should be removed entirely rather than left as commented code, as noted in previous reviews. This aligns with the PR objective and keeps the codebase clean.-//func constructArtifactFromSHA(jarFile string) (javaArtifact, error) { -// dep := javaArtifact{} -// // we look up the jar in maven -// file, err := os.Open(jarFile) -// if err != nil { -// return dep, err -// } -// defer file.Close() -// -// hash := sha1.New() -// _, err = io.Copy(hash, file) -// if err != nil { -// return dep, err -// } -// -// sha1sum := hex.EncodeToString(hash.Sum(nil)) -// -// // Make an HTTPS request to search.maven.org -// searchURL := fmt.Sprintf("https://search.maven.org/solrsearch/select?q=1:%s&rows=20&wt=json", sha1sum) -// resp, err := http.Get(searchURL) -// if err != nil { -// return dep, err -// } -// defer resp.Body.Close() -// -// // Read and parse the JSON response -// body, err := io.ReadAll(resp.Body) -// if err != nil { -// return dep, err -// } -// -// var data map[string]interface{} -// err = json.Unmarshal(body, &data) -// if err != nil { -// return dep, err -// } -// -// // Check if a single result is found -// response, ok := data["response"].(map[string]interface{}) -// if !ok { -// return dep, err -// } -// -// numFound, ok := response["numFound"].(float64) -// if !ok { -// return dep, err -// } -// -// if numFound == 1 { -// jarInfo := response["docs"].([]interface{})[0].(map[string]interface{}) -// dep.GroupId = jarInfo["g"].(string) -// dep.ArtifactId = jarInfo["a"].(string) -// dep.Version = jarInfo["v"].(string) -// dep.sha1 = sha1sum -// dep.foundOnline = true -// return dep, nil -// } else if numFound > 1 { -// dep, err = constructArtifactFromPom(jarFile) -// if err == nil { -// dep.foundOnline = true -// return dep, nil -// } -// } -// return dep, fmt.Errorf("failed to construct artifact from maven lookup") -//} -
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
563-590: Improve efficiency and logging in dependency structure analysis.The implementation correctly infers group names and matches against dependency labels, but has some optimization opportunities:
+ originalGroupId := groupId sgmts := strings.Split(groupId, ".") for len(sgmts) > 0 { // check against depToLabels. add * groupIdRegex := strings.Join([]string{groupId, "*"}, ".") if depToLabels[groupIdRegex] != nil { - log.V(10).Info(fmt.Sprintf("%s is a public dependency", jarFile)) + log.V(10).Info("jar identified as public dependency", "jarFile", jarFile, "originalGroupId", originalGroupId, "matchedGroupId", groupId) artifact.foundOnline = true return artifact, nil } else { // lets try to remove one segment from the end sgmts = sgmts[:len(sgmts)-1] groupId = strings.Join(sgmts, ".") - groupIdRegex = strings.Join([]string{groupId, "*"}, ".") } } - log.V(10).Info(fmt.Sprintf("could not decide whether %s is public, setting as private", jarFile)) + log.V(10).Info("jar identified as private dependency", "jarFile", jarFile, "inferredGroupId", originalGroupId)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go(6 hunks)external-providers/java-external-provider/pkg/java_external_provider/util.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- external-providers/java-external-provider/pkg/java_external_provider/dependency.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (3)
external-providers/java-external-provider/pkg/java_external_provider/util.go (3)
87-87: LGTM: Function signature updated correctly.The
decompilefunction signature has been properly updated to accept thedepLabelsparameter, which aligns with the PR objective to use artifact lists instead of SHA lookup.
502-519: LGTM: Dependency construction logic is correct.The function correctly implements the fallback strategy: first attempting POM-based construction, then falling back to structure-based construction if POM fails. The logic properly handles both success and failure cases.
592-655: LGTM: Group name inference logic is well-implemented.The
inferGroupNamefunction correctly:
- Filters out irrelevant files (inner classes, anonymous classes, metadata)
- Handles edge cases like empty JARs
- Implements proper longest common prefix algorithm
- Uses appropriate error handling
The implementation is robust and follows good practices.
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
667-732: Remove commented code as suggested in past reviews.The commented-out
constructArtifactFromSHAfunction should be completely removed rather than left as commented code, as indicated in previous review feedback.-//func constructArtifactFromSHA(jarFile string) (javaArtifact, error) { -// dep := javaArtifact{} -// // we look up the jar in maven -// file, err := os.Open(jarFile) -// if err != nil { -// return dep, err -// } -// defer file.Close() -// -// hash := sha1.New() -// _, err = io.Copy(hash, file) -// if err != nil { -// return dep, err -// } -// -// sha1sum := hex.EncodeToString(hash.Sum(nil)) -// -// // Make an HTTPS request to search.maven.org -// searchURL := fmt.Sprintf("https://search.maven.org/solrsearch/select?q=1:%s&rows=20&wt=json", sha1sum) -// resp, err := http.Get(searchURL) -// if err != nil { -// return dep, err -// } -// defer resp.Body.Close() -// -// // Read and parse the JSON response -// body, err := io.ReadAll(resp.Body) -// if err != nil { -// return dep, err -// } -// -// var data map[string]interface{} -// err = json.Unmarshal(body, &data) -// if err != nil { -// return dep, err -// } -// -// // Check if a single result is found -// response, ok := data["response"].(map[string]interface{}) -// if !ok { -// return dep, err -// } -// -// numFound, ok := response["numFound"].(float64) -// if !ok { -// return dep, err -// } -// -// if numFound == 1 { -// jarInfo := response["docs"].([]interface{})[0].(map[string]interface{}) -// dep.GroupId = jarInfo["g"].(string) -// dep.ArtifactId = jarInfo["a"].(string) -// dep.Version = jarInfo["v"].(string) -// dep.sha1 = sha1sum -// dep.foundOnline = true -// return dep, nil -// } else if numFound > 1 { -// dep, err = constructArtifactFromPom(jarFile) -// if err == nil { -// dep.foundOnline = true -// return dep, nil -// } -// } -// return dep, fmt.Errorf("failed to construct artifact from maven lookup") -//} -
🧹 Nitpick comments (2)
external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
573-600: Review the structure-based artifact identification logic.The
constructArtifactFromStructurefunction implements a reasonable approach to identify public dependencies by checking inferred group IDs against a label map. The progressive segmentation logic (removing segments from the end) provides good fallback behavior.Consider adding validation for edge cases:
func constructArtifactFromStructure(log logr.Logger, jarFile string, depToLabels map[string]*depLabelItem) (javaArtifact, error) { + if depToLabels == nil { + return javaArtifact{}, fmt.Errorf("depToLabels map is nil") + } log.V(10).Info(fmt.Sprintf("trying to infer if %s is a public dependency", jarFile)) groupId, err := inferGroupName(jarFile) if err != nil { return javaArtifact{}, err } + if groupId == "" { + log.V(10).Info(fmt.Sprintf("could not infer group name for %s", jarFile)) + return javaArtifact{}, nil + }
602-665: Review the group name inference algorithm.The
inferGroupNamefunction implements a sensible approach to extract package structure from JAR files. The logic correctly handles edge cases like inner classes, metadata paths, and finds the longest common prefix.Consider using
filepathinstead ofpathfor consistency with the rest of the codebase:- if strings.Contains(path.Base(file.Name), "$") { + if strings.Contains(filepath.Base(file.Name), "$") { continue } // Convert each path to a list of package segments var allPaths [][]string for _, p := range classPaths { - dir := path.Dir(p) + dir := filepath.Dir(p) parts := strings.Split(dir, "/") allPaths = append(allPaths, parts) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
JavaArchive(33-33)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (3)
external-providers/java-external-provider/pkg/java_external_provider/util.go (3)
12-12: LGTM: Import addition supports new functionality.The addition of the
pathimport is appropriate for the new structure-based artifact identification logic that analyzes JAR internal directory structures.
87-87: Function signature change aligns with refactoring objectives.The addition of
depLabels map[string]*depLabelItemparameter enables the structure-based dependency identification approach, replacing SHA-based lookups as intended.
162-162: Function signature change maintains consistency.The
decompileJavafunction signature correctly propagates thedepLabelsparameter throughout the call chain.
| func toDependency(_ context.Context, log logr.Logger, depToLabels map[string]*depLabelItem, jarFile string) (javaArtifact, error) { | ||
| //dep, err := constructArtifactFromSHA(jarFile) | ||
| //if err == nil { | ||
| // return dep, nil | ||
| //} | ||
| // if we fail to lookup on maven, construct it from pom | ||
| dep, err = constructArtifactFromPom(jarFile) | ||
| if err == nil { | ||
| return dep, nil | ||
| dep, err := constructArtifactFromPom(log, jarFile) | ||
| if err != nil { | ||
| log.V(10).Info("could not construct artifact object from pom for artifact, trying to infer from structure", "jarFile", jarFile, "error", err.Error()) | ||
|
|
||
| dep, err = constructArtifactFromStructure(log, jarFile, depToLabels) | ||
| if err != nil { | ||
| return javaArtifact{}, err | ||
| } | ||
| } | ||
|
|
||
| return dep, err | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the simplified logic flow.
The refactored toDependency function now properly handles the fallback from POM-based to structure-based construction. However, verify that error handling aligns with expectations.
🏁 Script executed:
#!/bin/bash
# Description: Verify that toDependency function calls are consistent with the new signature.
# Test: Search for all calls to toDependency function. Expect: All calls should pass log, depLabels, jarFile parameters.
ast-grep --pattern 'toDependency($$$)'Length of output: 341
Ensure all toDependency calls handle errors appropriately
The function signature is used consistently across the codebase, but one call still ignores the returned error. Please update or justify this:
• external-providers/java-external-provider/pkg/java_external_provider/util.go:364
dep, err := toDependency(ctx, log, depLabels, filePath)(error handled)
• external-providers/java-external-provider/pkg/java_external_provider/dependency.go:650
artifact, _ := toDependency(context.TODO(), w.log, w.depToLabels, path)(error ignored)
Either handle the error from toDependency at line 650 or add a comment explaining why it’s safe to ignore here.
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/dependency.go
around line 650, the call to toDependency ignores the returned error, which is
inconsistent with other usages. Update this call to properly handle the error by
checking if err is non-nil and handling it appropriately, such as logging the
error or returning it. If ignoring the error is intentional, add a clear comment
explaining why it is safe to do so.
| func removeIncompleteDependencies(dependencies []javaArtifact) []javaArtifact { | ||
| complete := []javaArtifact{} | ||
| for _, dep := range dependencies { | ||
| if dep.ArtifactId != "" && dep.GroupId == "" && dep.Version == "" { | ||
| complete = append(complete, dep) | ||
| } | ||
| } | ||
| return complete | ||
| } |
There was a problem hiding this comment.
Fix critical logic bug in dependency filtering.
The function has an inverted logic condition that will remove complete dependencies and keep incomplete ones.
-func removeIncompleteDependencies(dependencies []javaArtifact) []javaArtifact {
- complete := []javaArtifact{}
- for _, dep := range dependencies {
- if dep.ArtifactId != "" && dep.GroupId == "" && dep.Version == "" {
- complete = append(complete, dep)
- }
- }
- return complete
-}
+func removeIncompleteDependencies(dependencies []javaArtifact) []javaArtifact {
+ complete := []javaArtifact{}
+ for _, dep := range dependencies {
+ if dep.ArtifactId != "" && dep.GroupId != "" && dep.Version != "" {
+ complete = append(complete, dep)
+ }
+ }
+ return complete
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func removeIncompleteDependencies(dependencies []javaArtifact) []javaArtifact { | |
| complete := []javaArtifact{} | |
| for _, dep := range dependencies { | |
| if dep.ArtifactId != "" && dep.GroupId == "" && dep.Version == "" { | |
| complete = append(complete, dep) | |
| } | |
| } | |
| return complete | |
| } | |
| func removeIncompleteDependencies(dependencies []javaArtifact) []javaArtifact { | |
| complete := []javaArtifact{} | |
| for _, dep := range dependencies { | |
| if dep.ArtifactId != "" && dep.GroupId != "" && dep.Version != "" { | |
| complete = append(complete, dep) | |
| } | |
| } | |
| return complete | |
| } |
🤖 Prompt for AI Agents
In external-providers/java-external-provider/pkg/java_external_provider/util.go
around lines 211 to 219, the condition inside removeIncompleteDependencies
incorrectly filters dependencies by keeping incomplete ones and removing
complete ones. Fix this by changing the condition to keep dependencies where
ArtifactId, GroupId, and Version are all non-empty, ensuring only complete
dependencies are retained.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
demo-output.yaml (1)
642-663: Redundant & duplicatednode-sample-rule-002blockThis second sample rule repeats almost exactly the same content as rule 001 and suffers from the same duplication & invalid
lineNumber(value2is repeated).
If these Node-specific test artefacts are no longer needed, delete them entirely; otherwise merge them into a single rule and ensure each incident appears only once with a correct 1-based line number.- node-sample-rule-002: - description: Testing that the node provider works - function - … - effort: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo-output.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark (windows-latest, windows)
jmle
force-pushed
the
MTA-5588
branch
2 times, most recently
from
ebae882 to
16490d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
demo-output.yaml (3)
629-632:lineNumber: 0is invalid (must be ≥ 1)
This was pointed out previously and is still unfixed.
635-637: Duplicated incident entry – inflates counts
The twotest_b.tsincidents are identical except forlineNumber; keep one.
618-640: Removenode-sample-rule-001– it is out-of-scope and pollutes the reportThis block is unrelated to the “artifact list / Java provider” work, brings no value to the test-data set, and introduces several defects (see follow-up comments). Deleting it is the safest and least noisy option.
- node-sample-rule-001: - description: Testing that the node provider works - type - category: potential - incidents: - - uri: file:///examples/nodejs/test_a.ts - message: nodejs sample rule 001 - codeSnip: | - 1 export interface Greeter { - 2 name: string; - 3 hello(): string; - 4 } - 5 - 6 export const greeter: Greeter = { - 7 name: "Person1", - 8 hello() { - 9 return `Hello, I'm ${this.name}`; - 10 }, - 11 }; - lineNumber: 5 - variables: - file: file:///examples/nodejs/test_a.ts - - uri: file:///examples/nodejs/test_b.ts - message: nodejs sample rule 001 - codeSnip: | - 1 import { greeter } from './test_a'; - 2 - 3 console.log(greeter.hello()); - lineNumber: 0 - variables: - file: file:///examples/nodejs/test_b.ts - - uri: file:///examples/nodejs/test_b.ts - message: nodejs sample rule 001 - codeSnip: | - 1 import { greeter } from './test_a'; - 2 - 3 console.log(greeter.hello()); - lineNumber: 2 - variables: - file: file:///examples/nodejs/test_b.ts - effort: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo-output.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: benchmark (windows-latest, windows)
jmle
force-pushed
the
MTA-5588
branch
4 times, most recently
from
5837a1b to
322cc4e
Compare
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Extends #852 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Enhanced Java dependency analysis by inferring artifact information from JAR file structure, improving detection of open source and internal dependencies. * Improved logging and error handling during Java dependency processing. * Streamlined management and assignment of dependency labels for more precise classification. * Centralized Java source resolution and dependency label initialization within the service client for better maintainability. * Added handling for unidentified embedded JARs by assigning default embedded group labels. * Introduced a new configuration option to disable Maven SHA1 lookups, providing more control over dependency resolution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Signed-off-by: Emily McMullan <[email protected]> Signed-off-by: Shawn Hurley <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Shawn Hurley <[email protected]>
Extends #852 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Enhanced Java dependency analysis by inferring artifact information from JAR file structure, improving detection of open source and internal dependencies. * Improved logging and error handling during Java dependency processing. * Streamlined management and assignment of dependency labels for more precise classification. * Centralized Java source resolution and dependency label initialization within the service client for better maintainability. * Added handling for unidentified embedded JARs by assigning default embedded group labels. * Introduced a new configuration option to disable Maven SHA1 lookups, providing more control over dependency resolution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Signed-off-by: Emily McMullan <[email protected]> Signed-off-by: Shawn Hurley <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Shawn Hurley <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
…#868) Extends #852 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Enhanced Java dependency analysis by inferring artifact information from JAR file structure, improving detection of open source and internal dependencies. * Improved logging and error handling during Java dependency processing. * Streamlined management and assignment of dependency labels for more precise classification. * Centralized Java source resolution and dependency label initialization within the service client for better maintainability. * Added handling for unidentified embedded JARs by assigning default embedded group labels. * Introduced a new configuration option to disable Maven SHA1 lookups, providing more control over dependency resolution. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Signed-off-by: Emily McMullan <[email protected]> Signed-off-by: Shawn Hurley <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Shawn Hurley <[email protected]> Signed-off-by: Cherry Picker <[email protected]> --------- Signed-off-by: Juan Manuel Leflet Estrada <[email protected]> Signed-off-by: Emily McMullan <[email protected]> Signed-off-by: Shawn Hurley <[email protected]> Signed-off-by: Cherry Picker <[email protected]> Co-authored-by: Emily McMullan <[email protected]> Co-authored-by: Juan Manuel Leflet Estrada <[email protected]> Co-authored-by: Shawn Hurley <[email protected]>
1 participant
API Tests PR: 295
Summary by CodeRabbit
New Features
Bug Fixes
Chores