✨ Get Java deps from pom/structure & disable mvn search option #859
Conversation
WalkthroughThe changes replace Maven SHA1-based dependency identification with a heuristic that infers dependency information from JAR internal structure and a provided dependency label map. The logic for initializing and managing dependency labels is refactored and centralized, with new methods and function signatures introduced for label handling. Two Node.js sample rules are relocated within the YAML demo output file without content changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider
participant ServiceClient
participant Logger
participant Util
Provider->>ServiceClient: initialization(ctx)
Provider->>Util: initOpenSourceDepLabels(Logger, config)
Util-->>Provider: depLabels
Provider->>ServiceClient: SetDepLabels(depLabels)
Provider->>ServiceClient: resolveSourcesJarsForMaven(ctx, fernflower)
ServiceClient->>Util: decompileJava(ctx, Logger, fernflower, archivePath, m2RepoPath, cleanBin, depLabels)
Util->>Util: explode(..., depLabels)
Util->>Util: toDependency(..., Logger, depLabels, jarFile)
Util-->>ServiceClient: (explodedPath, projectPath, err)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: in the java external provider dependency walker (external-providers/java-external-provider/pkg/java_...Applied to files:
🧬 Code Graph Analysis (1)external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (27)
✨ 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. 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 (
|
eemcmullan
added
the
cherry-pick/release-0.7
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
636-636: Empty error message for missing JAVA8_HOMEThe function returns an error with an empty message when JAVA8_HOME is not set, making debugging difficult.
- return fmt.Errorf("") + return fmt.Errorf("JAVA8_HOME environment variable not set, required for gradle")
🧹 Nitpick comments (4)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
356-364: Consider thread safety for the SetDepLabels method.The implementation logic is correct and follows a good pattern for initialization/merging. However, consider whether this method needs synchronization using the existing
depsMutexif it could be called concurrently with other dependency operations.If thread safety is needed, consider this approach:
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 } } }external-providers/java-external-provider/pkg/java_external_provider/util.go (3)
421-421: Lower log level for default caseThe default case uses
log.Infowhich might be too verbose for normal operation. Consider using a lower log level.- log.Info("default case", "file", explodedFilePath) + log.V(8).Info("processing non-jar file", "file", explodedFilePath)
575-602: Consider caching depToLabels regex matchingThe
constructArtifactFromStructurefunction performs regex matching in a loop which could be expensive for large dependency label maps. Consider pre-compiling or caching the regex patterns.Consider optimizing the regex matching:
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 javaArtifact{}, err } artifact := javaArtifact{GroupId: groupId} + // Pre-compile regex patterns if needed + var groupIdPattern *depLabelItem sgmts := strings.Split(groupId, ".") for len(sgmts) > 0 { groupIdRegex := strings.Join([]string{groupId, "*"}, ".") - if depToLabels[groupIdRegex] != nil { + if groupIdPattern = depToLabels[groupIdRegex]; groupIdPattern != nil { log.V(10).Info(fmt.Sprintf("%s is a public dependency", jarFile)) artifact.foundOnline = true return artifact, nil } else { 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)) return artifact, nil }
683-683: Remove deprecated rand.Seed usageThe
rand.Seedfunction is deprecated in newer Go versions. Consider usingrand.Newwith a source instead.func RandomName() string { - rand.Seed(int64(time.Now().Nanosecond())) + r := rand.New(rand.NewSource(time.Now().UnixNano())) charset := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" b := make([]byte, 16) for i := range b { - b[i] = charset[rand.Intn(len(charset))] + b[i] = charset[r.Intn(len(charset))] } return string(b) }
📜 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(6 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(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (8)
demo-output.yaml (1)
605-650: Node sample rules are still present – contradicts PR summaryThe PR summary says the two Node.js sample rules (
node-sample-rule-001,node-sample-rule-002) were removed, yet they are still fully present here (lines 605-650 and following).
Please either:
- Delete these blocks if the intent was to drop the rules, or
- Fix the summary / commit-message so it accurately reflects the change set.
Leaving this mismatch will confuse reviewers and future readers.
Likely an incorrect or invalid review comment.
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (2)
959-961: LGTM! Error handling improvedThe changes properly return the dependency label items and error, improving error propagation.
972-975: Type assertion without checking could panicThe type assertion on line 972 doesn't check if the value is actually a slice of strings, which could cause a 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{}: + for _, item := range v { + if str, ok := item.(string); ok { + excludePackages = append(excludePackages, str) + } else { + return nil, fmt.Errorf("%s config must contain only strings", providerSpecificConfigExcludePackagesKey) + } + } + 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.
external-providers/java-external-provider/pkg/java_external_provider/provider.go (3)
349-354: LGTM! Early initialization improves error handlingMoving the open source dependency labels initialization early in the Init method ensures proper error handling before creating expensive resources.
684-684: Pass dependency labels to decompile functionThe decompile function call correctly passes the
s.depToLabelsparameter, ensuring proper dependency classification during decompilation.
868-868: Consistent use of dependency labels in decompilationThe Maven source resolution correctly passes dependency labels to the decompile function, maintaining consistency with the Gradle implementation.
external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
58-58: LGTM! Well-defined constant for embedded dependenciesThe constant provides a clear namespace for dependencies that cannot be identified through normal means.
519-531: LGTM! Improved dependency resolution logicThe refactored
toDependencyfunction properly attempts POM-based resolution first, then falls back to structure-based inference with appropriate logging.
external-providers/java-external-provider/pkg/java_external_provider/dependency.go
Outdated
Show resolved
Hide resolved
| func initOpenSourceDepLabels(log logr.Logger, providerSpecificConfig map[string]interface{}) (map[string]*depLabelItem, error) { | ||
| var ok bool | ||
| var v interface{} | ||
| if v, ok = p.config.ProviderSpecificConfig[providerSpecificConfigOpenSourceDepListKey]; !ok { | ||
| p.log.V(7).Info("Did not find open source dep list.") | ||
| return nil | ||
| if v, ok = providerSpecificConfig[providerSpecificConfigOpenSourceDepListKey]; !ok { | ||
| log.V(7).Info("Did not find open source dep list.") | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Inconsistent nil return on success
The function returns nil when no open source dependency list is found (line 936), which is inconsistent with the expected return type of map[string]*depLabelItem. This could cause nil pointer dereferences in callers.
- return nil, nil
+ return map[string]*depLabelItem{}, 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.
| func initOpenSourceDepLabels(log logr.Logger, providerSpecificConfig map[string]interface{}) (map[string]*depLabelItem, error) { | |
| var ok bool | |
| var v interface{} | |
| if v, ok = p.config.ProviderSpecificConfig[providerSpecificConfigOpenSourceDepListKey]; !ok { | |
| p.log.V(7).Info("Did not find open source dep list.") | |
| return nil | |
| if v, ok = providerSpecificConfig[providerSpecificConfigOpenSourceDepListKey]; !ok { | |
| log.V(7).Info("Did not find open source dep list.") | |
| return nil, nil | |
| } | |
| func initOpenSourceDepLabels(log logr.Logger, providerSpecificConfig map[string]interface{}) (map[string]*depLabelItem, error) { | |
| var ok bool | |
| var v interface{} | |
| if v, ok = providerSpecificConfig[providerSpecificConfigOpenSourceDepListKey]; !ok { | |
| log.V(7).Info("Did not find open source dep list.") | |
| return map[string]*depLabelItem{}, nil | |
| } | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In
external-providers/java-external-provider/pkg/java_external_provider/dependency.go
around lines 931 to 937, the function initOpenSourceDepLabels returns nil for
the map when the open source dependency list key is not found, which can cause
nil pointer dereferences. Modify the return statement to return an empty map
instead of nil to maintain consistent return types and avoid potential runtime
errors.
external-providers/java-external-provider/pkg/java_external_provider/dependency.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/util.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/util.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (2)
942-948: Potential nil pointer dereference issue persistsThe function still returns
nil, nilwhen the open source dependency list key is not found (line 947), which was previously flagged as potentially causing nil pointer dereferences in callers expecting a map.Consider returning an empty map instead:
- return nil, nil + return map[string]*depLabelItem{}, nil
976-992: Potential nil map access persistsThe function could receive a nil
depToLabelsparameter but doesn't check for it before passing toloadDepLabelItems. This was previously flagged and the issue remains.Consider adding nil check:
func initExcludeDepLabels(log logr.Logger, providerSpecificConfig map[string]interface{}, depToLabels map[string]*depLabelItem) (map[string]*depLabelItem, error) { + if depToLabels == nil { + depToLabels = make(map[string]*depLabelItem) + } var ok bool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/golang-dependency-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
Dockerfile(1 hunks)Makefile(1 hunks)external-providers/dotnet-external-provider/Dockerfile(1 hunks)external-providers/dotnet-external-provider/go.mod(1 hunks)external-providers/generic-external-provider/Dockerfile(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/golang-dependency-provider/Dockerfile(1 hunks)external-providers/golang-dependency-provider/go.mod(1 hunks)external-providers/java-external-provider/Dockerfile(1 hunks)external-providers/java-external-provider/go.mod(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency.go(10 hunks)external-providers/yq-external-provider/Dockerfile(1 hunks)external-providers/yq-external-provider/go.mod(3 hunks)go.mod(3 hunks)
✅ Files skipped from review due to trivial changes (10)
- external-providers/java-external-provider/Dockerfile
- external-providers/golang-dependency-provider/Dockerfile
- external-providers/generic-external-provider/Dockerfile
- external-providers/dotnet-external-provider/go.mod
- external-providers/yq-external-provider/Dockerfile
- Makefile
- external-providers/generic-external-provider/go.mod
- go.mod
- Dockerfile
- external-providers/dotnet-external-provider/Dockerfile
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
Learnt from: pranavgaikwad
PR: #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.
external-providers/golang-dependency-provider/go.mod (3)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Learnt from: pranavgaikwad
PR: #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.
external-providers/java-external-provider/go.mod (3)
Learnt from: pranavgaikwad
PR: #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.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
external-providers/yq-external-provider/go.mod (3)
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The go directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate toolchain directive (introduced in Go 1.21), e.g., toolchain go1.23.11.
Learnt from: shawn-hurley
PR: #863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Learnt from: pranavgaikwad
PR: #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.
🧬 Code Graph Analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (2)
engine/labels/labels.go (1)
AsString(35-40)provider/provider.go (2)
DepSourceLabel(32-32)DepExcludeLabel(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (10)
external-providers/golang-dependency-provider/go.mod (1)
37-37: Confirm thereplacepointing back to the repo root is intentional
replace github.com/konveyor/analyzer-lsp => ../../is useful for local multi-module development but will leak into downstream consumers if committed.
Please verify that this replace is required long-term; otherwise consider gating it via build tags or removing before publishing.external-providers/java-external-provider/go.mod (1)
10-12: Version drift across OpenTelemetry / gRPC / Protobuf packages – verify compatibilityAll OpenTelemetry libs are now at 1.34.0, Jaeger exporter at 1.17.0, gRPC at 1.72.2, genproto dated 2025-02-18, protobuf at 1.36.5.
Minor/patch mismatches are usually fine, but this combo sits close to several breaking changes (e.g.,protoc-gen-gov1.36 tightened API surface).Please run
go test ./...after ago mod tidyand smoke-test the external provider to ensure:• No duplicate proto package symbols
• Noundefined: codes.ResourceExhausted‐style errors at runtime
• Traces are exported correctly through OTLP / Jaeger pipelinesIf confident, ignore; otherwise align the trio (grpc / genproto / protobuf) to the same timestamped release.
Also applies to: 25-27, 41-42, 46-46
external-providers/yq-external-provider/go.mod (2)
43-43: Localreplacemust be removed before publishingThe relative replace path binds the module to a local checkout and breaks consumers pulling from
proxy.golang.org.
Confirm this is only for iterative development and will be dropped (or guarded by a build flag) before merging to the main branch.
3-3:godirective uses unsupported patch-level syntax
go 1.23.9will makego modcommands fail because thegodirective only acceptsmajor.minor.
Specify the patch with a separatetoolchaindirective (Go ≥1.21) and keep thegoline atmajor.minor.-go 1.23.9 +go 1.23 + +toolchain go1.23.9⛔ Skipped due to learnings
Learnt from: shawn-hurley PR: konveyor/analyzer-lsp#863 File: external-providers/generic-external-provider/go.mod:3-3 Timestamp: 2025-07-25T02:05:02.884Z Learning: The `go` directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate `toolchain` directive (introduced in Go 1.21), e.g., `toolchain go1.23.11`.Learnt from: shawn-hurley PR: konveyor/analyzer-lsp#863 File: external-providers/generic-external-provider/go.mod:3-3 Timestamp: 2025-07-25T02:05:02.884Z Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.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.external-providers/java-external-provider/pkg/java_external_provider/dependency.go (6)
22-22: LGTM - Import addition for loggingThe
logr.Loggerimport is appropriately added to support the enhanced logging functionality throughout the dependency handling code.
663-663: LGTM - Logger field addition to walker structAdding the
logr.Loggerfield to the walker struct enables consistent logging throughout the dependency discovery process, which aligns with the broader logging improvements in this refactoring.Also applies to: 675-675
694-694: Function signature updated with enhanced parametersThe
toDependencyfunction call now includes logger and dependency labels parameters, enabling improved artifact construction and classification. The ignored error aligns with previous maintainer decisions.
698-698: LGTM - Enhanced addDepLabels function callsThe
addDepLabelsfunction calls are correctly updated to include the newopenSourceparameter, enabling more precise dependency classification based on whether the artifact was found online or determined to be open source through other means.Also applies to: 731-731, 816-816
868-897: LGTM - Enhanced dependency label classification logicThe
addDepLabelsfunction has been improved with:
- Clear documentation of the
openSourceparameter purpose- Proper handling of label conflicts between open source and internal classifications
- Logical flow that prioritizes open source determination over default internal classification
The implementation correctly removes conflicting internal labels when open source status is determined.
996-1024: LGTM - Well-designed loadDepLabelItems refactoringThe function has been properly refactored with:
- Updated signature to accept existing
depToLabelsparameter- Proper nil handling and map initialization (lines 997-1000)
- Consistent map operations throughout the function
- Appropriate return of the populated map
The implementation correctly handles both new map creation and extending existing maps.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
533-538: Error handling for exclude labels initialization should be reconsidered.The error from
initExcludeDepLabelsis only logged but not propagated. If exclude labels are critical for proper dependency filtering, this could lead to unexpected behavior.external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
213-221: Fix logic error in removeIncompleteDependenciesThe condition on line 216 appears to have inverted logic - it's keeping dependencies that have only ArtifactId but no GroupId or Version, which seems incorrect based on the function name.
415-434: Improve error handling when copying jars to m2 repoThe code creates destination directories but doesn't check if the parent directories were created successfully before copying files. Also, there's a typo in "gropupPath" on line 427.
🧹 Nitpick comments (1)
demo-output.yaml (1)
605-650: Add language tags & remove noisy duplicates
- Neither
node-sample-rule-001nornode-sample-rule-002carry alabels:block, whereas similar rules elsewhere tag their language/tech (e.g.tag=Java,tag=Language=Golang).
Add something like:labels: - tag=Language=TypeScript - tag=Runtime=Node.jsto keep taxonomy consistent.
- Each rule lists two identical incidents for the same file & snippet (
test_b.tsin rule 001,test_a.tsin rule 002). Unless you are intentionally stress-testing deduplication, drop the second copy to reduce noise and effort scoring drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sumexternal-providers/golang-dependency-provider/go.sumis excluded by!**/*.sumexternal-providers/java-external-provider/go.sumis excluded by!**/*.sumexternal-providers/yq-external-provider/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
Dockerfile(1 hunks)Makefile(1 hunks)demo-output.yaml(1 hunks)external-providers/dotnet-external-provider/Dockerfile(1 hunks)external-providers/dotnet-external-provider/go.mod(1 hunks)external-providers/generic-external-provider/Dockerfile(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/golang-dependency-provider/Dockerfile(1 hunks)external-providers/golang-dependency-provider/go.mod(1 hunks)external-providers/java-external-provider/Dockerfile(1 hunks)external-providers/java-external-provider/go.mod(3 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(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(16 hunks)external-providers/yq-external-provider/Dockerfile(1 hunks)external-providers/yq-external-provider/go.mod(3 hunks)go.mod(3 hunks)
✅ Files skipped from review due to trivial changes (6)
- external-providers/java-external-provider/Dockerfile
- Makefile
- Dockerfile
- external-providers/generic-external-provider/Dockerfile
- external-providers/yq-external-provider/Dockerfile
- external-providers/dotnet-external-provider/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (9)
- external-providers/golang-dependency-provider/go.mod
- external-providers/golang-dependency-provider/Dockerfile
- external-providers/dotnet-external-provider/go.mod
- external-providers/java-external-provider/pkg/java_external_provider/service_client.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency.go
- external-providers/yq-external-provider/go.mod
- external-providers/java-external-provider/go.mod
- go.mod
- external-providers/generic-external-provider/go.mod
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
Learnt from: pranavgaikwad
PR: #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.
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
Learnt from: pranavgaikwad
PR: #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.
🧬 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 (3)
ClassFile(36-36)WebArchive(34-34)JavaArchive(33-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (6)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (2)
349-354: Early dependency label initialization looks good.The early initialization of
openSourceDepLabelswith proper error handling and context cancellation follows good practices. The error is appropriately propagated to prevent initialization with invalid state.
585-700: Source resolution methods successfully refactored to use service client state.The conversion of
resolveSourcesJarsForGradleandresolveSourcesJarsForMaveninto service client methods properly utilizes the client's internal state (logger, config, dependency labels). The methods are well-structured and maintain consistent error handling patterns.external-providers/java-external-provider/pkg/java_external_provider/util.go (4)
58-58: Well-defined constant for embedded dependencies.The
EMBEDDED_KONVEYOR_GROUPconstant provides a clear namespace for fallback dependencies that can't be identified through normal means.
89-89: Function signature updated to support dependency label tracking.The addition of
depLabelsparameter to thedecompilefunction properly integrates with the centralized dependency label management system.
534-546: Good refactoring of dependency resolution with proper fallback logic.The
toDependencyfunction now appropriately tries POM-based resolution first, then falls back to structure-based inference. The error handling and logging provide good visibility into the resolution process.
590-683: Well-implemented heuristic-based dependency inference.The
constructArtifactFromStructureandinferGroupNamefunctions provide a robust fallback mechanism for identifying dependencies when POM metadata is unavailable. The logic for finding common package prefixes in class files is sound and properly handles edge cases.
| - uri: file:///examples/nodejs/test_b.ts | ||
| message: nodejs sample rule 001 | ||
| codeSnip: " 1 import { greeter } from './test_a';\n 2 \n 3 console.log(greeter.hello());\n" | ||
| lineNumber: 0 | ||
| variables: | ||
| file: file:///examples/nodejs/test_b.ts |
There was a problem hiding this comment.
lineNumber: 0 is invalid – use a 1-based line index
YAML consumers (and the UI that renders incidents) expect lineNumber to be ≥ 1.
Keeping 0 breaks jump-to-line navigation and will be filtered out by some downstream tooling.
- lineNumber: 0
+ lineNumber: 1 # or the real first line you want to flag📝 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.
| - uri: file:///examples/nodejs/test_b.ts | |
| message: nodejs sample rule 001 | |
| codeSnip: " 1 import { greeter } from './test_a';\n 2 \n 3 console.log(greeter.hello());\n" | |
| 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';\n 2 \n 3 console.log(greeter.hello());\n" | |
| - lineNumber: 0 | |
| + lineNumber: 1 # or the real first line you want to flag | |
| variables: | |
| file: file:///examples/nodejs/test_b.ts |
🤖 Prompt for AI Agents
In demo-output.yaml around lines 615 to 620, the lineNumber field is set to 0,
which is invalid because line numbers should be 1-based. Update the lineNumber
value to a valid 1-based index (e.g., 1 or higher) to ensure proper navigation
and compatibility with downstream tools.
There was a problem hiding this comment.
while this somewhat makes sense, not an issue I am worried about right now. going to resolve this (talking about bot's comment #859 (comment))
There was a problem hiding this comment.
@pranavgaikwad Understood! You're absolutely right to prioritize based on your current needs. Thanks for taking the time to review it - feel free to revisit this if it becomes relevant for your workflow later on.
🐰
╱|、
(˚ˎ 。7
|、˜〵
じしˍ,)ノ
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]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
external-providers/java-external-provider/pkg/java_external_provider/util.go (2)
213-221: Function logic is now correct.The
removeIncompleteDependenciesfunction correctly filters out dependencies missing any of the required fields (ArtifactId, GroupId, Version). This addresses the previously identified logic error.
415-434: Address typo and missing error handling.Two issues identified in previous review:
- Line 427: "gropupPath" should be "groupPath"
- Missing directory creation before file copy operations
Apply the suggested fixes from the previous review to ensure proper error handling and correct the spelling error.
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
590-618: Consider populating artifact fields when dependency is identified as public.The function correctly identifies public dependencies but only sets
foundOnline=true. Consider also populating theGroupIdandVersionfields when a match is found indepLabelsto create more complete artifacts.if depLabels[groupIdRegex] != nil { log.V(10).Info(fmt.Sprintf("%s is a public dependency", jarFile)) artifact.foundOnline = true + artifact.GroupId = groupId + // Consider extracting version from depLabels if available return artifact, nil
📜 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)
external-providers/java-external-provider/pkg/java_external_provider/util.go (1)
Learnt from: pranavgaikwad
PR: #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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (7)
external-providers/java-external-provider/pkg/java_external_provider/util.go (7)
12-12: LGTM! Import and constant additions support new functionality.The added
pathimport andEMBEDDED_KONVEYOR_GROUPconstant are appropriately used in the new dependency inference logic.Also applies to: 58-59
89-89: LGTM! Function signature updates enable dependency inference.The added
depLabelsandlogr.Loggerparameters are consistently applied across related functions and support the new dependency classification logic.Also applies to: 164-164, 226-226
258-258: Good naming improvement for clarity.Renaming
filePathtoexplodedFilePaththroughout the function improves code readability by clearly indicating the nature of the path variable.Also applies to: 262-262, 268-268, 270-270, 275-275, 279-279, 304-304, 321-321, 343-343, 350-350, 352-352, 358-358, 360-360, 366-366, 368-368
534-546: Excellent improvement to dependency resolution.The new approach using POM properties first, then falling back to structure inference, is more robust than the previous SHA1-based lookup and reduces external dependencies.
548-588: LGTM! Logger addition improves observability.The core POM extraction logic remains sound, and the added logging will help with debugging dependency resolution issues.
620-683: Excellent implementation of group name inference.The function uses a sophisticated approach to extract group names from JAR package structure. The filtering logic appropriately handles edge cases (inner classes, metadata paths), and the longest common prefix algorithm is well-suited for finding package roots.
134-134: LGTM! Function calls properly updated for new signatures.All function calls have been consistently updated to pass the required
depLabelsparameter and use the newremoveIncompleteDependenciesfunction appropriately.Also applies to: 177-177, 183-183, 190-190
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
Signed-off-by: Emily McMullan <[email protected]>
* The labeling of deps will happen in a different part of the code * Setting Online means that we don't try and decompile the source at the correct time. Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
eemcmullan
changed the title
eemcmullan
changed the title
| - uri: file:///examples/nodejs/test_b.ts | ||
| message: nodejs sample rule 001 | ||
| codeSnip: " 1 import { greeter } from './test_a';\n 2 \n 3 console.log(greeter.hello());\n" | ||
| lineNumber: 0 | ||
| variables: | ||
| file: file:///examples/nodejs/test_b.ts |
There was a problem hiding this comment.
while this somewhat makes sense, not an issue I am worried about right now. going to resolve this (talking about bot's comment #859 (comment))
| } | ||
|
|
||
| } else { | ||
| log.Info("failed to add dependency to list of depdencies - using file to create dummy values", "file", explodedFilePath) |
There was a problem hiding this comment.
this part is genius!
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]>
4 participants
Extends #852
Summary by CodeRabbit