fix: исправление проблемы загрузки через ibcmd #12 #13
Conversation
- Исправлен порядок аргументов команды в выполнении IbcmdPlan - Добавлен префикс 'infobase' для команд конфигурации - Реализован полный IbcmdBuildAction с поддержкой DSL - Добавлен ConnectionStringUtils для корректной обработки путей - Улучшен ProcessExecutor с логированием и таймаутами - Добавлены comprehensive unit и integration тесты - Удален неиспользуемый IbcmdDsl.kt и рефакторинг в IbcmdPlanDsl.kt Closes alkoleft#12
WalkthroughОбновлены документы (новые секции задач/рефлексии по IBCMD). Существенно переработан поток сборки через IBCMD: нормализация строки подключения, новый план/DSL, расширенный контекст, явное формирование команд и выполнение с улучшенным исполнителем процессов и логированием. Добавлены утилиты и тесты; удалён старый IbcmdDsl. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Пользователь
participant Launcher as LauncherService
participant Build as IbcmdBuildAction
participant Plan as IbcmdPlan / IbcmdPlanDsl
participant Ctx as IbcmdContext
participant Proc as ProcessExecutor
participant ibcmd as ibcmd CLI
User->>Launcher: Запуск сборки
Launcher->>Launcher: Нормализация строки подключения (ConnectionStringUtils)
Launcher->>Build: executeBuildDsl(changedSourceSets, normalizedConn)
Build->>Ctx: Настройка dbPath/user/password/data
Build->>Plan: Формирование плана (config/import/apply, extensions)
Plan->>Proc: execute() по командам (с verbose по необходимости)
Proc->>ibcmd: Запуск процесса с аргументами
ibcmd-->>Proc: STDOUT/STDERR, код возврата
Proc-->>Plan: ProcessResult (очистка логов)
Plan-->>Build: Список результатов по пунктам
Build-->>Launcher: BuildResult (аггрегированы успех/ошибки)
Launcher-->>User: Итог статуса
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
@alkoleft, я тебе рассказывал, тут все нейросетка сама накодила. У меня работает, но я хз, что она тут понаделала) |
![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
🧹 Nitpick comments (18)
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/process/ProcessExecutor.kt (1)
483-500: Clarify the autoCleanupLog logic.Логика в строке 490 выглядит правильно: если
autoCleanupLog = trueиlogFilePathне указан, генерируется временный файл. Однако название параметра может вводить в заблуждение - он контролирует не только очистку, но и генерацию временного пути.Рассмотрите возможность переименования параметра для большей ясности:
suspend fun executeWithLogging( commandArgs: List<String>, logFilePath: Path? = null, includeStdout: Boolean = true, verboseLogging: Boolean = false, - autoCleanupLog: Boolean = true, + useTemporaryLogFile: Boolean = true, ): ProcessResult { - val effectiveLogPath = if (autoCleanupLog) logFilePath ?: generateLogFilePath() else logFilePath + val effectiveLogPath = if (useTemporaryLogFile) logFilePath ?: generateLogFilePath() else logFilePath return executeProcess(Или добавьте KDoc, поясняющий, что параметр также управляет генерацией временного файла.
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/PlatformDsl.kt (1)
19-19: Скрыть свойствоcontextили документировать его публичный доступ
Единственное обращение кdsl.contextнаходится вIbcmdBuildAction.kt:144для созданияIbcmdContext. Вместо открытогоval contextсделайте егоprivateи добавьте вPlatformDslметод-фабрику (например,createIbcmdContext()), либо поясните необходимость публичного доступа в KDoc.src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContext.kt (2)
21-22: Уточнить тип свойстваutilityPath.Согласно нашим гайдам, явно указываем типы. Добавьте тип
String.- val utilityPath + val utilityPath: String get() = platformContext.locateUtilitySync(UtilityType.IBCMD).executablePath.toString()
66-94: Мелкая чистка: убрать пустые строки внутри функции.Внутри
buildBaseArgsесть лишние пустые строки; наши гайды просят не оставлять пустые строки в функциях Kotlin.src/test/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContextTest.kt (2)
27-31: Хрупкое мокирование цепочкиexecutablePath.toString().Лучше замокать
executablePathкак Path и не стробитьtoString()по цепочке.- val mockUtil = mockk<io.github.alkoleft.mcp.core.modules.UtilityLocation>(relaxed = true) - every { mockUtil.name } returns "ibcmd" - every { mockUtil.executablePath.toString() } returns "/mock/path/to/ibcmd.exe" + val mockUtil = mockk<io.github.alkoleft.mcp.core.modules.UtilityLocation>() + every { mockUtil.name } returns "ibcmd" + every { mockUtil.executablePath } returns java.nio.file.Paths.get("/mock/path/to/ibcmd.exe")
47-55: Проверьте соседство флагов и значений, а не только наличие.Тесты сейчас проверяют только наличие элементов. Добавьте проверки, что значение следует сразу за соответствующим флагом — это важно для корректного порядка.
Пример:
val idxDb = args.indexOf("--db-path"); assertEquals("/db/path", args.getOrNull(idxDb + 1)) val idxUser = args.indexOf("--user"); assertEquals("testuser", args.getOrNull(idxUser + 1)) val idxPwd = args.indexOf("--password"); assertEquals("testpass", args.getOrNull(idxPwd + 1)) val idxData = args.indexOf("--data"); assertEquals("/temp/data", args.getOrNull(idxData + 1))src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlan.kt (2)
65-74: Надёжнее собирать токены команды и учесть регистр.
split(" ")и сравнениеmode == "config"чувствительны к лишним пробелам/регистру. Предлагаю нормализовать и работать с токенами.- val commandParts = command.commandName.split(" ") - val mode = commandParts.first() - val subCommand = commandParts.drop(1).joinToString(" ") - val fullCommand = - if (mode == "config") { - "infobase config $subCommand" - } else { - command.commandName - } - val commandArgs = listOf(context.utilityPath) + fullCommand.split(" ") + baseArgs + command.arguments + val tokens = command.commandName.trim().split(Regex("\\s+")) + val fullTokens = + if (tokens.firstOrNull()?.equals("config", ignoreCase = true) == true) { + listOf("infobase", "config") + tokens.drop(1) + } else { + tokens + } + val commandArgs = listOf(context.utilityPath) + fullTokens + baseArgs + command.arguments
46-48: Ошибки лучше логировать уровнем error.Сейчас ошибки пишутся через
info. Повысьте уровень для видимости в прод-логах.- logger.info { "Ошибка выполнения команды ${command.commandName}: ${result.error}" } + logger.error { "Ошибка выполнения команды ${command.commandName}: ${result.error}" }src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (2)
76-79: Магическое число 8.Дайте имя константе или используйте
Files.createTempDirectory, тогда подстрока не нужна.
76-79: Дублирование логики сConnectionStringUtils.В PR упомянут новый
ConnectionStringUtilsдля корректной обработки путей. Здесь введён свойextractFilePath. Чтобы избежать расхождений, используйте единую утилиту.Готов заменить вызовы на
ConnectionStringUtils(укажите точные сигнатуры методов), удалить приватный helper и добавить тесты на интеграцию.Also applies to: 127-172
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlanDsl.kt (1)
52-55: Имена методов без глаголов и отсутствуют явные типы возврата.В гайдах рекомендуется начинать имена функций с глагола и указывать тип возврата. Это необязательная правка для DSL, но стоит зафиксировать.
Also applies to: 62-64, 71-73, 80-82, 89-90, 96-97, 103-104, 110-111
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtils.kt (2)
25-29: Используйте isNotEmpty() вместо !isEmpty() для идиоматичного Kotlin кода.Проверка
!isEmpty()не является идиоматичной для Kotlin. Рекомендуется использоватьisNotEmpty()илиisNotBlank()для лучшей читаемости.Примените этот diff для улучшения кода:
return when { - !pathMatch.groupValues[1].isEmpty() -> pathMatch.groupValues[1] // Одинарные кавычки - !pathMatch.groupValues[2].isEmpty() -> pathMatch.groupValues[2] // Двойные кавычки + pathMatch.groupValues[1].isNotEmpty() -> pathMatch.groupValues[1] // Одинарные кавычки + pathMatch.groupValues[2].isNotEmpty() -> pathMatch.groupValues[2] // Двойные кавычки else -> pathMatch.groupValues[3] // Без кавычек }
48-49: Рассмотрите использование Path API для манипуляции путями.Удаление завершающего слэша через строковые операции (
removeSuffix("/")) может быть заменено на использование Path API, что более надёжно и кросс-платформенно.Рассмотрите этот вариант:
} else { // Относительный путь - val baseString = basePath.toString().removeSuffix("/") - Paths.get("$baseString/$rawPath").toAbsolutePath().toString() + basePath.resolve(rawPath).toAbsolutePath().toString() }src/test/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildActionIntegrationTest.kt (3)
24-45: Удалите пустую строку внутри инициализации свойства.Согласно руководству по стилю кода, пустые строки внутри функций и блоков инициализации не рекомендуются. Строка 40 содержит лишнюю пустую строку внутри инициализации
mockProperties.Примените этот diff:
items = emptyList(), ), - )As per coding guidelines
49-50: Рассмотрите использование тестовой конфигурации вместо прямой установки системных свойств.Прямая установка системных свойств в тестовом методе может привести к проблемам при параллельном выполнении тестов и затрудняет поддержку.
Рассмотрите использование
@BeforeEachдля настройки или Spring@TestPropertySource:@TestPropertySource(properties = [ "IBCMD_IB_USR=testuser", "IBCMD_IB_PSW=testpass" ])Или используйте
@BeforeEach:@BeforeEach fun setUp() { System.setProperty("IBCMD_IB_USR", "testuser") System.setProperty("IBCMD_IB_PSW", "testpass") }
80-86: Тест не проверяет фактическую очистку временной директории.Текущий тест только проверяет успешное выполнение, но не верифицирует, что временная директория была действительно очищена после выполнения.
Хотите, чтобы я сгенерировал код для проверки очистки или открыл новый issue для отслеживания этой задачи?
Пример проверки очистки:
@Test fun `temp dir should be cleaned up after execution`() { // Запомнить список файлов до выполнения val filesBefore = tempDir.toFile().listFiles()?.size ?: 0 val action = IbcmdBuildAction(dsl) val result = action.executeBuildDsl(mockProperties, mockSourceSet) assertTrue(result.success) // Проверить, что временные файлы были удалены val filesAfter = tempDir.toFile().listFiles()?.size ?: 0 assertEquals(filesBefore, filesAfter, "Temporary files were not cleaned up") }src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtilsTest.kt (2)
11-12: Удалите неиспользуемое поле tempDir.Поле
tempDirобъявлено с аннотацией@TempDir, но не используется ни в одном из тестов. Согласно руководству по стилю кода, неиспользуемый код следует удалять.Примените этот diff:
class ConnectionStringUtilsTest { - @TempDir - lateinit var tempDir: Path - @TestAs per coding guidelines
47-52: Рассмотрите более точное утверждение вместо startsWith.Использование
assertTrue(result.startsWith(...))менее информативно при падении теста. Рекомендуется использовать более точное утверждение, чтобы при ошибке было понятно, какой путь получился фактически.Рассмотрите использование точного сравнения:
val connectionString = "File=relative/db;Ref=base;" val basePath = Paths.get("/project/base") val result = ConnectionStringUtils.extractDbPath(connectionString, basePath) - assertTrue(result.startsWith("/project/base/relative/db")) + val expectedPath = Paths.get("/project/base/relative/db").toAbsolutePath().toString() + assertEquals(expectedPath, result)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
memory-bank/activeContext.md(1 hunks)memory-bank/progress.md(1 hunks)memory-bank/reflection/reflection-ibcmd-optimization.md(1 hunks)memory-bank/tasks.md(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/services/LauncherService.kt(6 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/PlatformDsl.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/common/PlatformUtilityContext.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContext.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdDsl.kt(0 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlan.kt(4 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlanDsl.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/process/ProcessExecutor.kt(20 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtils.kt(1 hunks)src/test/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildActionIntegrationTest.kt(1 hunks)src/test/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContextTest.kt(1 hunks)src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtilsTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdDsl.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/kotlin.mdc)
**/*.kt: Always declare the type of each variable and function (parameters and return value) in Kotlin; avoid using Any; create necessary types
Don't leave blank lines within a Kotlin function
Use PascalCase for Kotlin classes
Use camelCase for Kotlin variables, functions, and methods
Avoid magic numbers; define constants
Start each function name with a verb
Use verb-based names for boolean variables (e.g., isLoading, hasError, canDelete)
Use complete words instead of abbreviations (except standard ones like API, URL, and i/j, err, ctx, req/res/next)
Write short Kotlin functions with a single purpose (less than 20 instructions)
Name boolean-returning functions as isX/hasX/canX; name void-like functions as executeX/saveX
Avoid nesting by using early returns and extracting utility functions
Use higher-order functions (map, filter, reduce, etc.) to avoid nesting; use expression bodies for simple functions; use named functions for non-simple ones
Use default parameter values instead of checking for null
Reduce function parameters and results using RO-RO (objects for input and output) and declare necessary types
Maintain a single level of abstraction within functions
Use data classes for data representations
Avoid abusing primitive types; encapsulate data in composite types
Avoid data validations in functions; prefer classes with internal validation
Prefer immutability; use val for data that doesn't change
Declare interfaces to define contracts
Write small classes with a single purpose (less than 200 instructions, <10 public methods, <10 properties)
Use exceptions to handle unexpected errors; catch exceptions only to fix expected problems or add context; otherwise use a global handler
Files:
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContext.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/process/ProcessExecutor.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/services/LauncherService.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/PlatformDsl.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlanDsl.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlan.ktsrc/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtilsTest.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtils.ktsrc/test/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContextTest.ktsrc/test/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildActionIntegrationTest.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/common/PlatformUtilityContext.kt
src/{test,androidTest}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/kotlin.mdc)
src/{test,androidTest}/**/*.kt: Follow Arrange-Act-Assert in tests and name test variables as inputX, mockX, actualX, expectedX
Write unit tests for each public function; use test doubles for dependencies (except inexpensive third-party dependencies)
Files:
src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtilsTest.ktsrc/test/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContextTest.ktsrc/test/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildActionIntegrationTest.kt
🧬 Code graph analysis (1)
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (3)
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdContext.kt (4)
data(56-58)dbPath(29-31)user(38-40)password(47-49)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlanDsl.kt (5)
data(52-55)dbPath(62-64)user(71-73)password(80-82)config(89-89)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/commands/common/CommandBuilders.kt (2)
import(126-129)apply(108-108)
🔇 Additional comments (12)
src/main/kotlin/io/github/alkoleft/mcp/application/services/LauncherService.kt (2)
99-104: Нормализация строки подключения выглядит корректно.Использование
ConnectionStringUtils.normalizeForCliдля подготовки строки подключения к CLI-использованию - правильный подход. Метод обрабатывает относительные пути и форматFile=.
155-164: Все вызовы updateIB соответствуют новой сигнатуре.src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/process/ProcessExecutor.kt (2)
262-269: Автоматическая очистка логов реализована корректно.Использование блока
finallyдля удаления временных файлов логов - хороший подход. Обработка исключений при удалении предотвращает подавление основных ошибок.
72-74: Verbose logging отключён по умолчанию
ПараметрverboseLoggingвreadProcessStreamsи во внутренних вызовах по умолчанию равенfalse, и ни в одном из вызововexecuteWithLoggingилиreadProcessStreamsон не передаётся какtrue.src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (1)
90-100: Проверка подключения: для не-File подключенияdbPathможет быть некорректным.Если
connectionStringне видаFile=..., вdbPathуходит вся строка подключения. Уточните, что ibcmd ожидает для серверного подключения (возможно, нужен другой флаг/аргумент).Предлагаю: при отсутствии
File=не задаватьdbPath, а передавать корректный аргумент для серверного подключения (или вообще полагаться на--connection/аналог). Могу предложить правку, если подтвердите целевой формат ibcmd для Srvr/Ref.Also applies to: 136-142
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlanDsl.kt (1)
125-130: Верифицируйте наличие конструкторов без аргументов у билдера.Использование рефлексии требует no-arg конструктора у всех T. Убедитесь, что это так для всех CommandBuilder’ов, иначе DSL упадёт в рантайме.
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtils.kt (3)
59-62: LGTM!Логика добавления кавычек для путей с пробелами или точками с запятой реализована корректно.
72-78: LGTM!Публичный API корректно делегирует работу приватным методам, следуя принципу единой ответственности.
89-98: LGTM!Использование раннего возврата для не-File= строк и делегирование обработки приватным методам делает код чистым и легко читаемым.
src/test/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildActionIntegrationTest.kt (1)
58-60: Приведение типов в forEach указывает на слабую типизацию.Необходимость явного приведения
(shellResult as ShellCommandResult)предполагает, что тип результата недостаточно строго определён. Это может привести к runtime исключениям, если тип окажется другим.Убедитесь, что все элементы в
result.sourceSet.valuesгарантированно являютсяShellCommandResult, или рассмотрите улучшение типизации вsourceSetдля избежания приведения типов.src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/ConnectionStringUtilsTest.kt (2)
70-76: Убедитесь в корректности обработки завершающей точки с запятой.Тест показывает, что входная строка
"File=/path/no spaces;"(с точкой с запятой) нормализуется в"File=/path/no spaces"(без точки с запятой). Убедитесь, что это ожидаемое поведение для использования в CLI.Проверьте, требует ли CLI ibcmd наличие или отсутствие завершающей точки с запятой в параметрах File=. Если точка с запятой требуется, то её следует сохранять или добавлять в
formatFileConnection.
94-101: LGTM!Тест корректно проверяет разрешение относительного пути с пробелами и динамическое формирование ожидаемого результата обеспечивает кросс-платформенную совместимость.
| private fun extractFilePath( | ||
| connectionString: String, | ||
| properties: ApplicationProperties, | ||
| ): String? { | ||
| if (!connectionString.startsWith("File=")) return null | ||
|
|
||
| val pathMatch = | ||
| Regex("""File=(?:'([^']+)'|"([^"]+)"|([^;]+));?""").find(connectionString) | ||
| ?: return null | ||
|
|
||
| val rawPath = | ||
| when { | ||
| !pathMatch.groupValues[1].isEmpty() -> pathMatch.groupValues[1] // Одинарные кавычки | ||
| !pathMatch.groupValues[2].isEmpty() -> pathMatch.groupValues[2] // Двойные кавычки | ||
| else -> pathMatch.groupValues[3] // Без кавычек | ||
| } | ||
|
|
||
| // Если относительный, разрешаем относительно basePath | ||
| val basePath = properties.basePath.toString().removeSuffix("/") | ||
| val resolvedPath = | ||
| if (rawPath.startsWith('/') || rawPath.matches(Regex("""[A-Za-z]:[/\\].*"""))) { | ||
| rawPath | ||
| } else { | ||
| "$basePath/$rawPath" | ||
| } | ||
|
|
||
| return resolvedPath | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Надёжнее разбирать File= и резолвить пути через Paths, а не строковыми операциями.
Текущий код чувствителен к формату (регистр, пробелы) и смешивает разделители. Предлагаю:
- case-insensitive и допускаем пробелы перед
File= - резолв через
Paths/Path.resolve(кроссплатформенно)
- if (!connectionString.startsWith("File=")) return null
+ if (!connectionString.trimStart().startsWith("File=", ignoreCase = true)) return null
- val pathMatch =
- Regex("""File=(?:'([^']+)'|"([^"]+)"|([^;]+));?""").find(connectionString)
+ val pathMatch =
+ Regex("""(?i)File\s*=\s*(?:'([^']+)'|"([^"]+)"|([^;]+))(?:;|$)""").find(connectionString)
?: return null
- val rawPath =
+ val rawPath =
when {
!pathMatch.groupValues[1].isEmpty() -> pathMatch.groupValues[1] // Одинарные кавычки
!pathMatch.groupValues[2].isEmpty() -> pathMatch.groupValues[2] // Двойные кавычки
else -> pathMatch.groupValues[3] // Без кавычек
}
-
- // Если относительный, разрешаем относительно basePath
- val basePath = properties.basePath.toString().removeSuffix("/")
- val resolvedPath =
- if (rawPath.startsWith('/') || rawPath.matches(Regex("""[A-Za-z]:[/\\].*"""))) {
- rawPath
- } else {
- "$basePath/$rawPath"
- }
-
- return resolvedPath
+ val raw = java.nio.file.Paths.get(rawPath)
+ val base = properties.basePath // Path
+ val resolved = if (raw.isAbsolute) raw else base.resolve(raw).normalize()
+ return resolved.toString()📝 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.
| private fun extractFilePath( | |
| connectionString: String, | |
| properties: ApplicationProperties, | |
| ): String? { | |
| if (!connectionString.startsWith("File=")) return null | |
| val pathMatch = | |
| Regex("""File=(?:'([^']+)'|"([^"]+)"|([^;]+));?""").find(connectionString) | |
| ?: return null | |
| val rawPath = | |
| when { | |
| !pathMatch.groupValues[1].isEmpty() -> pathMatch.groupValues[1] // Одинарные кавычки | |
| !pathMatch.groupValues[2].isEmpty() -> pathMatch.groupValues[2] // Двойные кавычки | |
| else -> pathMatch.groupValues[3] // Без кавычек | |
| } | |
| // Если относительный, разрешаем относительно basePath | |
| val basePath = properties.basePath.toString().removeSuffix("/") | |
| val resolvedPath = | |
| if (rawPath.startsWith('/') || rawPath.matches(Regex("""[A-Za-z]:[/\\].*"""))) { | |
| rawPath | |
| } else { | |
| "$basePath/$rawPath" | |
| } | |
| return resolvedPath | |
| } | |
| private fun extractFilePath( | |
| connectionString: String, | |
| properties: ApplicationProperties, | |
| ): String? { | |
| if (!connectionString.trimStart().startsWith("File=", ignoreCase = true)) return null | |
| val pathMatch = | |
| Regex("""(?i)File\s*=\s*(?:'([^']+)'|"([^"]+)"|([^;]+))(?:;|$)""").find(connectionString) | |
| ?: return null | |
| val rawPath = | |
| when { | |
| !pathMatch.groupValues[1].isEmpty() -> pathMatch.groupValues[1] // Одинарные кавычки | |
| !pathMatch.groupValues[2].isEmpty() -> pathMatch.groupValues[2] // Двойные кавычки | |
| else -> pathMatch.groupValues[3] // Без кавычек | |
| } | |
| val raw = java.nio.file.Paths.get(rawPath) | |
| val base = properties.basePath // Path | |
| val resolved = if (raw.isAbsolute) raw else base.resolve(raw).normalize() | |
| return resolved.toString() | |
| } |
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt
around lines 34 to 61, the File= extraction and path resolution rely on fragile
string operations (case- and space-sensitive, manual separators); update the
logic to use a case-insensitive, whitespace-tolerant regex to capture the value
after "File=" (allowing single/double/none quotes and optional trailing
semicolon), trim the captured raw path, then resolve it using
java.nio.file.Path/Paths (Path.of or Paths.get) so that if the captured path is
absolute you return that path.toString(), otherwise resolve it against
properties.basePath with path.resolve(basePath) or basePath.resolve(path) as
appropriate; ensure any surrounding quotes are removed and return the resolved
Path.toString().
| val results = mutableMapOf<String, ShellCommandResult>() | ||
| val errors = mutableListOf<String>() | ||
|
|
||
| // Создание временного каталога данных для операций ibcmd | ||
| val tempDataDir = System.getProperty("java.io.tmpdir") + "mcp-temp-" + UUID.randomUUID().toString().substring(0, 8) | ||
| logger.debug { "Используется временный каталог данных: $tempDataDir" } | ||
|
|
There was a problem hiding this comment.
Создавайте и очищайте временную директорию корректно.
Каталог не создаётся и не очищается. Используйте Files.createTempDirectory и удаление в конце.
- // Создание временного каталога данных для операций ibcmd
- val tempDataDir = System.getProperty("java.io.tmpdir") + "mcp-temp-" + UUID.randomUUID().toString().substring(0, 8)
- logger.debug { "Используется временный каталог данных: $tempDataDir" }
+ // Создание временного каталога данных для операций ibcmd
+ val tempDir = java.nio.file.Files.createTempDirectory("mcp-temp-")
+ val tempDataDir = tempDir.toString()
+ logger.debug { "Используется временный каталог данных: $tempDataDir" }И перед return BuildResult(...):
+ // Очистка временного каталога
+ runCatching { tempDir.toFile().deleteRecursively() }🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt
around lines 78 to 84, the temporary data directory is only constructed as a
path string and never created or cleaned up; replace string concatenation with
Files.createTempDirectory to create a real temp directory (use a suitable
prefix), assign its Path or toFile(), use that path for ibcmd operations, and
ensure you recursively delete the directory (or use Kotlin/Java utilities to
delete tree) in a finally block or just before returning BuildResult(...) so the
temp directory is removed even on error.
| // Построение плана DSL для импорта и применения основной конфигурации | ||
| val plan = | ||
| dsl.ibcmd { | ||
| data(tempDataDir) | ||
| dbPath(dbPathValue) | ||
| user(ibUser) | ||
| password(ibPassword) | ||
| config { | ||
| import(path) | ||
| apply { | ||
| force = true | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Несоответствие типов: IbcmdPlan.execute() возвращает ProcessResult, а карта ожидает ShellCommandResult.
Сейчас в results кладётся procResults[0] несовместимого типа — это не скомпилируется.
Предлагаю маппер и использование его результата:
+import io.github.alkoleft.mcp.infrastructure.platform.dsl.process.ProcessResult
+// ...
+private fun ProcessResult.toShellResult(): ShellCommandResult = ShellCommandResult(
+ success = success,
+ output = output,
+ error = error,
+ exitCode = exitCode,
+ duration = duration,
+)И ниже:
- val procResults = plan.execute()
- if (procResults.isNotEmpty()) {
- results[configItem.name] = procResults[0]
- procResults.forEach { proc ->
+ val procResults = plan.execute()
+ val shellResults = procResults.map { it.toShellResult() }
+ if (shellResults.isNotEmpty()) {
+ results[configItem.name] = shellResults[0]
+ shellResults.forEach { proc ->
if (!proc.success) {
errors.add(proc.error ?: "Неизвестная ошибка импорта/применения основной конфигурации")
}
}
}Also applies to: 116-124
| val procResults = manualPlan.execute() | ||
| if (procResults.isNotEmpty()) { | ||
| results[extItem.name] = procResults[0] // Первый - импорт, но собираем все | ||
| procResults.forEach { proc -> | ||
| if (!proc.success) { | ||
| errors.add(proc.error ?: "Неизвестная ошибка импорта/применения расширения ${extItem.name}") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Та же проблема типов для расширений и сбор ошибок.
Отразите маппинг и здесь.
- val procResults = manualPlan.execute()
- if (procResults.isNotEmpty()) {
- results[extItem.name] = procResults[0] // Первый - импорт, но собираем все
- procResults.forEach { proc ->
+ val procResults = manualPlan.execute()
+ val shellResults = procResults.map { it.toShellResult() }
+ if (shellResults.isNotEmpty()) {
+ results[extItem.name] = shellResults[0] // Первый - импорт, но собираем все
+ shellResults.forEach { proc ->
if (!proc.success) {
errors.add(proc.error ?: "Неизвестная ошибка импорта/применения расширения ${extItem.name}")
}
}
}📝 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.
| val procResults = manualPlan.execute() | |
| if (procResults.isNotEmpty()) { | |
| results[extItem.name] = procResults[0] // Первый - импорт, но собираем все | |
| procResults.forEach { proc -> | |
| if (!proc.success) { | |
| errors.add(proc.error ?: "Неизвестная ошибка импорта/применения расширения ${extItem.name}") | |
| } | |
| } | |
| } | |
| val procResults = manualPlan.execute() | |
| val shellResults = procResults.map { it.toShellResult() } | |
| if (shellResults.isNotEmpty()) { | |
| results[extItem.name] = shellResults[0] // Первый - импорт, но собираем все | |
| shellResults.forEach { proc -> | |
| if (!proc.success) { | |
| errors.add(proc.error ?: "Неизвестная ошибка импорта/применения расширения ${extItem.name}") | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt
around lines 163-171, the code stores procResults[0] directly and aggregates
errors without applying the same mapping used elsewhere for extension results;
update this block to map procResults to the expected extension result type (same
mapper used in other places), assign the mapped first result into
results[extItem.name], and collect/append errors from all procResults using
proc.error ?: "Неизвестная ошибка импорта/применения расширения ${extItem.name}"
so types are consistent and error collection mirrors the rest of the codebase.
| private val properties: ApplicationProperties, | ||
| private val applicationContext: ApplicationContext, | ||
| ) { | ||
| val applicationProperties: ApplicationProperties = properties |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify the necessity of exposing applicationProperties.
Добавление публичного свойства applicationProperties предоставляет прямой доступ к конфигурации приложения. Хотя это может быть удобно для DSL-компонентов, это также может привести к нежелательным зависимостям и нарушению инкапсуляции.
Проверьте, действительно ли требуется прямой доступ к applicationProperties вне PlatformUtilityContext:
Рекомендации:
- Если свойство используется только DSL-компонентами, убедитесь, что это документировано
- Рассмотрите предоставление специфичных методов доступа вместо прямого доступа ко всей конфигурации
- Если публичный доступ необходим, добавьте KDoc, объясняющий назначение и ограничения использования
🏁 Script executed:
#!/bin/bash
# Поиск использования applicationProperties через контекст
rg -nP --type=kt '\.applicationProperties\b' -B 2 -A 1Length of output: 26
Перезапустим поиск использования без указания типа файлов:
🏁 Script executed:
#!/bin/bash
rg -n '\.applicationProperties\b' -C2 --glob '*.kt'Length of output: 0
Ограничьте видимость или задокументируйте applicationProperties
Свойство applicationProperties нигде не используется в кодовой базе. Если оно нужно для внешнего использования в DSL, добавьте KDoc с описанием его назначения; иначе сделайте его private или удалите.
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/common/PlatformUtilityContext.kt
at line 27, the public property `applicationProperties` is declared but not used
anywhere; either reduce its visibility to private (or internal) if it is only
used inside this class, remove it if it's truly unused, or—if it is intended to
be part of the external DSL API—keep it public and add a KDoc explaining its
purpose and usage; update the code accordingly and run the build to ensure no
callers rely on it.
| var dbPath: String? = null | ||
| set(value) { | ||
| field = value | ||
| ibcmdContext.dbPath(value ?: "") | ||
| } |
There was a problem hiding this comment.
Не передавайте в контекст пустые строки при null.
value ?: "" приводит к появлению флагов с пустыми значениями (--db-path, --user, --password). Передавайте в контекст только при ненулевом значении.
- var dbPath: String? = null
- set(value) {
- field = value
- ibcmdContext.dbPath(value ?: "")
- }
+ var dbPath: String? = null
+ set(value) {
+ field = value
+ if (value != null) ibcmdContext.dbPath(value)
+ }
- var user: String? = null
- set(value) {
- field = value
- ibcmdContext.user(value ?: "")
- }
+ var user: String? = null
+ set(value) {
+ field = value
+ if (value != null) ibcmdContext.user(value)
+ }
- var password: String? = null
- set(value) {
- field = value
- ibcmdContext.password(value ?: "")
- }
+ var password: String? = null
+ set(value) {
+ field = value
+ if (value != null) ibcmdContext.password(value)
+ }
- var tempDataPath: String? = null
- set(value) {
- field = value
- ibcmdContext.data(value ?: "")
- }
+ var tempDataPath: String? = null
+ set(value) {
+ field = value
+ if (value != null) ibcmdContext.data(value)
+ }Also applies to: 29-39, 41-45
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/IbcmdPlanDsl.kt
around lines 23-27 (and similarly lines 29-39 and 41-45), the setters call
ibcmdContext.* with value ?: "" which passes empty strings and creates empty
flags; change each setter to only call the corresponding ibcmdContext method
when value is non-null (e.g., guard with if (value != null) or use value?.let {
... }) so you do not pass empty strings for dbPath, user, password (and other
similar properties).
|
@brake71 Не все нравится как она сделала, позже посмотрю. |
2 participants
Closes #12
Summary by CodeRabbit