feat: add partial configuration loading support #26
Conversation
- Add PartialLoadListGenerator for generating file list with BSL->XML mapping - Extend BuildAction interface with runPartial() method - Implement partial loading in AbstractBuildAction with automatic mode selection - Add loadConfigurationPartial() to DesignerBuildAction using -partial -listFile - Add loadConfigurationPartial() to IbcmdBuildAction using config import files --partial - Integrate partial loading in LauncherService - Add configurable threshold via app.build.partial-load-threshold (default: 20) - Improve MapDB error handling to avoid log pollution - Add unit tests for PartialLoadListGenerator Closes: partial load feature inspired by https://github.com/Nikolay-Shirokov/partial-load-config
WalkthroughДобавлена поддержка частичной загрузки конфигураций при сборке: введена новая утилита Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Области, требующие особого внимания:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/storage/MapDbHashStorage.kt (2)
112-115: Нефатальные ошибки batchUpdate выглядят согласованно с идеей «кэш можно перестроить»Логирование
warnс количеством файлов + подробности вdebugи безопасный rollback дают понятное поведение: сборка не падает, а кэш будет пересчитан при следующем запуске. Это выглядит разумным компромиссом для ускорения сборок, когда MapDB ведёт себя нестабильно.В качестве необязательного улучшения можно сразу логировать исключение на уровне
warn, чтобы стек не терялся в проде, гдеdebugобычно отключён:- } catch (e: Exception) { - logger.warn { "Не удалось сохранить хеши файлов (${updates.size} шт.) - кэш будет перестроен при следующей сборке" } - logger.debug(e) { "Детали ошибки сохранения хешей" } - tryRollback() - } + } catch (e: Exception) { + logger.warn(e) { + "Не удалось сохранить хеши файлов (${updates.size} шт.) - кэш будет перестроен при следующей сборке" + } + tryRollback() + }
141-155: Единообразная обработка ошибок и отдельный tryRollback — хорошо; можно чуть доработать сигнатуру и логПоведение
storeTimestampтеперь симметричноbatchUpdate: при ошибке пишетсяwarnи выполняется безопасный rollback через отдельный helper — это делает фейлы MapDB нефатальными и предсказуемыми для вызывающего кода.Для нового
tryRollbackможно слегка уточнить типизацию и лог, чтобы следовать гайдам и не терять стек-трейс:- private fun tryRollback() { - try { - db.rollback() - } catch (e: Exception) { - logger.debug { "Rollback не выполнен: ${e.message}" } - } - } + private fun tryRollback(): Unit { + try { + db.rollback() + } catch (e: Exception) { + logger.debug(e) { "Rollback не выполнен" } + } + }Так мы явно указываем возвращаемый тип
Unitи логируем исключение вместе со стеком. As per coding guidelines, ...src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.kt (2)
136-148: Рассмотрите упрощение через when-expressionМожно упростить логику, используя when-expression с присваиванием:
- when { - absolutePath.extension.equals("bsl", ignoreCase = true) -> { + when (absolutePath.extension.lowercase()) { + "bsl" -> { processBslFile(sourceSetPath, relativePath, filesToLoad) } - absolutePath.extension.equals("xml", ignoreCase = true) -> { + "xml" -> { filesToLoad.add(relativePathStr) logger.trace { "Добавлен XML: $relativePathStr" } } else -> { filesToLoad.add(relativePathStr) logger.trace { "Добавлен файл: $relativePathStr" } } }
256-268: Оптимизируйте проверку Configuration.xmlТекущая реализация всегда вычисляет
relativePathдаже для файлов внеsourceSetPath. Можно оптимизировать:fun hasConfigurationXmlChanges( sourceSetPath: Path, changedFiles: Set<Path>, ): Boolean { - return changedFiles.any { file -> - val relativePath = if (file.startsWith(sourceSetPath)) { - sourceSetPath.relativize(file) - } else { - file - } - relativePath.name.equals("Configuration.xml", ignoreCase = true) - } + return changedFiles.any { file -> + file.name.equals("Configuration.xml", ignoreCase = true) && + file.startsWith(sourceSetPath) + } }src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/DesignerBuildAction.kt (1)
85-99: Дублирование логики между loadConfigurationPartial() и loadExtensionPartial()Методы различаются только вызовом
extension(name). Рассмотрите извлечение общей логики.+private fun loadPartial( + name: String, + path: Path, + changedFiles: Set<Path>, + isExtension: Boolean, +): ProcessResult { + val listFile = partialLoadListGenerator.generate(path, changedFiles) + return actionDsl.loadConfigFromFiles { + fromPath(path) + if (isExtension) extension(name) + partial() + listFile(listFile) + updateConfigDumpInfo() + updateDBCfg() + } +} + override fun loadConfigurationPartial( name: String, path: Path, changedFiles: Set<Path>, -): ProcessResult { - val listFile = partialLoadListGenerator.generate(path, changedFiles) - return actionDsl.loadConfigFromFiles { - fromPath(path) - partial() - listFile(listFile) - updateConfigDumpInfo() - updateDBCfg() - } -} +): ProcessResult = loadPartial(name, path, changedFiles, false) override fun loadExtensionPartial( name: String, path: Path, changedFiles: Set<Path>, -): ProcessResult { - val listFile = partialLoadListGenerator.generate(path, changedFiles) - return actionDsl.loadConfigFromFiles { - fromPath(path) - extension(name) - partial() - listFile(listFile) - updateConfigDumpInfo() - updateDBCfg() - } -} +): ProcessResult = loadPartial(name, path, changedFiles, true)src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (1)
59-75: Дублирование кода между методами частичной загрузкиАналогично DesignerBuildAction, здесь также наблюдается дублирование логики между
loadConfigurationPartial()иloadExtensionPartial().+private fun loadPartial( + name: String, + path: Path, + extensionName: String? = null, +): ProcessResult { + lateinit var result: ProcessResult + actionDsl.config { + result = import(path) { + extensionName?.let { extension = it } + importSubCommand = "files" + partial = true + baseDir = path.toString() + } + } + return result +} + override fun loadConfigurationPartial( name: String, path: Path, changedFiles: Set<Path>, -): ProcessResult { - // Для ibcmd используем подкоманду "files" и параметр --partial - // ibcmd infobase config import files --partial <path> - lateinit var result: ProcessResult - actionDsl.config { - result = import(path) { - importSubCommand = "files" - partial = true - baseDir = path.toString() - } - } - return result -} +): ProcessResult = loadPartial(name, path) override fun loadExtensionPartial( name: String, path: Path, changedFiles: Set<Path>, -): ProcessResult { - // Для ibcmd используем подкоманду "files" и параметр --partial с расширением - lateinit var result: ProcessResult - actionDsl.config { - result = import(path) { - extension = name - importSubCommand = "files" - partial = true - baseDir = path.toString() - } - } - return result -} +): ProcessResult = loadPartial(name, path, name)Also applies to: 91-107
src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGeneratorTest.kt (1)
33-284: Несоответствия локальным правилам код-стайла для тестов (типы, имена, пустые строки)В текущем виде класс тестов не полностью следует выданным правилам:
- Локальные переменные и параметры функций не имеют явных типов (
val sourceSetPath = ...,val changedFiles = ...и т.п.), хотя по гайдам типы должны быть задекларированы явно.- Внутри тестовых функций довольно много пустых строк между выражениями, что противоречит правилу «Don’t leave blank lines within a Kotlin function».
- Имена тестовых переменных (
sourceSetPath,changedFiles,contentи т.д.) не следуют схемеinputX/mockX/actualX/expectedX, рекомендованной для файлов подsrc/test/**.Функционально это не мешает, но для выравнивания со стилем проекта можно постепенно привести новые тесты к этим правилам (например, начиная с явных типов в ключевых местах и более строгого AAA‑разделения без лишних пустых строк).
As per coding guidelines, ...
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/AbstractBuildAction.kt (2)
145-158: Проверить соответствие ключей вsourceSetChangesи то, что вchangedFilesпопадают только файлы конкретного source setВ
executeBuildDslPartialвы берёте изменения по имени конфигурации/расширения:val changes = sourceSetChanges[configuration.name] // / extension.name ... shouldUsePartialLoad(changes, threshold, configPath/extensionPath)и далее пробрасываете
changes.changedFilesвPartialLoadListGenerator. Для корректной работы важно, чтобы:
- карта
sourceSetChangesдействительно формировалась с ключамиconfiguration.name/extension.name, а не, например, по путям;- в каждом
SourceSetChanges.changedFilesсодержались только файлы, относящиеся к соответствующей конфигурации или расширению (иначе порог и проверкаConfiguration.xmlбудут считаться по «лишним» файлам).Если это уже гарантируется логикой построения
sourceSetChanges, всё ок; если нет — имеет смысл либо фильтровать список поsourceSet.basePath.resolve(...).normalize(), либо документировать допущение.Also applies to: 165-183, 194-207
73-84: Отклонения от локальных Kotlin-правил (явные типы локальных переменных, пустые строки внутри функций)Во вновь добавленных/изменённых методах (
measureExecutionTime,executeBuildDsl,executeBuildDslPartialи т.п.) есть несколько расхождений с правилами:
- Локальные переменные объявлены без явных типов (
val startTime = ...,val state = BuildActionState(),val threshold = properties.build.partialLoadThreshold,val configPath = ...,val changes = ...и т.д.), хотя по гайдам «Always declare the type of each variable».- Внутри функций довольно много пустых строк между логическими блоками (включая новые участки кода), что формально противоречит правилу «Don't leave blank lines within a Kotlin function».
Это не влияет на поведение и читаемость здесь в целом нормальная, но для единообразия со стилем проекта можно в будущем при правках этого файла постепенно добавлять явные типы локальных переменных и немного уплотнить функции, убирая лишние пустые строки.
As per coding guidelines, ...
Also applies to: 90-129, 134-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/AbstractBuildAction.kt(5 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/DesignerBuildAction.kt(4 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt(4 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/actions/common/Actions.kt(2 hunks)src/main/kotlin/io/github/alkoleft/mcp/application/services/LauncherService.kt(3 hunks)src/main/kotlin/io/github/alkoleft/mcp/configuration/properties/ApplicationProperties.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/configuration/properties/BuildProperties.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/storage/FileBuildStateManager.kt(1 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/storage/MapDbHashStorage.kt(2 hunks)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.kt(1 hunks)src/test/kotlin/io/github/alkoleft/mcp/RealTests.kt(2 hunks)src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGeneratorTest.kt(1 hunks)
🧰 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/configuration/properties/ApplicationProperties.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/storage/FileBuildStateManager.ktsrc/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGeneratorTest.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/services/LauncherService.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/actions/build/DesignerBuildAction.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/actions/common/Actions.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/storage/MapDbHashStorage.ktsrc/main/kotlin/io/github/alkoleft/mcp/configuration/properties/BuildProperties.ktsrc/main/kotlin/io/github/alkoleft/mcp/application/actions/build/AbstractBuildAction.ktsrc/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.ktsrc/test/kotlin/io/github/alkoleft/mcp/RealTests.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/PartialLoadListGeneratorTest.ktsrc/test/kotlin/io/github/alkoleft/mcp/RealTests.kt
🧠 Learnings (1)
📚 Learning: 2025-08-11T22:22:35.393Z
Learnt from: CR
Repo: alkoleft/mcp-onec-test-runner PR: 0
File: .cursor/rules/kotlin.mdc:0-0
Timestamp: 2025-08-11T22:22:35.393Z
Learning: Applies to src/androidTest/**/*.kt : Use integration tests for each API module
Applied to files:
src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGeneratorTest.ktsrc/test/kotlin/io/github/alkoleft/mcp/RealTests.kt
🧬 Code graph analysis (3)
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (1)
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/ibcmd/commands/config/ConfigCommandBuilder.kt (1)
import(76-79)
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/DesignerBuildAction.kt (1)
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/platform/dsl/designer/commands/LoadConfigFromFilesCommand.kt (4)
fromPath(57-59)partial(73-75)listFile(77-79)updateConfigDumpInfo(85-87)
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/AbstractBuildAction.kt (3)
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/DesignerBuildAction.kt (6)
initDsl(44-51)loadConfigurationPartial(61-74)loadConfiguration(53-59)loadExtensionPartial(85-99)loadExtension(76-83)updateDb(101-101)src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (6)
initDsl(44-48)loadConfigurationPartial(59-75)loadConfiguration(50-57)loadExtensionPartial(91-107)loadExtension(77-89)updateDb(109-113)src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.kt (1)
shouldUsePartialLoad(278-295)
🔇 Additional comments (15)
.gitignore (1)
49-52: Убедитесь, чтоmcp-example.jsonиDEPLOYMENT.md- это локальные файлы, а не примеры/документация для разработчиков.Правила для
mcp-configs/логичны для локальной конфигурации. Однакоmcp-example.jsonможет быть шаблоном или примером, который новые разработчики должны скопировать и использовать. Если это так, файл следует поместить в репо с расширением типа.templateили.example, а не игнорировать полностью. Аналогичная ситуация сDEPLOYMENT.md— если это общая документация по развертыванию, она должна быть доступна в репо.Пожалуйста, проверьте:
- Является ли
mcp-example.jsonрабочим локальным файлом (можно игнорировать) или примером конфигурации для разработчиков (должен быть в репо)?- Является ли
DEPLOYMENT.mdлокальным документом развертывания (можно игнорировать) или общей документацией проекта (должна быть в репо)?src/main/kotlin/io/github/alkoleft/mcp/infrastructure/storage/FileBuildStateManager.kt (1)
93-95: Упрощение updateHashes за счёт делегации в hashStorage выглядит удачнымС учётом того, что
MapDbHashStorage.batchUpdateтеперь сам обрабатывает ошибки и делает безопасный rollback, прямой вызов без локальногоtry/catchделает код чище и не дублирует обработку ошибок.src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.kt (4)
49-94: LGTM: Полный набор типов метаданных 1ССписок типов метаданных охватывает все основные типы объектов конфигурации 1С, что обеспечивает корректную работу с различными элементами конфигурации.
128-131: LGTM: Корректная валидация принадлежности файлаПроверка
startsWith(sourceSetPath)с предупреждением и ранним выходом корректно обрабатывает файлы вне source set.
278-296: LGTM: Корректная логика принятия решенияЛогика
shouldUsePartialLoad()правильно проверяет оба условия (порог и Configuration.xml) с соответствующим логированием.
158-191: Based on my research into 1C metadata structure standards, I can now assess the review comment. The web search confirms key findings that are relevant to the concerns raised:Findings from 1C Metadata Standards:
CommonModules structure (per official 1C documentation): Files are stored as
CommonModules/<Name>.bsldirectly—without anExtsubdirectory. This differs from other metadata types.Other metadata types (Catalogs, DataProcessors, Reports, etc.): Use the pattern
ObjectType/<ObjectName>/Ext/<ModuleType>.bslForms and Commands within objects: Typically stored as:
ObjectType/<ObjectName>/Forms/<FormName>/Module.bslObjectType/<ObjectName>/Commands/<CommandName>/Module.bslHowever, I was unable to verify the actual implementation of the
processBslFile()function,findMetadataInfo(), andaddAllFilesFromDirectory()due to repository clone failures. Without access to the code, I cannot confirm:
- Whether
findMetadataInfo()correctly distinguishes between CommonModules and other metadata types- Whether the logic properly handles BSL files in various locations (Forms, Commands, etc.)
- What the actual behavior would be for edge cases
Верифицируйте обработку вложенных BSL-файлов в соответствии со стандартной структурой 1C
На основе стандартов 1C:
- CommonModules хранят BSL-файлы напрямую:
CommonModules/<Name>.bsl(БЕЗ подкаталога Ext)- Другие типы метаданных используют:
ObjectType/<ObjectName>/Ext/<ModuleType>.bsl- BSL-файлы в Forms и Commands расположены внутри структуры объектов
Требуется проверить, что
findMetadataInfo()корректно обрабатывает все эти варианты размещения файлов.src/main/kotlin/io/github/alkoleft/mcp/application/actions/common/Actions.kt (1)
58-74: LGTM: Чёткое API для частичной загрузкиНовый метод
runPartial()корректно расширяет интерфейсBuildAction:
- Параметр
sourceSetChangesнеобходим для передачи информации об изменениях- KDoc ясно описывает логику автоматического выбора режима загрузки
- Сигнатура согласуется с существующим методом
run()src/test/kotlin/io/github/alkoleft/mcp/RealTests.kt (1)
88-88: LGTM: Корректное обновление конструктора в тестеДобавление
PartialLoadListGenerator()соответствует новой сигнатуре конструктораDesignerBuildAction.src/main/kotlin/io/github/alkoleft/mcp/configuration/properties/ApplicationProperties.kt (1)
42-42: LGTM: Последовательное расширение конфигурацииДобавление поля
buildсоответствует существующему паттерну вложенных свойств конфигурации (connection, tools).src/main/kotlin/io/github/alkoleft/mcp/application/services/LauncherService.kt (1)
147-161: LGTM: Корректная интеграция частичной загрузкиОбновление метода
updateIB():
- Новый параметр
sourceSetChangesнеобходим для принятия решения о режиме загрузки- Вызов
runPartial()вместоrun()активирует логику автоматического выбора- KDoc ясно описывает новое поведение
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/DesignerBuildAction.kt (2)
38-41: LGTM: Корректное внедрение зависимостиКонструктор правильно принимает
PartialLoadListGeneratorи передаёт его в суперкласс.
61-74: Consider makingupdateConfigDumpInfo()conditional for partial configuration loadsThe 1C Designer documentation confirms that
-updateConfigDumpInfois an optional flag for partial loads. When omitted, the partial load still functions correctly but does not synchronizeConfigDumpInfo.xml. Since this method updates XML metadata on disk with each partial load—even when only a few files change—it may introduce unnecessary I/O overhead.Recommendation: Evaluate whether
ConfigDumpInfo.xmlmust be synchronized immediately after every partial load or if it can be:
- Regenerated lazily when needed
- Updated only after batches of changes are complete
- Skipped entirely for partial loads where consistency is not critical
If ConfigDumpInfo synchronization is essential, document the performance implications and consider batching updates.
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt (1)
59-75: Based on the web search results, I can now provide a conclusive verification:The
baseDirparameter is correctly implemented and compatible with ibcmd.The ibcmd documentation confirms:
--base-diris the correct parameter name for the "files" import mode with--partialflagbaseDir = path.toString()properly converts the Path object to a String, which matches the CLI parameter format- The path can be either absolute or relative (this is typically determined by the file list contents, not the baseDir parameter itself)
- The implementation aligns with the documented ibcmd syntax:
ibcmd infobase config import files --base-dir <path> --partialThe original review comment's concerns about absolute vs. relative paths and compatibility with the
partialparameter are both addressed by the ibcmd documentation, which confirms this is the standard and correct approach.
No issues with the
baseDirparameter setting. The implementation is correct and follows ibcmd documentation.The parameter is properly configured for partial configuration imports using the "files" subcommand, regardless of whether the paths in the file list are absolute or relative.
src/test/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGeneratorTest.kt (1)
33-284: Хорошее и детальное покрытие сценариев PartialLoadListGeneratorНабор тестов хорошо проверяет ключевые случаи: простые XML, изменения BSL с добавлением связанного XML, «разворот» всего каталога/документа при изменении одного модуля, учёт Configuration.xml, порог partial‑load, CommonModules, DataProcessors, несколько объектов и игнорирование файлов вне source set. С точки зрения функциональности и Arrange‑Act‑Assert структура выглядит корректной.
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/AbstractBuildAction.kt (1)
44-46: Корректная интеграция поддержки частичной загрузки в AbstractBuildActionИнтеграция
PartialLoadListGeneratorвыглядит аккуратной:
runPartialсимметричен существующемуrunи использует тот жеmeasureExecutionTime, что сохраняет единый подсчёт длительности и обработку ошибок.executeBuildDslPartialповторяет общий pipeline полной сборки, но для конфигурации и расширений принимает решение черезshouldUsePartialLoad(...)и, при необходимости, вызываетloadConfigurationPartial/loadExtensionPartial.- Порог берётся из
properties.build.partialLoadThreshold, а проверкаConfiguration.xmlделегируется генератору, что концентрирует доменную логику в одном месте.- После частичных/полных загрузок по‑прежнему используется
BuildActionStateи общийupdateDb()+ финальныйtoResult, как и в полной сборке, так что поведение наружу остаётся единообразным.С учётом предоставленных сниппетов реализаций в
DesignerBuildActionиIbcmdBuildActionобщий дизайн выглядит целостным и расширяемым.Also applies to: 55-69, 131-192, 194-207
| override fun loadConfigurationPartial( | ||
| name: String, | ||
| path: Path, | ||
| changedFiles: Set<Path>, | ||
| ): ProcessResult { | ||
| // Для ibcmd используем подкоманду "files" и параметр --partial | ||
| // ibcmd infobase config import files --partial <path> | ||
| lateinit var result: ProcessResult | ||
| actionDsl.config { | ||
| result = import(path) { | ||
| importSubCommand = "files" | ||
| partial = true | ||
| baseDir = path.toString() | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Неиспользуемый параметр changedFiles
Оба метода loadConfigurationPartial() и loadExtensionPartial() принимают параметр changedFiles: Set<Path>, но не используют его. В отличие от DesignerBuildAction, здесь не вызывается partialLoadListGenerator.generate().
Возможные проблемы:
- Некорректная реализация: Если ibcmd не поддерживает файл списка (listFile), как DESIGNER, то частичная загрузка может не работать корректно
- Неполная реализация: Возможно, для ibcmd требуется другой механизм передачи списка изменённых файлов
Проверьте документацию ibcmd и уточните:
- Поддерживает ли
ibcmd config import files --partialавтоматическое определение изменённых файлов? - Или требуется явная передача списка файлов?
ibcmd config import files --partial changed files list
Also applies to: 91-107
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/application/actions/build/IbcmdBuildAction.kt
around lines 59-75 and 91-107, the changedFiles parameter is accepted but never
used; verify ibcmd's behavior and either (A) if ibcmd supports passing an
explicit changed-files list for partial imports, call the
partialLoadListGenerator to generate the list from changedFiles and pass that
list path (e.g., set the import/listFile parameter or equivalent) into the
actionDsl.import call, or (B) if ibcmd auto-detects changed files or does not
support a list, remove the unused changedFiles parameter and/or add a clear
comment explaining why it is unused; implement the chosen path consistently in
both loadConfigurationPartial and loadExtensionPartial.
| data class BuildProperties( | ||
| /** | ||
| * Порог количества файлов для переключения на частичную загрузку. | ||
| * Если количество измененных файлов меньше этого значения, | ||
| * будет использована частичная загрузка (partial load). | ||
| * По умолчанию: 20 файлов | ||
| */ | ||
| val partialLoadThreshold: Int = 20, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Добавьте валидацию порогового значения
Значение partialLoadThreshold может быть установлено в некорректное значение (≤0 или чрезмерно большое) через конфигурацию, что приведёт к неожиданному поведению.
Добавьте валидацию:
data class BuildProperties(
/**
* Порог количества файлов для переключения на частичную загрузку.
* Если количество измененных файлов меньше этого значения,
* будет использована частичная загрузка (partial load).
* По умолчанию: 20 файлов
*/
val partialLoadThreshold: Int = 20,
-)
+) {
+ init {
+ require(partialLoadThreshold > 0) {
+ "partialLoadThreshold должен быть больше 0, получено: $partialLoadThreshold"
+ }
+ }
+}📝 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.
| data class BuildProperties( | |
| /** | |
| * Порог количества файлов для переключения на частичную загрузку. | |
| * Если количество измененных файлов меньше этого значения, | |
| * будет использована частичная загрузка (partial load). | |
| * По умолчанию: 20 файлов | |
| */ | |
| val partialLoadThreshold: Int = 20, | |
| ) | |
| data class BuildProperties( | |
| /** | |
| * Порог количества файлов для переключения на частичную загрузку. | |
| * Если количество измененных файлов меньше этого значения, | |
| * будет использована частичная загрузка (partial load). | |
| * По умолчанию: 20 файлов | |
| */ | |
| val partialLoadThreshold: Int = 20, | |
| ) { | |
| init { | |
| require(partialLoadThreshold > 0) { | |
| "partialLoadThreshold должен быть больше 0, получено: $partialLoadThreshold" | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/configuration/properties/BuildProperties.kt
around lines 27 to 35, add runtime validation for partialLoadThreshold to reject
non-positive or unreasonably large values; implement an init block (or use
require) to ensure partialLoadThreshold is within an acceptable range (e.g.
1..10_000) and throw an IllegalArgumentException with a clear message if
violated, so invalid configuration values (≤0 or too large) are rejected early.
| fun generate( | ||
| sourceSetPath: Path, | ||
| changedFiles: Set<Path>, | ||
| ): Path { | ||
| logger.debug { "Генерация списка файлов для частичной загрузки: ${changedFiles.size} файлов" } | ||
|
|
||
| val filesToLoad = mutableSetOf<String>() | ||
|
|
||
| changedFiles.forEach { absolutePath -> | ||
| processChangedFile(sourceSetPath, absolutePath, filesToLoad) | ||
| } | ||
|
|
||
| logger.info { "Подготовлено ${filesToLoad.size} файлов для частичной загрузки" } | ||
|
|
||
| return createListFile(filesToLoad) | ||
| } |
There was a problem hiding this comment.
Проверьте очистку временных файлов
Метод создаёт временный файл через createListFile(), но не обеспечивает его удаление. В случае многократных вызовов это может привести к накоплению временных файлов.
Рекомендация: добавьте механизм очистки временных файлов после использования или используйте deleteOnExit():
private fun createListFile(files: Set<String>): Path {
val listFile = Files.createTempFile("partial-load-", ".txt")
+ listFile.toFile().deleteOnExit()
Files.write(listFile, files.sorted())
logger.debug { "Создан файл списка: $listFile с ${files.size} записями" }
return listFile
}Либо измените API так, чтобы вызывающий код отвечал за очистку, явно документировав это.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.kt
around lines 103-118, the generate() method calls createListFile(filesToLoad)
which creates a temporary file but never ensures its deletion; modify the
implementation so temporary files are cleaned up: either (A) call createListFile
to mark the file for deletion (e.g., invoke deleteOnExit() on the created
Path/File or register a JVM shutdown hook) before returning, or (B) change the
API so generate() returns a managed resource (e.g., a Closeable/AutoCloseable
wrapper or provide an explicit delete() method) and document that the caller is
responsible for deletion; implement one approach consistently and update method
javadoc to state who owns cleanup responsibility.
| try { | ||
| directory.walk() | ||
| .filter { Files.isRegularFile(it) } | ||
| .forEach { file -> | ||
| val relativePath = sourceSetPath.relativize(file).toString().replace("\\", "/") | ||
| filesToLoad.add(relativePath) | ||
| logger.trace { "Добавлен связанный файл: $relativePath" } | ||
| } | ||
| } catch (e: Exception) { | ||
| logger.warn(e) { "Ошибка при сканировании каталога: $directory" } | ||
| } |
There was a problem hiding this comment.
Улучшите обработку ошибок при сканировании каталогов
При ошибке сканирования выводится предупреждение, но процесс продолжается. Это может привести к неполному списку файлов и неожиданным результатам сборки.
Рекомендации:
- Логируйте конкретный тип исключения для диагностики
- Рассмотрите прерывание генерации при критических ошибках доступа
} catch (e: Exception) {
- logger.warn(e) { "Ошибка при сканировании каталога: $directory" }
+ logger.error(e) { "Критическая ошибка при сканировании каталога $directory: ${e.javaClass.simpleName}" }
+ throw IllegalStateException("Не удалось собрать полный список файлов для частичной загрузки", e)
}📝 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.
| try { | |
| directory.walk() | |
| .filter { Files.isRegularFile(it) } | |
| .forEach { file -> | |
| val relativePath = sourceSetPath.relativize(file).toString().replace("\\", "/") | |
| filesToLoad.add(relativePath) | |
| logger.trace { "Добавлен связанный файл: $relativePath" } | |
| } | |
| } catch (e: Exception) { | |
| logger.warn(e) { "Ошибка при сканировании каталога: $directory" } | |
| } | |
| try { | |
| directory.walk() | |
| .filter { Files.isRegularFile(it) } | |
| .forEach { file -> | |
| val relativePath = sourceSetPath.relativize(file).toString().replace("\\", "/") | |
| filesToLoad.add(relativePath) | |
| logger.trace { "Добавлен связанный файл: $relativePath" } | |
| } | |
| } catch (e: Exception) { | |
| logger.error(e) { "Критическая ошибка при сканировании каталога $directory: ${e.javaClass.simpleName}" } | |
| throw IllegalStateException("Не удалось собрать полный список файлов для частичной загрузки", e) | |
| } |
🤖 Prompt for AI Agents
In
src/main/kotlin/io/github/alkoleft/mcp/infrastructure/utility/PartialLoadListGenerator.kt
around lines 226 to 236, the catch block currently logs a generic warning and
continues, which can mask the specific error and allow generation to proceed
with incomplete file lists; update the catch to log the exception type and
message (e.g., include e::class.simpleName and e.message) and decide on failing
fast for critical I/O errors by rethrowing a wrapped RuntimeException or
specific custom exception after logging so the generation is aborted when
access/permission errors occur; non-critical exceptions can still be logged and
skipped, but ensure the log includes full diagnostic details (exception class,
message, and stacktrace) to aid troubleshooting.
1 participant
Описание
Добавлена поддержка частичной загрузки конфигурации для ускорения сборки.
Вместо загрузки всех файлов загружаются только измененные через параметры
-partial -listFile.Вдохновлено проектом: https://github.com/Nikolay-Shirokov/partial-load-config
Изменения
PartialLoadListGenerator— генерация списка файлов с BSL→XML маппингомBuildActionметодомrunPartial()-partial -listFile) и IBCMD (config import files --partial)app.build.partial-load-threshold(по умолчанию 20)Результат
Сборка 2 файлов: ~60 сек вместо ~15 мин 🚀
Summary by CodeRabbit
Примечания к выпуску
Новые возможности
partialLoadThreshold(по умолчанию: 20) для управления режимом загрузки.Тесты
Улучшения
✏️ Tip: You can customize this high-level summary in your review settings.