🐛 [IGNORE] Test PR #879
🐛 [IGNORE] Test PR #879
Conversation
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Emily McMullan <[email protected]>
Signed-off-by: Emily McMullan <[email protected]>
…m structure Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Emily McMullan <[email protected]>
WalkthroughAdds logging and open-source/internal dependency labeling throughout the Java external provider, refactors dependency discovery to infer artifacts from local JARs (removing remote Maven lookups), converts Maven/Gradle resolvers into javaServiceClient methods, adds SetDepLabels, and includes temporary demo YAML edits that were later reverted. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Provider Init
participant Labels as Dep Label Init
participant Client as javaServiceClient
participant Maven as Maven Resolver
participant Gradle as Gradle Resolver
participant Decomp as Decompile/Explode
Init->>Labels: initOpenSourceDepLabels()
Labels-->>Init: depToLabels or error
Init->>Client: SetDepLabels(depToLabels)
Init->>Labels: initExcludeDepLabels(depToLabels)
Labels-->>Init: excludeLabels (logged on error)
Init->>Client: SetDepLabels(excludeLabels)
Init->>Maven: s.resolveSourcesJarsForMaven(fernflower)
Init->>Gradle: s.resolveSourcesJarsForGradle(fernflower)
Init->>Decomp: decompileJava(..., depToLabels)
sequenceDiagram
participant JAR as JAR
participant toDep as toDependency(log, depLabels)
participant Pom as constructArtifactFromPom
participant Struct as constructArtifactFromStructure
participant Label as addDepLabels(openSource?)
participant Project as createJavaProject
JAR->>toDep: resolve dependency
toDep->>Pom: try pom.properties inside jar
Pom-->>toDep: artifact or nil
alt Pom not found
toDep->>Struct: infer from classes/package paths
Struct-->>toDep: inferred artifact (may be embedded)
end
toDep->>Label: apply open-source/internal labels
toDep-->>Project: dependency artifact (or synthetic embedded artifact)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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/dependency.go (1)
870-900: Fix mutual-exclusivity bug in addDepLabels (open-source vs internal labels)s is built from m before m is modified, so deleting the internal label from m never removes it from the returned slice — the function can return both internal and open-source labels. Build the slice after you update m (or update m instead of s).
- Location: external-providers/java-external-provider/pkg/java_external_provider/dependency.go (func addDepLabels, ~lines 870–900)
- Tests: I couldn't find unit tests for this function; please add tests covering combinations (openSource true/false, existing labels present/absent).
Suggested change:
func addDepLabels(depToLabels map[string]*depLabelItem, depName string, openSource bool) []string { m := map[string]interface{}{} for _, d := range depToLabels { if d.r.Match([]byte(depName)) { for label := range d.labels { m[label] = nil } } } - s := []string{} - for k := range m { - s = append(s, k) - } // if open source label is not found and we don't know if it's open source yet, qualify the dep as being internal by default _, openSourceLabelFound := m[labels.AsString(provider.DepSourceLabel, javaDepSourceOpenSource)] _, internalSourceLabelFound := m[labels.AsString(provider.DepSourceLabel, javaDepSourceInternal)] if openSourceLabelFound || openSource { if !openSourceLabelFound { - s = append(s, labels.AsString(provider.DepSourceLabel, javaDepSourceOpenSource)) + m[labels.AsString(provider.DepSourceLabel, javaDepSourceOpenSource)] = nil } if internalSourceLabelFound { delete(m, labels.AsString(provider.DepSourceLabel, javaDepSourceInternal)) } } else { if !internalSourceLabelFound { - s = append(s, labels.AsString(provider.DepSourceLabel, javaDepSourceInternal)) + m[labels.AsString(provider.DepSourceLabel, javaDepSourceInternal)] = nil } } + s := []string{} + for k := range m { + s = append(s, k) + } s = append(s, labels.AsString(provider.DepLanguageLabel, "java")) return s }
🧹 Nitpick comments (2)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
427-428: Fix typo in variable name.There's a typo in the variable name "gropupPath" which should be "groupPath".
-gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) -destPath := filepath.Join(m2Repo, gropupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version)) +groupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) +destPath := filepath.Join(m2Repo, groupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version))external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
349-354: Consider making dependency labeling initialization non-fatal.The initialization fails completely if open-source dependency labels cannot be loaded. This seems overly strict, especially since the exclude labels initialization (lines 552-557) only logs errors without failing.
Consider making this consistent with the exclude labels handling:
openSourceDepLabels, err := initOpenSourceDepLabels(log, config.ProviderSpecificConfig) if err != nil { - log.V(5).Error(err, "failed to initialize dep labels lookup for open source packages") - cancelFunc() - return nil, provider.InitConfig{}, err + log.Error(err, "failed to initialize dep labels lookup for open source packages, continuing without open source labeling") + openSourceDepLabels = map[string]*depLabelItem{} }This would allow the provider to continue functioning even if the dependency labeling configuration has issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo-output.yaml(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency.go(10 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(14 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(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/dependency.goexternal-providers/java-external-provider/pkg/java_external_provider/util.goexternal-providers/java-external-provider/pkg/java_external_provider/provider.go
🧬 Code Graph Analysis (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (3)
event/export/tag.go (1)
Labels(20-37)engine/labels/labels.go (1)
AsString(35-40)provider/provider.go (2)
DepSourceLabel(32-32)DepExcludeLabel(34-34)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (3)
ClassFile(36-36)WebArchive(34-34)JavaArchive(33-33)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (2)
provider/provider.go (2)
InitConfig(120-151)Location(310-313)tracing/trace.go (1)
StartNewSpan(65-69)
🔇 Additional comments (2)
demo-output.yaml (1)
613-658: Inconsistency between summary and actual content.The AI-generated summary indicates that node-sample-rule-001 and node-sample-rule-002 blocks were added and then removed entirely, resulting in no net change. However, the actual code shows these blocks are present in the final file (lines 613-658). This appears to be an inconsistency in the summary rather than the code itself.
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
983-986: Type assertion could panic if configuration is incorrect.The type assertion on line 983 doesn't have proper error handling. If the configuration value is not a slice of strings, this will panic.
- excludePackages, ok := v.([]string) - if !ok { - return nil, fmt.Errorf("%s config must be a list of packages to exclude", providerSpecificConfigExcludePackagesKey) + var excludePackages []string + switch v := v.(type) { + case []string: + excludePackages = v + case []interface{}: + // Handle case where YAML/JSON unmarshaling creates []interface{} + for _, item := range v { + if str, ok := item.(string); ok { + excludePackages = append(excludePackages, str) + } else { + return nil, fmt.Errorf("%s config contains non-string value: %v", providerSpecificConfigExcludePackagesKey, item) + } + } + default: + return nil, fmt.Errorf("%s config must be a list of packages to exclude, got %T", providerSpecificConfigExcludePackagesKey, v) }Likely an incorrect or invalid review comment.
| excludeDepLabels, err := initExcludeDepLabels(svcClient.log, svcClient.config.ProviderSpecificConfig, openSourceDepLabels) | ||
| if err != nil { | ||
| cancelFunc() | ||
| return nil, provider.InitConfig{}, err | ||
| log.Error(err, "error initializing labels for excluding dependencies") | ||
| } else { | ||
| svcClient.SetDepLabels(excludeDepLabels) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling between label initializations.
The error handling for initExcludeDepLabels differs from initOpenSourceDepLabels. The former logs errors and continues, while the latter fails the entire initialization. This inconsistency could be confusing.
Either both should fail initialization or both should log and continue. For consistency and robustness, consider:
excludeDepLabels, err := initExcludeDepLabels(svcClient.log, svcClient.config.ProviderSpecificConfig, openSourceDepLabels)
if err != nil {
- log.Error(err, "error initializing labels for excluding dependencies")
-} else {
- svcClient.SetDepLabels(excludeDepLabels)
+ log.Error(err, "error initializing labels for excluding dependencies, continuing without exclude labels")
+ excludeDepLabels = openSourceDepLabels
}
+svcClient.SetDepLabels(excludeDepLabels)📝 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.
| excludeDepLabels, err := initExcludeDepLabels(svcClient.log, svcClient.config.ProviderSpecificConfig, openSourceDepLabels) | |
| if err != nil { | |
| cancelFunc() | |
| return nil, provider.InitConfig{}, err | |
| log.Error(err, "error initializing labels for excluding dependencies") | |
| } else { | |
| svcClient.SetDepLabels(excludeDepLabels) | |
| } | |
| excludeDepLabels, err := initExcludeDepLabels(svcClient.log, svcClient.config.ProviderSpecificConfig, openSourceDepLabels) | |
| if err != nil { | |
| log.Error(err, "error initializing labels for excluding dependencies, continuing without exclude labels") | |
| excludeDepLabels = openSourceDepLabels | |
| } | |
| svcClient.SetDepLabels(excludeDepLabels) |
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/provider.go
around lines 552 to 557, the initExcludeDepLabels call currently logs errors and
continues while initOpenSourceDepLabels fails initialization on error; make them
consistent by treating initExcludeDepLabels failures the same as
initOpenSourceDepLabels: if initExcludeDepLabels returns an error, log the error
with context and return that error to abort initialization (or propagate a
wrapped error) instead of merely logging and continuing, and keep
svcClient.SetDepLabels only in the success path.
| func (p *javaServiceClient) SetDepLabels(depLabels map[string]*depLabelItem) { | ||
| if p.depToLabels == nil { | ||
| p.depToLabels = depLabels | ||
| } else { | ||
| for k, v := range depLabels { | ||
| p.depToLabels[k] = v | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Thread safety concern in SetDepLabels method.
The SetDepLabels method modifies the depToLabels map without synchronization. Since this map is accessed from multiple goroutines (e.g., in GetAllSymbols and dependency resolution methods), this could lead to race conditions.
Consider adding mutex protection:
func (p *javaServiceClient) SetDepLabels(depLabels map[string]*depLabelItem) {
+ p.depsMutex.Lock()
+ defer p.depsMutex.Unlock()
if p.depToLabels == nil {
p.depToLabels = depLabels
} else {
for k, v := range depLabels {
p.depToLabels[k] = v
}
}
}Alternatively, consider using a dedicated mutex for depToLabels if depsMutex is meant only for dependency caching.
📝 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 (p *javaServiceClient) SetDepLabels(depLabels map[string]*depLabelItem) { | |
| if p.depToLabels == nil { | |
| p.depToLabels = depLabels | |
| } else { | |
| for k, v := range depLabels { | |
| p.depToLabels[k] = v | |
| } | |
| } | |
| } | |
| func (p *javaServiceClient) SetDepLabels(depLabels map[string]*depLabelItem) { | |
| p.depsMutex.Lock() | |
| defer p.depsMutex.Unlock() | |
| if p.depToLabels == nil { | |
| p.depToLabels = depLabels | |
| } else { | |
| for k, v := range depLabels { | |
| p.depToLabels[k] = v | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
around lines 356 to 364, SetDepLabels mutates the shared depToLabels map without
synchronization; add mutex protection around both writes and reads to avoid
races — either use the existing depsMutex if it is intended to protect
depToLabels, or add a new mutex field (e.g., depLabelsMutex) on
javaServiceClient and wrap the assignment/merge loop with Lock/Unlock (and
ensure all readers like GetAllSymbols and dependency resolution methods use
RLock/RUnlock accordingly).
| log.Info("failed to add dependency to list of depdencies - using file to create dummy values", "file", explodedFilePath) | ||
| name, _ := strings.CutSuffix(filepath.Base(explodedFilePath), ".jar") | ||
| newDep := javaArtifact{ | ||
| foundOnline: false, | ||
| packaging: "", | ||
| GroupId: EMBEDDED_KONVEYOR_GROUP, | ||
| ArtifactId: name, | ||
| Version: "0.0.0-SNAPSHOT", | ||
| sha1: "", | ||
| } | ||
| dependencies = append(dependencies, newDep) | ||
| gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) | ||
| destPath := filepath.Join(m2Repo, gropupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version)) | ||
| if err := CopyFile(explodedFilePath, destPath); err != nil { | ||
| log.V(8).Error(err, "failed copying jar to m2 local repo") | ||
| } else { | ||
| log.V(8).Info("copied jar file", "src", explodedFilePath, "dest", destPath) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Embedded dependency version should be deterministic.
When creating dummy dependencies for unresolved JARs, the version is hardcoded as "0.0.0-SNAPSHOT". This could lead to issues if multiple JARs with the same base name but different actual versions exist, as they would all resolve to the same path in the local repository.
Consider using a hash of the JAR content to create a unique version:
+import "crypto/sha256"
+
log.Info("failed to add dependency to list of depdencies - using file to create dummy values", "file", explodedFilePath)
name, _ := strings.CutSuffix(filepath.Base(explodedFilePath), ".jar")
+
+// Generate a deterministic version based on JAR content
+file, err := os.Open(explodedFilePath)
+if err == nil {
+ defer file.Close()
+ h := sha256.New()
+ if _, err := io.Copy(h, file); err == nil {
+ version := fmt.Sprintf("0.0.0-%x", h.Sum(nil)[:8])
+ newDep := javaArtifact{
+ foundOnline: false,
+ packaging: "",
+ GroupId: EMBEDDED_KONVEYOR_GROUP,
+ ArtifactId: name,
+ Version: version,
+ sha1: "",
+ }
+ dependencies = append(dependencies, newDep)
+ gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...)
+ destPath := filepath.Join(m2Repo, gropupPath, name, version, fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version))
+ // ... rest of the code
+ }
+}📝 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.
| log.Info("failed to add dependency to list of depdencies - using file to create dummy values", "file", explodedFilePath) | |
| name, _ := strings.CutSuffix(filepath.Base(explodedFilePath), ".jar") | |
| newDep := javaArtifact{ | |
| foundOnline: false, | |
| packaging: "", | |
| GroupId: EMBEDDED_KONVEYOR_GROUP, | |
| ArtifactId: name, | |
| Version: "0.0.0-SNAPSHOT", | |
| sha1: "", | |
| } | |
| dependencies = append(dependencies, newDep) | |
| gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) | |
| destPath := filepath.Join(m2Repo, gropupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version)) | |
| if err := CopyFile(explodedFilePath, destPath); err != nil { | |
| log.V(8).Error(err, "failed copying jar to m2 local repo") | |
| } else { | |
| log.V(8).Info("copied jar file", "src", explodedFilePath, "dest", destPath) | |
| } | |
| log.Info("failed to add dependency to list of depdencies - using file to create dummy values", "file", explodedFilePath) | |
| name, _ := strings.CutSuffix(filepath.Base(explodedFilePath), ".jar") | |
| // Generate a deterministic version based on JAR content (fall back to snapshot on error) | |
| version := "0.0.0-SNAPSHOT" | |
| if file, err := os.Open(explodedFilePath); err == nil { | |
| defer file.Close() | |
| h := sha256.New() | |
| if _, err := io.Copy(h, file); err == nil { | |
| version = fmt.Sprintf("0.0.0-%x", h.Sum(nil)[:8]) | |
| } else { | |
| log.V(8).Error(err, "failed hashing jar file - using fallback version", "file", explodedFilePath) | |
| } | |
| } else { | |
| log.V(8).Error(err, "failed opening jar for hashing - using fallback version", "file", explodedFilePath) | |
| } | |
| newDep := javaArtifact{ | |
| foundOnline: false, | |
| packaging: "", | |
| GroupId: EMBEDDED_KONVEYOR_GROUP, | |
| ArtifactId: name, | |
| Version: version, | |
| sha1: "", | |
| } | |
| dependencies = append(dependencies, newDep) | |
| gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) | |
| destPath := filepath.Join(m2Repo, gropupPath, name, version, fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version)) | |
| if err := CopyFile(explodedFilePath, destPath); err != nil { | |
| log.V(8).Error(err, "failed copying jar to m2 local repo") | |
| } else { | |
| log.V(8).Info("copied jar file", "src", explodedFilePath, "dest", destPath) | |
| } |
| // constructArtifactFromStructure builds an artifact object out of the JAR internal structure. | ||
| 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) | ||
| if err != nil { | ||
| return dep, err | ||
| return javaArtifact{}, err | ||
| } | ||
| // since the extracted groupId is not reliable, lets just name the dependency after its filename | ||
| artifact := javaArtifact{ArtifactId: filepath.Base(jarFile)} | ||
| // 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, ".") | ||
| 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)) | ||
| 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, "*"}, ".") | ||
| } | ||
| } | ||
| defer file.Close() | ||
| log.V(10).Info(fmt.Sprintf("could not decide whether %s is public, setting as private", jarFile)) | ||
| return artifact, nil | ||
| } |
There was a problem hiding this comment.
Group ID inference might be too permissive.
The constructArtifactFromStructure function checks for public dependencies by progressively removing segments from the inferred group ID. This could lead to false positives - for example, if "com.*" is in the public list, it would match any company's internal packages starting with "com."
The current implementation allows overly broad matching. Consider requiring more specific patterns or a minimum number of segments:
sgmts := strings.Split(groupId, ".")
+// Require at least 2 segments to remain for a valid match
-for len(sgmts) > 0 {
+for len(sgmts) > 1 {
// 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))
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, "*"}, ".")
}
}Also consider logging when patterns match with very few segments remaining, as this might indicate overly broad patterns in the configuration.
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
416-433: Embedded dependency version should be deterministic.Using a fixed version "0.0.0-SNAPSHOT" could lead to conflicts when multiple JARs with the same base name but different content exist, as they would all resolve to the same path in the local repository.
Consider using a hash of the JAR content to create a unique version:
+import "crypto/sha256" + log.Info("failed to add dependency to list of depdencies - using file to create dummy values", "file", explodedFilePath) name, _ := strings.CutSuffix(filepath.Base(explodedFilePath), ".jar") + +// Generate a deterministic version based on JAR content +file, err := os.Open(explodedFilePath) +if err == nil { + defer file.Close() + h := sha256.New() + if _, err := io.Copy(h, file); err == nil { + version := fmt.Sprintf("0.0.0-%x", h.Sum(nil)[:8]) + newDep := javaArtifact{ + foundOnline: false, + packaging: "", + GroupId: EMBEDDED_KONVEYOR_GROUP, + ArtifactId: name, + Version: version, + sha1: "", + } + dependencies = append(dependencies, newDep) + gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) + destPath := filepath.Join(m2Repo, gropupPath, name, version, fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version)) + // ... rest of the code + } +}
590-618: Group ID inference might be too permissive.The function progressively removes segments from the inferred group ID, which could lead to false positives if patterns like "com.*" are in the public list.
Consider requiring a minimum number of segments:
sgmts := strings.Split(groupId, ".") +// Require at least 2 segments to remain for a valid match -for len(sgmts) > 0 { +for len(sgmts) > 1 { // 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)) 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, "*"}, ".") } }
🧹 Nitpick comments (2)
external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
58-58: DefineEMBEDDED_KONVEYOR_GROUPusing idiomatic Go constant style.Consider using camelCase for constant naming to align with Go conventions.
-const EMBEDDED_KONVEYOR_GROUP = "io.konveyor.embeddedep" +const embeddedKonveyorGroup = "io.konveyor.embeddedep"Note: If this constant is exported for use in other packages, then the current naming convention is appropriate.
699-699: Replace deprecatedrand.Seedwith modern approach.The
rand.Seedfunction is deprecated in Go 1.20+. The random number generator is automatically seeded.-rand.Seed(int64(time.Now().Nanosecond())) charset := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"If you need deterministic behavior for testing, consider using
rand.New(rand.NewSource(...))instead.
📜 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(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/util.go
🔇 Additional comments (3)
external-providers/java-external-provider/pkg/java_external_provider/util.go (3)
183-183: Good practice: Filtering incomplete dependencies before project creation.The addition of
removeIncompleteDependenciesensures that only complete dependencies are included in the generated pom.xml, which prevents Maven build failures.
534-546: Appropriate fallback strategy for dependency resolution.The implementation correctly attempts to construct artifacts from pom.properties first, then falls back to structure-based inference. This aligns with the learning that errors from toDependency should be ignored as they're not considered important by maintainers.
620-683: Well-implemented group name inference from JAR structure.The function correctly infers the group ID by finding the longest common prefix among package paths, properly filtering out inner classes, metadata paths, and handling edge cases.
| sha1: "", | ||
| } | ||
| dependencies = append(dependencies, newDep) | ||
| gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) |
There was a problem hiding this comment.
Fix typo in variable name.
The variable name has a typo: "gropup" should be "group".
-gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...)
+groupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...)Also update the usage on line 428:
-destPath := filepath.Join(m2Repo, gropupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version))
+destPath := filepath.Join(m2Repo, groupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version))📝 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.
| gropupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) | |
| groupPath := filepath.Join(strings.Split(EMBEDDED_KONVEYOR_GROUP, ".")...) | |
| destPath := filepath.Join(m2Repo, groupPath, name, "0.0.0-SNAPSHOT", fmt.Sprintf("%s-%s.jar", newDep.ArtifactId, newDep.Version)) |
🤖 Prompt for AI Agents
In external-providers/java-external-provider/pkg/java_external_provider/util.go
around line 427, the variable name gropupPath is misspelled; rename it to
groupPath and update any subsequent references (including the usage on line 428)
to use groupPath instead; search nearby lines for other occurrences of the typo
and correct them to keep the identifier consistent.
2 participants
API tests PR: 321
Summary by CodeRabbit
New Features
Refactor
Revert