Skip to content

fix(amazonq): fix didChangeDependencyPathsParams with absolute pathing #5492

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 21, 2025
Merged

fix(amazonq): fix didChangeDependencyPathsParams with absolute pathing #5492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65d274c
update dependency pathing
samgst-amazon Mar 20, 2025
e48b163
Merge branch 'feature/q-lsp' into samgst/q-lsp-change-dependency-paths
samgst-amazon Mar 21, 2025
52a9581
detekt
samgst-amazon Mar 21, 2025
7e39617
prefix and suffix changes for messages
samgst-amazon Mar 21, 2025
e4976fd
Merge branch 'feature/q-lsp' into samgst/q-lsp-change-dependency-paths
samgst-amazon Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ...ware/aws/toolkits/jetbrains/services/amazonq/lsp/dependencies/ModuleDependencyProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ package software.aws.toolkits.jetbrains.services.amazonq.lsp.dependencies

import com.intellij.openapi.extensions.ExtensionPointName
import com.intellij.openapi.module.Module
import com.intellij.openapi.roots.ModuleRootManager
import com.intellij.openapi.vfs.VirtualFile
import software.aws.toolkits.jetbrains.services.amazonq.lsp.model.aws.dependencies.DidChangeDependencyPathsParams
import software.aws.toolkits.jetbrains.services.amazonq.lsp.util.FileUriUtil.toUriString

