-
Notifications
You must be signed in to change notification settings - Fork 97
Get the direct dependencies while checking for promotion #952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
eedc65e
1d4ca96
d112421
ea52c38
0398525
4fa1c05
44b2781
c248b7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,13 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Licensed under the MIT license. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as fs from 'fs'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as semver from 'semver'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as glob from 'glob'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { promisify } from 'util'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const globAsync = promisify(glob); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Uri } from 'vscode'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Jdtls } from "../java/jdtls"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { NodeKind, type INodeData } from "../java/nodeData"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { type DependencyCheckItem, type UpgradeIssue, type PackageDescription, UpgradeReason } from "./type"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -11,6 +17,7 @@ import { buildPackageId } from './utility'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import metadataManager from './metadataManager'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { sendInfo } from 'vscode-extension-telemetry-wrapper'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { batchGetCVEIssues } from './cve'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { ContainerPath } from '../views/containerNode'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function packageNodeToDescription(node: INodeData): PackageDescription | null { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const version = node.metaData?.["maven.version"]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -143,62 +150,235 @@ async function getDependencyIssues(dependencies: PackageDescription[]): Promise< | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return issues; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function getProjectIssues(projectNode: INodeData): Promise<UpgradeIssue[]> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const issues: UpgradeIssue[] = []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const dependencies = await getAllDependencies(projectNode); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issues.push(...await getCVEIssues(dependencies)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issues.push(...getJavaIssues(projectNode)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issues.push(...await getDependencyIssues(dependencies)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async function getWorkspaceIssues(projectDeps:{projectNode: INodeData, dependencies: PackageDescription[]}[]): Promise<UpgradeIssue[]> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const issues: UpgradeIssue[] = []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const dependenciesSet: Set<PackageDescription> = new Set(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const { projectNode, dependencies } of projectDeps) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issues.push(...getJavaIssues(projectNode)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dependencies.forEach(dep => dependenciesSet.add(dep)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issues.push(...await getCVEIssues(Array.from(dependenciesSet))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issues.push(...await getDependencyIssues(Array.from(dependenciesSet))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const dependenciesSet: Set<PackageDescription> = new Set(); | |
| for (const { projectNode, dependencies } of projectDeps) { | |
| issues.push(...getJavaIssues(projectNode)); | |
| dependencies.forEach(dep => dependenciesSet.add(dep)); | |
| } | |
| issues.push(...await getCVEIssues(Array.from(dependenciesSet))); | |
| issues.push(...await getDependencyIssues(Array.from(dependenciesSet))); | |
| const dependencyMap: Map<string, PackageDescription> = new Map(); | |
| for (const { projectNode, dependencies } of projectDeps) { | |
| issues.push(...getJavaIssues(projectNode)); | |
| for (const dep of dependencies) { | |
| const key = `${dep.groupId}:${dep.artifactId}:${dep.version ?? ""}`; | |
| if (!dependencyMap.has(key)) { | |
| dependencyMap.set(key, dep); | |
| } | |
| } | |
| } | |
| const uniqueDependencies = Array.from(dependencyMap.values()); | |
| issues.push(...await getCVEIssues(uniqueDependencies)); | |
| issues.push(...await getDependencyIssues(uniqueDependencies)); |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern /<dependencyManagement>[\s\S]*?<\/dependencyManagement>/g uses a non-greedy match, but this can fail with nested structures. If the XML contains CDATA sections, comments with these tags, or multiple dependencyManagement blocks, the regex might remove content incorrectly. Also, the regex won't handle self-closing tags or handle namespace prefixes (e.g., <maven:dependencyManagement>). Consider using a proper XML parser instead of regex-based string manipulation.
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Maven dependency parser doesn't filter out test-scoped dependencies. Test dependencies should typically be excluded from upgrade notifications since they're not part of the production runtime. Consider adding logic to parse and exclude dependencies with <scope>test</scope> or other non-runtime scopes (provided, system, etc.).
| // Match <dependency> blocks and extract groupId and artifactId | |
| const dependencyRegex = /<dependency>\s*<groupId>([^<]+)<\/groupId>\s*<artifactId>([^<]+)<\/artifactId>/g; | |
| let match = dependencyRegex.exec(withoutDepMgmt); | |
| while (match !== null) { | |
| const groupId = match[1].trim(); | |
| const artifactId = match[2].trim(); | |
| // Skip property references like ${project.groupId} | |
| if (!groupId.includes('${') && !artifactId.includes('${')) { | |
| directDeps.add(`${groupId}:${artifactId}`); | |
| } | |
| match = dependencyRegex.exec(withoutDepMgmt); | |
| // Match full <dependency> blocks so we can inspect scope as well as coordinates | |
| const dependencyBlockRegex = /<dependency>([\s\S]*?)<\/dependency>/g; | |
| let blockMatch = dependencyBlockRegex.exec(withoutDepMgmt); | |
| while (blockMatch !== null) { | |
| const dependencyBlock = blockMatch[1]; | |
| const groupIdMatch = /<groupId>\s*([^<]+?)\s*<\/groupId>/.exec(dependencyBlock); | |
| const artifactIdMatch = /<artifactId>\s*([^<]+?)\s*<\/artifactId>/.exec(dependencyBlock); | |
| if (groupIdMatch && artifactIdMatch) { | |
| const groupId = groupIdMatch[1].trim(); | |
| const artifactId = artifactIdMatch[1].trim(); | |
| // Skip property references like ${project.groupId} | |
| if (!groupId.includes('${') && !artifactId.includes('${')) { | |
| // Extract scope if present; default (no scope) is treated as compile/runtime | |
| const scopeMatch = /<scope>\s*([^<]+?)\s*<\/scope>/.exec(dependencyBlock); | |
| const scope = scopeMatch ? scopeMatch[1].trim() : undefined; | |
| const nonRuntimeScopes = new Set(['test', 'provided', 'system']); | |
| if (!scope || !nonRuntimeScopes.has(scope)) { | |
| directDeps.add(`${groupId}:${artifactId}`); | |
| } | |
| } | |
| } | |
| blockMatch = dependencyBlockRegex.exec(withoutDepMgmt); |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern for parsing Maven dependencies has limitations:
- It requires groupId to come before artifactId, but XML allows any order
- It doesn't handle extra whitespace or newlines between elements robustly
- The pattern assumes no whitespace inside tags, but XML parsers would allow
< groupId >
Consider using a proper XML parser (e.g., xml2js or fast-xml-parser) for more reliable parsing of pom.xml files.
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Gradle dependency parser includes test-scoped dependencies (testImplementation, testCompileOnly, testRuntimeOnly) at lines 260 and 272. These test dependencies should typically be excluded from upgrade notifications as they are not part of the production runtime. Consider either filtering out test configurations or making this behavior configurable.
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex patterns for parsing Gradle dependencies have several limitations:
- They don't handle multiline dependency declarations (e.g., when group, name, and version are on separate lines)
- The Kotlin DSL syntax differs from Groovy (uses parentheses and different string syntax)
- Complex scenarios like dependency constraints, platform dependencies, or dependencies with closures will be missed
Consider using Gradle's built-in dependency reporting (e.g., parsing output from gradle dependencies) or a more robust parser.
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check if (!dependencies) at line 343 is incorrect. The expression fulfilled.map(x => x.value).flat() will always return an array (possibly empty), never null or undefined. This condition will never be true. If you want to check for an empty array, use if (dependencies.length === 0) instead.
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch blocks at lines 362-364 silently ignore all errors when parsing build files. This makes debugging difficult when the parsing logic fails. Consider logging errors or sending telemetry with error details to help diagnose why direct dependency parsing might fail in production.
| } catch { | |
| // Ignore errors | |
| } catch (error) { | |
| // Log parsing errors for diagnostics while preserving fallback behavior | |
| let errorName: string | undefined; | |
| let errorMessage: string | undefined; | |
| let errorStack: string | undefined; | |
| if (error instanceof Error) { | |
| errorName = error.name; | |
| errorMessage = error.message; | |
| errorStack = error.stack; | |
| } else if (error !== undefined && error !== null) { | |
| errorMessage = String(error); | |
| } | |
| sendInfo("", { | |
| operationName: "java.dependency.assessmentManager.getDirectDependencies.parseDirectDependencies.error", | |
| buildTool: isMaven ? "maven" : "gradle", | |
| projectUri: projectNode.uri, | |
| errorName, | |
| errorMessage, | |
| // Truncate stack to avoid oversized telemetry; rely on default batching limits. | |
| errorStack, | |
| }); |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback behavior at line 372 returns all dependencies when direct dependency parsing fails, which undermines the primary goal of this PR to exclude transitive dependencies. This could lead to false positives in upgrade notifications. Consider either:
- Returning an empty array when parsing fails (with appropriate telemetry)
- Using a different heuristic to identify direct dependencies
- At minimum, logging a warning that the direct dependency filtering failed so users understand the behavior
| //TODO: fallback to return all dependencies if we cannot parse direct dependencies or just return empty? | |
| return dependencies; | |
| // When we cannot parse direct dependencies, avoid returning transitive dependencies; return empty instead. | |
| return []; |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is redundant. If directDependencyIds is null at line 367, the function returns at line 372, so directDependencyIds cannot be null when reaching this line. Additionally, the check directDependencyIds.size > 0 at line 375 is problematic: if the size is 0 (meaning no dependencies were parsed), the function should either return an empty array or log a warning, not return unfiltered dependencies.
| //TODO: fallback to return all dependencies if we cannot parse direct dependencies or just return empty? | |
| return dependencies; | |
| } | |
| // Filter to only direct dependencies if we have build file info | |
| if (directDependencyIds && directDependencyIds.size > 0) { | |
| dependencies = dependencies.filter(pkg => | |
| directDependencyIds!.has(`${pkg.groupId}:${pkg.artifactId}`) | |
| ); | |
| } | |
| // When we cannot parse direct dependencies, return an empty list instead of all dependencies. | |
| return []; | |
| } | |
| // Filter to only direct dependencies if we have build file info | |
| if (directDependencyIds.size === 0) { | |
| sendInfo("", { | |
| operationName: "java.dependency.assessmentManager.getDirectDependencies.emptyDirectDependencyList" | |
| }); | |
| return []; | |
| } | |
| dependencies = dependencies.filter(pkg => | |
| directDependencyIds.has(`${pkg.groupId}:${pkg.artifactId}`) | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
globpackage is imported at line 6 but it's only listed as a devDependency in package.json (line 1103), not as a regular dependency. This will cause runtime errors in production builds since devDependencies are not included. Moveglobfrom devDependencies to dependencies in package.json, or consider using the built-inglobbypackage which is already listed as a regular dependency (line 1118).