refact predefined subsystem functions #163
Conversation
WalkthroughВ данном пулл-реквесте внесены изменения в несколько файлов, касающихся обработки предопределенных значений и разрешения конфликтов в системе. Основное внимание уделено переносу функций в новый служебный модуль, что изменяет способ обработки предопределенных элементов. Обновлены версии библиотек, а также добавлены новые функции и процедуры для управления предопределенными значениями, включая их создание, обновление и удаление. Изменения также включают обновление метаданных в XML-файлах. Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Analysis Details3 Issues
Coverage and Duplications
Project ID: PSSL |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/cf/CommonModules/пбп_ПредопределенныеЗначения/Ext/Module.bsl (5)
Line range hint
34-47: Отсутствует проверка входных параметров на заполненностьВ функции
ОбработатьПредопределенныеЗначенияне проводится проверка параметровТаблицаПредопределенныхиМенеджерОбъектана заполненность илиНеопределено. Это может привести к ошибкам выполнения при передаче некорректных значений.Рекомендуется добавить проверки входных параметров:
Функция ОбработатьПредопределенныеЗначения(ТаблицаПредопределенных, МенеджерОбъекта) Экспорт Если Не ЗначениеЗаполнено(ТаблицаПредопределенных) Тогда Возврат Новый ТаблицаЗначений; КонецЕсли; Если Не ЗначениеЗаполнено(МенеджерОбъекта) Тогда Возврат Новый ТаблицаЗначений; КонецЕсли; // Дальнейший код... КонецФункции
Line range hint
75-76: Лишний возврат из функцииВ строке 75-76 производится возврат переменной
Параметры, которая уже инициализирована в начале функцииДопПараметрыОбработкиПредопределенныхЭлементов. Этот возврат избыточен, так как функция все равно продолжит выполнение кода ниже.Можно убрать данный блок кода, чтобы исключить избыточность:
Если Не МенеджерВходитВПодсистемуПредопределенных(МенеджерОбъекта) Тогда - Возврат Параметры; КонецЕсли;
Line range hint
134-140: Риск SQL-инъекции при формировании текста запросаПри использовании функции
СтрЗаменитьдля формирования текста запроса (строки 135-139) существует риск внедрения вредоносного кода при подстановке данных. Это может привести к SQL-инъекциям.Рекомендуется использовать параметры запроса вместо подстановки строк:
Запрос.Текст = "ВЫБРАТЬ | ... |ИЗ | &ИмяОбъекта КАК пбп_ПредопределенныеЗначения |ГДЕ | ..."; Запрос.УстановитьПараметр("ИмяОбъекта", ПолноеИмяОбъекта);
Line range hint
259-266: Не обработан случай отсутствия элемента в выборкеВ процедуре
УдалитьИзТаблицыСуществующиеЭлементыотсутствует обработка ситуации, когда элемент не найден в выборкеВыборкаДетальныеЗаписи(строки 259-266). Это может привести к непредвиденному поведению программы.Рекомендуется добавить обработку случая, когда элемент не найден:
Если ВыборкаДетальныеЗаписи.НайтиСледующий(Строка.ИдентификаторНастройки, "ИдентификаторНастройки") Тогда // Существующий код... Иначе // Обработка ситуации отсутствия элемента КонецЕсли;
Line range hint
488-490: Избыточная проверка на заполненностьВ процедуре
ОбновитьЭлементпроверяется заполненностьСтрокаТаблицы.Служебный_ПредопределенныйЭлемент(строки 488-490). Если до этой процедуры гарантируется, что данный элемент всегда заполнен, эту проверку можно удалить для упрощения кода.Удалите избыточную проверку:
- Если Не ЗначениеЗаполнено(СтрокаТаблицы.Служебный_ПредопределенныйЭлемент) Тогда - Возврат; - КонецЕсли;src/cf/CommonForms/пбп_ФормаРазрешенияКонфликтовПредопределенныхЭлементов/Ext/Form/Module.bsl (1)
208-212: Рекомендуется добавить обработку ошибокДля повышения надежности кода рекомендуется добавить блок
Попытка...Исключениепри вызове методов служебного модуля, так как операции с предопределенными элементами могут вызвать исключения во время выполнения.Менеджер = пбп_Переадресация.МенеджерОбъектаПоПолномуИмени(ИмяФормыВладельца); + Попытка ДопПараметры = пбп_ПредопределенныеЗначенияСлужебный.ДопПараметрыОбработкиПредопределенныхЭлементов( Таблица, Менеджер); пбп_ПредопределенныеЗначенияСлужебный.СоздатьОбновитьПредопределенныеЗначения(Таблица, ДопПараметры); + Исключение + ЗаписьЖурналаРегистрации("ОбработкаПредопределенныхЭлементов", + УровеньЖурналаРегистрации.Ошибка,,, + ПодробноеПредставлениеОшибки(ИнформацияОбОшибке())); + ВызватьИсключение; + КонецПопытки
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/cf/CommonForms/пбп_ФормаРазрешенияКонфликтовПредопределенныхЭлементов/Ext/Form/Module.bsl(1 hunks)src/cf/CommonModules/пбп_ОбновлениеИнформационнойБазыПБП/Ext/Module.bsl(1 hunks)src/cf/CommonModules/пбп_ПредопределенныеЗначения/Ext/Module.bsl(1 hunks)src/cf/CommonModules/пбп_ПредопределенныеЗначенияСлужебный/Ext/Module.bsl(2 hunks)src/cf/Configuration.xml(1 hunks)src/cf/Subsystems/пбп_ПредопределенныеЗначения.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/cf/Configuration.xml
🔇 Additional comments (3)
src/cf/Subsystems/пбп_ПредопределенныеЗначения.xml (1)
41-41: Добавление нового модуля в подсистему
В строке 41 добавлен новый модуль пбп_ПредопределенныеЗначенияСлужебный в содержимое подсистемы. Убедитесь, что добавление данного модуля необходимо и корректно, а также что это не нарушает зависимостей между модулями.
Запустите следующий скрипт для проверки зависимостей:
✅ Verification successful
Let me generate additional scripts to verify the module dependencies and structure:
Based on the analysis of the code and subsystem structure, I can now provide the final response:
Добавление модуля корректно и обосновано
Модуль пбп_ПредопределенныеЗначенияСлужебный является ключевым компонентом подсистемы предопределенных значений и используется другими модулями подсистемы для:
- Обработки предопределенных элементов
- Управления состояниями предопределенных элементов
- Проверки зависимостей между модулями
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Описание: Найти все модули, использующие пбп_ПредопределенныеЗначенияСлужебный
rg 'пбп_ПредопределенныеЗначенияСлужебный' src/
Length of output: 1942
Script:
#!/bin/bash
# Проверим содержимое модуля для понимания его назначения
cat src/cf/CommonModules/пбп_ПредопределенныеЗначенияСлужебный/Ext/Module.bsl
# Проверим структуру подсистемы
cat src/cf/Subsystems/пбп_ПредопределенныеЗначения.xml
Length of output: 27910
src/cf/CommonModules/пбп_ОбновлениеИнформационнойБазыПБП/Ext/Module.bsl (1)
77-77: Обновление версии без соответствующих изменений
В строке 77 версия обновлена с "1.0.4.14" до "1.0.4.15". Однако в данном модуле отсутствуют другие изменения, кроме обновления версии. Убедитесь, что версия обновляется синхронно с изменениями в функциональности или других связанных модулях.
Проверьте соответствие номера версии с изменениями в проекте и обновите описание изменений в файле CHANGELOG при необходимости.
src/cf/CommonForms/пбп_ФормаРазрешенияКонфликтовПредопределенныхЭлементов/Ext/Form/Module.bsl (1)
209-211: Проверьте зависимости после переноса функциональности в служебный модуль
Перенос функциональности из пбп_ПредопределенныеЗначения в пбп_ПредопределенныеЗначенияСлужебный выполнен корректно. Однако необходимо убедиться, что все зависимые модули обновлены соответствующим образом.
✅ Verification successful
Let me generate additional verification scripts to check for any other potential references or usages that might need updating.
Based on the analysis of the search results, I can now provide the final response:
Перенос функциональности в служебный модуль выполнен корректно
Проверка показала, что:
- Все вызовы методов
ДопПараметрыОбработкиПредопределенныхЭлементовиСоздатьОбновитьПредопределенныеЗначениякорректно перенесены в служебный модуль - Старые ссылки на эти методы в модуле
пбп_ПредопределенныеЗначенияотсутствуют - Оставшиеся упоминания старого модуля являются корректными (в комментариях к API и в запросах к метаданным)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to the old module have been updated
# Search for any remaining references to the old module
rg "пбп_ПредопределенныеЗначения\.(?:ДопПараметрыОбработкиПредопределенныхЭлементов|СоздатьОбновитьПредопределенныеЗначения)"
# Search for references to the new service module to ensure consistent usage
rg "пбп_ПредопределенныеЗначенияСлужебный\.(?:ДопПараметрыОбработкиПредопределенныхЭлементов|СоздатьОбновитьПредопределенныеЗначения)"
Length of output: 1437
Script:
#!/bin/bash
# Search for any other references to пбп_ПредопределенныеЗначения module
rg "пбп_ПредопределенныеЗначения\." -A 2
# Search for files that might contain both old and new module references
rg -l "пбп_ПредопределенныеЗначения" | xargs rg "пбп_ПредопределенныеЗначенияСлужебный"
Length of output: 19403
| Функция УдалитьИзТаблицыСуществующиеЭлементы(ТаблицаПредопределенных, ДополнительныеПараметры) Экспорт | ||
|
|
||
| ТаблицаКонфликтныхЭлементов = пбп_ПредопределенныеЗначенияПереопределяемый | ||
| .ТаблицаКонфликтныхЭлементов(ДополнительныеПараметры.Менеджер, ТаблицаПредопределенных.СкопироватьКолонки()); | ||
|
|
||
| Если Не ЗначениеЗаполнено(ТаблицаПредопределенных) Тогда | ||
| Возврат ТаблицаКонфликтныхЭлементов; | ||
| КонецЕсли; | ||
|
|
||
| ПолноеИмяОбъекта = ИмяОбъектаМетаданных(ДополнительныеПараметры.ПолноеИмя); | ||
|
|
||
| ТаблицаПредопределенных.Сортировать("ЭтоГруппа УБЫВ, УровеньИерархии"); | ||
|
|
||
| ОбработатьДублиИдентификаторов(ТаблицаПредопределенных, ТаблицаКонфликтныхЭлементов); | ||
|
|
||
| ИдентификаторыНастроек = ТаблицаПредопределенных.ВыгрузитьКолонку("ИдентификаторНастройки"); | ||
|
|
||
| Запрос = Новый Запрос; | ||
| ТекстЗапроса = | ||
| "ВЫБРАТЬ | ||
| | пбп_ПредопределенныеЗначения.Ссылка КАК Ссылка, | ||
| | пбп_ПредопределенныеЗначения.ИдентификаторНастройки КАК ИдентификаторНастройки, | ||
| | пбп_СостоянияПредопределенныхЭлементов.ХешСумма КАК ХешСумма, | ||
| | ЕСТЬNULL(пбп_СостоянияПредопределенныхЭлементов.РучноеИзменение, ЛОЖЬ) КАК РучноеИзменение, | ||
| | пбп_ПредопределенныеЗначения.ПометкаУдаления КАК ПометкаУдаления, | ||
| | пбп_СостоянияПредопределенныхЭлементов.ХешСумма ЕСТЬ NULL КАК ХешОтсутствует | ||
| |ИЗ | ||
| | %1 КАК пбп_ПредопределенныеЗначения | ||
| | ЛЕВОЕ СОЕДИНЕНИЕ РегистрСведений.пбп_СостоянияПредопределенныхЭлементов КАК пбп_СостоянияПредопределенныхЭлементов | ||
| | ПО пбп_ПредопределенныеЗначения.Ссылка = пбп_СостоянияПредопределенныхЭлементов.Объект | ||
| |ГДЕ | ||
| | пбп_ПредопределенныеЗначения.ИдентификаторНастройки В(&СписокИдентификаторов)"; | ||
|
|
||
| Запрос.Текст = СтрШаблон(ТекстЗапроса, ПолноеИмяОбъекта); | ||
|
|
||
| Запрос.УстановитьПараметр("СписокИдентификаторов", ИдентификаторыНастроек); | ||
|
|
||
| ВыборкаДетальныеЗаписи = Запрос.Выполнить().Выбрать(); | ||
|
|
||
| СтрокиДляУдаления = Новый Массив; | ||
| Для Каждого Строка Из ТаблицаПредопределенных Цикл | ||
| ВыборкаДетальныеЗаписи.Сбросить(); | ||
| Если ВыборкаДетальныеЗаписи.НайтиСледующий(Строка.ИдентификаторНастройки, "ИдентификаторНастройки") Тогда | ||
| Если ВыборкаДетальныеЗаписи.РучноеИзменение Или ВыборкаДетальныеЗаписи.ХешСумма = Строка.ХешСумма Тогда | ||
| СтрокиДляУдаления.Добавить(Строка); | ||
| Иначе | ||
| ОбработатьСтроку(Строка, ВыборкаДетальныеЗаписи, ТаблицаКонфликтныхЭлементов, СтрокиДляУдаления); | ||
| КонецЕсли; | ||
| КонецЕсли; | ||
| КонецЦикла; | ||
|
|
||
| Для Каждого Строка Из СтрокиДляУдаления Цикл | ||
| ТаблицаПредопределенных.Удалить(Строка); | ||
| КонецЦикла; | ||
|
|
||
| Возврат ТаблицаКонфликтныхЭлементов; | ||
|
|
||
| КонецФункции |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Отсутствует проверка на пустоту переданной таблицы
В функции УдалитьИзТаблицыСуществующиеЭлементы нет проверки на пустоту параметра ТаблицаПредопределенных. При передаче пустой таблицы возможны ошибки при выполнении дальнейших операций.
Добавьте проверку на пустоту в начале функции:
Если ТаблицаПредопределенных.Количество() = 0 Тогда
Возврат Новый ТаблицаЗначений;
КонецЕсли;There was a problem hiding this comment.
Если Не ЗначениеЗаполнено(ТаблицаПредопределенных) Тогда
Возврат ТаблицаКонфликтныхЭлементов;
КонецЕсли;вот проверка вначале, значение заполнено оптимально проверяет, есть ли записи в таблице значений
There was a problem hiding this comment.
@denisushakov, спасибо за пояснение. Вы правы, проверка на пустоту таблицы уже присутствует в функции с использованием ЗначениеЗаполнено(ТаблицаПредопределенных). Приношу извинения за невнимательность.
(_/)
( •_•)
✏️ Learnings added
Learnt from: denisushakov
PR: firstBitSportivnaya/PSSL#163
File: src/cf/CommonModules/пбп_ПредопределенныеЗначенияСлужебный/Ext/Module.bsl:217-274
Timestamp: 2024-12-12T15:00:30.060Z
Learning: В языке BSL функция `ЗначениеЗаполнено(ТаблицаЗначений)` эффективно проверяет наличие записей в таблице значений.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@denisushakov а он че понимает, что ты ему пишешь? Он в исключения себе заносит или просто данное замечание перестает игнорировать, как сонар?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@denisushakov а он че понимает, что ты ему пишешь? Он в исключения себе заносит или просто данное замечание перестает игнорировать, как сонар?
Он учится чтобы потом заменить тебя
There was a problem hiding this comment.
@thvvmas @ivanmolodec Начинаем давать не правильные правила написания кода)
There was a problem hiding this comment.
@denisushakov а он че понимает, что ты ему пишешь? Он в исключения себе заносит или просто данное замечание перестает игнорировать, как сонар?
Он учится чтобы потом заменить тебя
Ну во вкусно и точка всегда вакансии есть =)
|
|
||
| Просмотренные = Новый Соответствие; | ||
|
|
||
| СтрокиДляУдаления = Новый Массив; | ||
| Для Каждого Строка Из ТаблицаПредопределенных Цикл | ||
| Если ПустаяСтрока(Строка.ИдентификаторНастройки) Тогда | ||
| СтрокиДляУдаления.Добавить(Строка); | ||
|
|
||
| СообщитьОНезаполненностиИдентификатора(Строка.Наименование); | ||
| Продолжить; | ||
| КонецЕсли; | ||
|
|
||
| Если Просмотренные[Строка.ИдентификаторНастройки] <> Неопределено Тогда | ||
| Просмотренные[Строка.ИдентификаторНастройки] = Просмотренные[Строка.ИдентификаторНастройки] + 1; | ||
| Иначе | ||
| Просмотренные.Вставить(Строка.ИдентификаторНастройки, 1); | ||
| КонецЕсли; | ||
| КонецЦикла; | ||
|
|
||
| Для Каждого Строка Из ТаблицаПредопределенных Цикл | ||
| Количество = Просмотренные[Строка.ИдентификаторНастройки]; | ||
| Если Количество <> Неопределено И Количество > 1 Тогда | ||
| СтрокиДляУдаления.Добавить(Строка); | ||
|
|
||
| СтрокаКонфликт = ТаблицаКонфликтныхЭлементов.Добавить(); | ||
| ЗаполнитьЗначенияСвойств(СтрокаКонфликт, Строка); | ||
| СтрокаКонфликт.Служебный_ДублированиеИдентификаторов = Истина; | ||
|
|
||
| СообщитьОДублированииИдентификаторов(Строка.ИдентификаторНастройки); | ||
| КонецЕсли; | ||
| КонецЦикла; | ||
|
|
||
| Для Каждого Строка Из СтрокиДляУдаления Цикл | ||
| ТаблицаПредопределенных.Удалить(Строка); | ||
| КонецЦикла; | ||
|
|
||
| КонецПроцедуры |
There was a problem hiding this comment.
Потенциальная ошибка при удалении строк из таблицы в цикле
В процедуре ОбработатьДублиИдентификаторов строки удаляются из таблицы внутри цикла по самой этой таблице (строки 407-410). Это может привести к непредсказуемому поведению цикла.
Рекомендуется собрать строки для удаления в отдельный массив и после цикла удалить их из таблицы:
// Собираем строки для удаления
СтрокиДляУдаления = Новый Массив;
// ...
// После цикла удаляем строки
Для Каждого УдаляемаяСтрока Из СтрокиДляУдаления Цикл
ТаблицаПредопределенных.Удалить(УдаляемаяСтрока);
КонецЦикла;There was a problem hiding this comment.
Нет, конфликта не будет, мы обходим массив строк, и удаляем каждую строку
There was a problem hiding this comment.
@denisushakov, спасибо за пояснение. Если удаление строк происходит по отдельному массиву, проблем не возникнет.
(•‿•)
✏️ Learnings added
Learnt from: denisushakov
PR: firstBitSportivnaya/PSSL#163
File: src/cf/CommonModules/пбп_ПредопределенныеЗначенияСлужебный/Ext/Module.bsl:375-411
Timestamp: 2024-12-12T15:04:03.281Z
Learning: В процедуре `ОбработатьДублиИдентификаторов` в файле `src/cf/CommonModules/пбп_ПредопределенныеЗначенияСлужебный/Ext/Module.bsl` допустимо удалять строки из `ТаблицаПредопределенных` внутри цикла, если перебор осуществляется по массиву строк для удаления, а не по самой таблице.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Новые функции
Исправления ошибок
Документация