interface ModuleDependencyProvider {
companion object {
Expand All @@ -14,4 +17,9 @@ interface ModuleDependencyProvider {

fun isApplicable(module: Module): Boolean
fun createParams(module: Module): DidChangeDependencyPathsParams

fun getWorkspaceFolderPath(module: Module): String {
val contentRoots: Array<VirtualFile> = ModuleRootManager.getInstance(module).contentRoots
return contentRoots.firstOrNull()?.let { toUriString(it) }.orEmpty()
}
}
21 changes: 5 additions & 16 deletions ...its/jetbrains/services/amazonq/lsp/dependencies/providers/JavaModuleDependencyProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import com.intellij.openapi.module.Module
import com.intellij.openapi.projectRoots.JavaSdkType
import com.intellij.openapi.roots.ModuleRootManager
import com.intellij.openapi.roots.OrderRootType
import com.intellij.openapi.vfs.VfsUtil
import software.aws.toolkits.jetbrains.services.amazonq.lsp.dependencies.ModuleDependencyProvider
import software.aws.toolkits.jetbrains.services.amazonq.lsp.model.aws.dependencies.DidChangeDependencyPathsParams

Expand All @@ -16,31 +15,21 @@ internal class JavaModuleDependencyProvider : ModuleDependencyProvider {
ModuleRootManager.getInstance(module).sdk?.sdkType is JavaSdkType

override fun createParams(module: Module): DidChangeDependencyPathsParams {
val sourceRoots = getSourceRoots(module)
val dependencies = mutableListOf<String>()

ModuleRootManager.getInstance(module).orderEntries().forEachLibrary { library ->
library.getUrls(OrderRootType.CLASSES).forEach { url ->
dependencies.add(VfsUtil.urlToPath(url))
library.getFiles(OrderRootType.CLASSES).forEach { file ->
dependencies.add(file.path.removeSuffix("!/"))
Copy link
Contributor

@rli rli Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this suffix come from?

Copy link
Contributor Author

@samgst-amazon samgst-amazon Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signifies jar as root archive path, was told this was not needed for flare purposes

}
true
}

return DidChangeDependencyPathsParams(
moduleName = module.name,
programmingLanguage = "Java",
files = sourceRoots,
dirs = dependencies,
moduleName = getWorkspaceFolderPath(module),
programmingLanguage = "java",
paths = dependencies,
includePatterns = emptyList(),
excludePatterns = emptyList()
)
}

private fun getSourceRoots(module: Module): List<String> =
ModuleRootManager.getInstance(module).contentEntries
.flatMap { contentEntry ->
contentEntry.sourceFolders
.filter { !it.isTestSource }
.mapNotNull { it.file?.path }
}
}
28 changes: 11 additions & 17 deletions ...s/jetbrains/services/amazonq/lsp/dependencies/providers/PythonModuleDependencyProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package software.aws.toolkits.jetbrains.services.amazonq.lsp.dependencies.providers

import com.intellij.openapi.module.Module
import com.intellij.openapi.roots.ModuleRootManager
import com.jetbrains.python.packaging.management.PythonPackageManager
import com.jetbrains.python.sdk.PythonSdkUtil
import software.aws.toolkits.jetbrains.services.amazonq.lsp.dependencies.ModuleDependencyProvider
Expand All @@ -15,31 +14,26 @@ internal class PythonModuleDependencyProvider : ModuleDependencyProvider {
PythonSdkUtil.findPythonSdk(module) != null

override fun createParams(module: Module): DidChangeDependencyPathsParams {
val sourceRoots = getSourceRoots(module)
val dependencies = mutableListOf<String>()

PythonSdkUtil.findPythonSdk(module)?.let { sdk ->
val packageManager = PythonPackageManager.forSdk(module.project, sdk)
packageManager.installedPackages.forEach { pkg ->
dependencies.add(pkg.name)
PythonSdkUtil.getSitePackagesDirectory(sdk)?.let { sitePackagesDir ->
val packageManager = PythonPackageManager.forSdk(module.project, sdk)
packageManager.installedPackages.forEach { pkg ->
Copy link
Contributor Author

@samgst-amazon samgst-amazon Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not certain this is the proper way to get Python packages -- need to revisit with better test setup

Copy link
Contributor

@rli rli Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be correct

val packageDir = sitePackagesDir.findChild(pkg.name)
if (packageDir != null) {
dependencies.add(packageDir.path)
}
}
}
}

return DidChangeDependencyPathsParams(
moduleName = module.name,
programmingLanguage = "Python",
files = sourceRoots,
dirs = dependencies,
moduleName = getWorkspaceFolderPath(module),
programmingLanguage = "python",
paths = dependencies,
includePatterns = emptyList(),
excludePatterns = emptyList()
)
}

private fun getSourceRoots(module: Module): List<String> =
ModuleRootManager.getInstance(module).contentEntries
.flatMap { contentEntry ->
contentEntry.sourceFolders
.filter { !it.isTestSource }
.mapNotNull { it.file?.path }
}
}
3 changes: 1 addition & 2 deletions ...s/jetbrains/services/amazonq/lsp/model/aws/dependencies/DidChangeDependencyPathsParams.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ package software.aws.toolkits.jetbrains.services.amazonq.lsp.model.aws.dependenc
class DidChangeDependencyPathsParams(
val moduleName: String,
val programmingLanguage: String,
val files: List<String>,
val dirs: List<String>,
val paths: List<String>,
val includePatterns: List<String>,
val excludePatterns: List<String>,
)
52 changes: 10 additions & 42 deletions ...lkits/jetbrains/services/amazonq/lsp/dependencies/DefaultModuleDependenciesServiceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,13 @@ class DefaultModuleDependenciesServiceTest {
val module = mockk<Module>()
val params = DidChangeDependencyPathsParams(
moduleName = "testModule",
programmingLanguage = "Java",
files = listOf("src/main"),
dirs = listOf("lib"),
programmingLanguage = "java",
paths = listOf("/path/to/dependency.jar"),
includePatterns = emptyList(),
excludePatterns = emptyList()
)

every { mockModuleManager.modules } returns arrayOf(module)

prepDependencyProvider(listOf(Pair(module, params)))

sut = DefaultModuleDependenciesService(project, mockk())
Expand All @@ -114,17 +112,15 @@ class DefaultModuleDependenciesServiceTest {
val module2 = mockk<Module>()
val params1 = DidChangeDependencyPathsParams(
moduleName = "module1",
programmingLanguage = "Java",
files = listOf("src/main1"),
dirs = listOf("lib1"),
programmingLanguage = "java",
paths = listOf("/path/to/dependency1.jar"),
includePatterns = emptyList(),
excludePatterns = emptyList()
)
val params2 = DidChangeDependencyPathsParams(
moduleName = "module2",
programmingLanguage = "Java",
files = listOf("src/main2"),
dirs = listOf("lib2"),
programmingLanguage = "python",
paths = listOf("/path/to/site-packages/package1"),
includePatterns = emptyList(),
excludePatterns = emptyList()
)
Expand All @@ -138,41 +134,18 @@ class DefaultModuleDependenciesServiceTest {

sut = DefaultModuleDependenciesService(project, mockk())

// Verify both modules were synced
verify { mockLanguageServer.didChangeDependencyPaths(params1) }
verify { mockLanguageServer.didChangeDependencyPaths(params2) }
}

@Test
fun `test rootsChanged with non-applicable module`() {
// Arrange
val module = mockk<Module>()

every { mockModuleManager.modules } returns arrayOf(module)

every {
EP_NAME.forEachExtensionSafe(any<Consumer<ModuleDependencyProvider>>())
} answers {
val consumer = firstArg<Consumer<ModuleDependencyProvider>>()
every { mockDependencyProvider.isApplicable(any()) } returns false
consumer.accept(mockDependencyProvider)
}

sut = DefaultModuleDependenciesService(project, mockk())

// Verify no sync occurred
verify(exactly = 0) { mockLanguageServer.didChangeDependencyPaths(any()) }
}

@Test
fun `test rootsChanged withFileTypesChange`() {
// Arrange
val module = mockk<Module>()
val params = DidChangeDependencyPathsParams(
moduleName = "testModule",
programmingLanguage = "Java",
files = listOf("src/main"),
dirs = listOf("lib"),
programmingLanguage = "java",
paths = listOf("/path/to/dependency.jar"),
includePatterns = emptyList(),
excludePatterns = emptyList()
)
Expand All @@ -182,10 +155,8 @@ class DefaultModuleDependenciesServiceTest {

sut = DefaultModuleDependenciesService(project, mockk())

// Act
sut.rootsChanged(event)

// Verify sync occurred once - once on init and rootsChange ignores
verify(exactly = 1) { mockLanguageServer.didChangeDependencyPaths(params) }
}

Expand All @@ -195,9 +166,8 @@ class DefaultModuleDependenciesServiceTest {
val module = mockk<Module>()
val params = DidChangeDependencyPathsParams(
moduleName = "testModule",
programmingLanguage = "Java",
files = listOf("src/main"),
dirs = listOf("lib"),
programmingLanguage = "java",
paths = listOf("/path/to/dependency.jar"),
includePatterns = emptyList(),
excludePatterns = emptyList()
)
Expand All @@ -210,10 +180,8 @@ class DefaultModuleDependenciesServiceTest {

sut = DefaultModuleDependenciesService(project, mockk())

// Act
sut.rootsChanged(event)

// Verify sync occurred twice - once on init and once after rootsChanged
verify(exactly = 2) { mockLanguageServer.didChangeDependencyPaths(params) }
}

Expand Down
Loading