bsl-language-server
bsl-language-server copied to clipboard
1c-syntax
[FP] Нарушение правил работы с транзакциями для метода 'ЗафиксироватьТранзакцию' при досрочном выходе из процедуры.
Диагностика
Нарушение правил работы с транзакциями для метода 'ЗафиксироватьТранзакцию' Метод 'ЗафиксироватьТранзакцию' должен идти последним в блоке 'Попытка' перед оператором 'Исключение'
Версия
Сонар плагин 1.10.0
Описание ложного срабатывания диагностики
Для кода вида:
НачатьТранзакцию();
Попытка
// нечто
Если Истина Тогда
ЗафиксироватьТранзакцию();
Возврат;
КонецЕсли;
// Еще нечто
ЗафиксироватьТранзакцию();
Исключение
ОтменитьТранзакцию();
ВызватьИсключение;
КонецПопытки
диагностика срабатывает в блоке Если, хотя раз за фиксацией транзакции следует возврат, то это явно последний метод в блоке попытка и процедуре вообще.
Пример кода
см. выше
Скриншоты
Дополнительная информация
Обсуждение в телеграм возможных подводных камней:
Дмитрий, [23.03.2022 11:17] ЗафиксироватьТранзакцию(); Возврат Результат; Код выше ловится правилом Метод 'ЗафиксироватьТранзакцию' должен идти последним в блоке 'Попытка' перед оператором 'Исключение' Я конечно понимаю, что формально правило нарушено, но когда следующей строчкой идет возврат - то логически это все-таки последний оператор в блоке. Это достойно ИШШ или холивар, и вопрос лучше не поднимать? (естественно все это происходит внутри попытки, и таких досрочных выходов там пяток)
Artur, [23.03.2022 11:32] [In reply to Дмитрий] считаю, что такое срабатывание ложно. предлагаю сделать ишуз
Abramov Dmitry, [23.03.2022 11:32] [In reply to Artur] А если будет что-то типо Возврат Результат.НесуществующееПоле?
Artur, [23.03.2022 11:34] [In reply to Abramov Dmitry] обычное исключение с двойным падением ) к обсуждение правилу падение не относится.
Кирилл Черненко, [23.03.2022 11:34] [In reply to Abramov Dmitry] Скорее уместно спросить что делать с Возврат ПроцедураНаЕщеТыщуСтрокКода()
Artur, [23.03.2022 11:35] [In reply to Кирилл Черненко] ну да, для вызовов методов уже сложнее.
проще, конечно, следовать правилу из стандарта )
Дмитрий, [23.03.2022 11:35] [In reply to Abramov Dmitry] можно ограничить "Возврат;"
Кирилл Черненко, [23.03.2022 11:36] [In reply to Artur] Ну так правило по стандарту, и выше не фп а нарушение стандарта, то что людям нравится так писать не значит что правило должно подстраиваться
Дмитрий, [23.03.2022 11:37] [In reply to Кирилл Черненко] Для процедур вы согласны, что ЗафиксироватьТранзакцию(); Возврат; это четкое следование духу стандарта?
Кирилл Черненко, [23.03.2022 11:38] [In reply to Дмитрий] Нет, ведь и возврат в этой конструкции может упасть в исключение в случае если был параметр без знач в который сунули не сериализуемое значение и это было вызов сервера с клиента
Nikita Fedkin, [23.03.2022 11:43] и вот никто из вас не подумал, что упасть может фиксация транзакции
Дмитрий, [23.03.2022 11:44] И что? Любой код ниже в этой ветке исполнения недостижим, значит Попытка ... Зафиксировать(); Возврат; ... Исключение Отмена(); ВызватьИсключение; конецПопытки; КонецПроцедуры; и Попытка ... ЕслиИстина Зафиксировать(); Иначе ... КонецЕсли; Исключение Отмена(); ВызватьИсключение; конецПопытки; КонецПроцедуры; эквивалентны Все равно, даже если падать в момент возврата транзакция в обоих случаях будет зафиксирована, а потом исключение. Только если возврат внутри попытки он сначала свалится в блок исключения. Хотя при этом нарушится парность начала/отмены транзакции... Можно ненароком откатить внешнюю.
Artur, [23.03.2022 11:44] [In reply to Nikita Fedkin] это в стандарте, это неинтересно
Nikita Fedkin, [23.03.2022 11:44] поэтому фиксация и в попытке
Дмитрий, [23.03.2022 11:44] [In reply to Nikita Fedkin] Так фиксация в любом случае в попытке.
Кирилл Черненко, [23.03.2022 11:44] [In reply to Nikita Fedkin] Если фиксация упадет не будет сайд эффекта в бд
Указанный код на раз-два рефакторится Представь это ОбработкеПроведения() и тебе нужно проверить наличие остатков
НачатьТранзакцию();
Попытка
// нечто - ответственное чтение
Если Условие Тогда
ВызватьИсключение;
КонецЕсли;
// ЕшеНечто
ЗафиксироватьТранзакцию();
Исключение
ОтменитьТранзакцию();
КонецПопытки
или, если так надо пробрасывать исключение
НачатьТранзакцию();
Попытка
// нечто
Если ДействиеДоступно() Тогда
// Еще нечто
Иначе
Отказ = Истина;
КонецЕсли;
ЗафиксироватьТранзакцию();
Исключение
ОтменитьТранзакцию();
ВызватьИсключение;
КонецПопытки
Если Отказ Тогда
Возврат;
КонецЕсли;
или так
НачатьТранзакцию();
Попытка
НечтоИЕщёЧтото(Отказ);
ЗафиксироватьТранзакцию();
Исключение
ОтменитьТранзакцию();
ВызватьИсключение;
КонецПопытки
Если Отказ Тогда
Возврат;
КонецЕсли;
Зависит от контекста. Какой у вас код?
- У меня типовой код и я не хочу рефакторить за 1С.
- У меня может быть простыня на 5 экранов. И для некоторого условия я могу сразу сказать ок и свалить из функции. Почему-то во всех примерах ты крутишься вокруг перехода на отказ. Да, если все плохо я могу сразу выкинуть исключение и упасть в отмену транзакции. Но мне как раз наоборот стало хорошо раньше простыни обработки крайних случаев. В этот момент я фиксирую транзакцию и ухожу, вместо того, чтобы загнать весь код ниже в блок "иначе" и в 2 раза увеличить когнитивку обработки особых ситуаций.
Типичный пример ниже. И строчки ЗафиксироватьТранзакцию() и отменитьТранзакцию() с возвратом после них не должны учитываться при расчете парности совсем.
Функция ПодготовитьКОтправкеДокумент(Сообщение, КонтекстДиагностики, СообщенияДляОбработки, ОбработанныеСообщения, ИдентификаторПакета = Неопределено)
Результат = Ложь;
Действие = Перечисления.ДействияПоЭДО.ПодготовитьКОтправке;
НачатьТранзакцию();
Попытка
СообщениеОбъект = ОбщегоНазначенияБЭД.ОбъектПоСсылкеДляИзменения(Сообщение);
Если СообщениеОбъект.Состояние <> Перечисления.СостоянияСообщенийЭДО.ПодготовкаКОтправке Тогда
ЗафиксироватьТранзакцию();
Возврат Результат;
КонецЕсли;
ЭтоВходящийЭДО = ЭтоВходящийЭДО(СообщениеОбъект.ЭлектронныйДокумент);
ТребуетсяПоискПакета = Не ЭтоВходящийЭДО
И СообщениеОбъект.ТипЭлементаРегламента = Перечисления.ТипыЭлементовРегламентаЭДО.ИнформацияОтправителя
И ИдентификаторПакета = Неопределено;
Блокировка = Новый БлокировкаДанных;
ЭлементБлокировки = Блокировка.Добавить("Документ.СообщениеЭДО");
ЭлементБлокировки.УстановитьЗначение("ЭлектронныйДокумент", СообщениеОбъект.ЭлектронныйДокумент);
ЭлементБлокировки = Блокировка.Добавить(ИмяТаблицыДокументаЭДО(ЭтоВходящийЭДО));
ЭлементБлокировки.УстановитьЗначение("Ссылка", СообщениеОбъект.ЭлектронныйДокумент);
Запрос = Новый Запрос;
ТекстыЗапроса = Новый Массив;
ТекстыЗапроса.Добавить(ТекстЗапросаДанныхДляПодготовкиКОтправке(ЭтоВходящийЭДО));
Запрос.УстановитьПараметр("Сообщение", Сообщение);
ТекстыЗапроса.Добавить(ТекстЗапросаПараметровСостоянияДокумента());
Запрос.УстановитьПараметр("ЭлектронныйДокумент", СообщениеОбъект.ЭлектронныйДокумент);
Если ТребуетсяПоискПакета Тогда
ЭлементБлокировки = Блокировка.Добавить("РегистрСведений.СоставПакетовДокументовЭДО");
ЭлементБлокировки.УстановитьЗначение("ЭлектронныйДокумент", СообщениеОбъект.ЭлектронныйДокумент);
ТекстыЗапроса.Добавить(ТекстЗапросаИдентификатораПакетаДокумента());
КонецЕсли;
Запрос.Текст = СтрСоединить(ТекстыЗапроса, ОбщегоНазначения.РазделительПакетаЗапросов());
Блокировка.Заблокировать();
РезультатыЗапроса = Запрос.ВыполнитьПакет();
Если РезультатыЗапроса[1].Пустой()
ИЛИ РезультатыЗапроса[2].Пустой()
ИЛИ РезультатыЗапроса[3].Пустой()
ИЛИ РезультатыЗапроса[4].Пустой() Тогда
ОтменитьТранзакцию();
Возврат Результат;
КонецЕсли;
Если ТребуетсяПоискПакета И Не РезультатыЗапроса[7].Пустой() Тогда
ВыборкаПакетов = РезультатыЗапроса[7].Выбрать();
ВыборкаПакетов.Следующий();
ИдентификаторПакета = ВыборкаПакетов.ИдентификаторПакета;
Подготовлены = ПодготовитьСообщенияПакетаКОтправке(ИдентификаторПакета, Сообщение,
СообщенияДляОбработки, ОбработанныеСообщения, КонтекстДиагностики);
Если Не Подготовлены Тогда
ОтменитьТранзакцию();
Возврат Результат;
КонецЕсли;
КонецЕсли;
СвойстваДокумента = РезультатыЗапроса[1].Выбрать();
СвойстваДокумента.Следующий();
ТипыСообщений = РезультатыЗапроса[2].Выбрать();
ТипыСообщений.Следующий();
ФайлыСообщения = РезультатыЗапроса[3].Выгрузить();
СостоянияСообщений = РезультатыЗапроса[4].Выгрузить();
ИдентификаторыОснований = РезультатыЗапроса[5].Выгрузить();
ДанныеОбъектов = СинхронизацияЭДО.НовыеДанныеОбъектов();
ДанныеОбъекта = СинхронизацияЭДО.ДобавитьДанныеОбъекта(ДанныеОбъектов);
ЗаполнитьДанныеОбъектаДляПодготовкиКОтправке(ДанныеОбъекта, СвойстваДокумента, СообщениеОбъект,
ТипыСообщений.ТипСообщения, ФайлыСообщения, ИдентификаторыОснований, КонтекстДиагностики);
Если ДанныеОбъекта.Отказ Тогда
ОтменитьТранзакцию();
Возврат Результат;
КонецЕсли;
РезультатПодготовки = СинхронизацияЭДО.ПодготовитьОбъектыКОтправке(ДанныеОбъектов, КонтекстДиагностики);
ПричинаОстановки = Перечисления.ПричиныОстановкиЭДО.ПустаяСсылка();
Если РезультатПодготовки.Отказ Тогда
Если ДанныеОбъекта.ТипОшибкиОтправки = СинхронизацияЭДО.ТипыОшибокОтправки().ОжидаетсяОтветНаПриглашение Тогда
ПричинаОстановки = Перечисления.ПричиныОстановкиЭДО.ОжидаетсяОтветНаПриглашение;
Иначе
ОтменитьТранзакцию();
Возврат Результат;
КонецЕсли;
КонецЕсли;
Комментарий = КомментарийКСостояниюДокумента(РезультатыЗапроса[6]);
Если ЗначениеЗаполнено(ПричинаОстановки) Тогда
ОстановитьДокументПриПодготовкиКОтправке(СообщениеОбъект, ПричинаОстановки, СостоянияСообщений,
КонтекстДиагностики, Комментарий);
Иначе
ОтметитьПодготовкуСообщенияКОтправке(СообщениеОбъект, СвойстваДокумента, СостоянияСообщений,
КонтекстДиагностики, Комментарий);
КонецЕсли;
ОбработанныеСообщения.Добавить(СообщениеОбъект);
Результат = Истина;
ЗафиксироватьТранзакцию();
Исключение
ОтменитьТранзакцию();
ТекстОшибки = ПодробноеПредставлениеОшибки(ИнформацияОбОшибке());
ДобавитьОшибкуВыполненияДействияПоСообщению(Сообщение, Действие, ТекстОшибки, КонтекстДиагностики);
КонецПопытки;
Возврат Результат;
КонецФункции
- У меня типовой код и я не хочу рефакторить за 1С.
В этом и суть ишуза, считаю.
// не хочу рефакторить за 1С
// BSLLS:PairingBrokenTransaction-off
Функция ПодготовитьКОтправкеДокумент(Сообщение, КонтекстДиагностики, СообщенияДляОбработки, ОбработанныеСообщения, ИдентификаторПакета = Неопределено)
КонецФункции
// BSLLS:PairingBrokenTransaction-on
Нет суть ишшуза в том, что
начатьТранзакцию();
Попытка
//нечто
Если нечто Тогда
ЗафиксироватьТранзацкцию();
Возврат;
КонецЕсли
//Еще нечто
Если ЕщеНечто Тогда
ОтменитьТранзакцию();
Возврат;
КонецЕсли;
// продолжаем кодить
ЗафиксироватьТранзакцию();
Исключение
ОтменитьТранзакцию();
ВызватьИсключение;
КонецПопытки;
это такой же допустимый код, как и просто
Процедура ЭтоНечто()
//нечто
Если нечто Тогда
Возврат;
КонецЕсли
//Еще нечто
Если ЕщеНечто Тогда
Возврат;
КонецЕсли;
// продолжаем кодить
КонецПроцедуры
Так как альтернатива не проходит когнитивку
Процедура ЭтоНечто()
//нечто
Если не нечто Тогда
//Еще нечто
Если не ЕщеНечто Тогда
// продолжаем кодить
КонецЕсли;
КонецЕсли;
КонецПроцедуры
Не воспринимайте это правило как однозначный закон ПДД или категоричное правило. Оно всего лишь подсвечивает code smells, и с этим успешно справляется.
Вы же хотите признания ваших допущений в коде - хотя вы сами и без правила видите что приведенный кусок кода сложно читать.
С другой стороны алерты на типовой код, который нет объективных (а не стилистических) причин рефакторить - это мозоль которую не надо трогать bslls дабы у пользователей не подгорало.
Но особенносить линтера такова, что при правке пары строк - анализируется вся процедура. Поэтому предлагаю сместить акцент.
Не подсвечивать участок кода как жестокая ошибка - если он был написан не вами и такие ошибки уже присутствовали. Перенеся ответственность с программиста на легаси код.
Ну я вижу тут явную ошибку по сравнению с тем, что пишет проверка. Для каждой Зафиксировать/ОтменитьТранзакцию я могу ткнуть пальцем в парную НачатьТранзакцию, а правило этого не видит. И кстати, если я приведенный кусок кода перепишу так, что правило ругаться не будет - читать будет еще хуже. А ОтменитьТранзакцию неожиданно на текущем наборе правил использовать вне блока Исключение оказывается нельзя, а это очевидно не так.
А ОтменитьТранзакцию неожиданно на текущем наборе правил использовать вне блока Исключение оказывается нельзя, а это очевидно не так.
Потому что может рухнуть буквально что угодно и до отмены транзакции не дойдёт.
Чиво? я же не предлагаю выкинуть отмену транзации из блока исключения. Я просто самостоятельно, без выбрасывания ошибки хочу отменить транзакцию и уйти из процедуры. Почему нельзя?
@nixel2007 мне эти ограничения напоминают ограничения ФП языка типа хаскеля)
Если КоличествоСтрокТЧ = 1 Тогда
НачатьТранзакцию();
Попытка
БлокировкаДанных = Новый БлокировкаДанных;
ЭлементБлокировкиДанных = БлокировкаДанных.Добавить("РегистрСведений.ПризнакиАВСБезЗаявки");
ЭлементБлокировкиДанных.УстановитьЗначение("ИдентификаторЗаявки", idЗаявки);
ЭлементБлокировкиДанных.Режим = РежимБлокировкиДанных.Исключительный;
БлокировкаДанных.Заблокировать();
Запрос = Новый Запрос;
Запрос.Текст =
"ВЫБРАТЬ
| ПризнакиАВСБезЗаявки.ИдентификаторЗаявки,
| ПризнакиАВСБезЗаявки.ПризнакАВС
|ИЗ
| РегистрСведений.ПризнакиАВСБезЗаявки КАК ПризнакиАВСБезЗаявки
|ГДЕ
| ПризнакиАВСБезЗаявки.ИдентификаторЗаявки = &idЗаявки";
Запрос.УстановитьПараметр("idЗаявки", idЗаявки);
РезультатЗапроса = Запрос.Выполнить();
ВыборкаДетальныеЗаписи = РезультатЗапроса.Выбрать();
Если ВыборкаДетальныеЗаписи.Следующий() Тогда
МенеджерЗаписи = РегистрыСведений.ДополнительныеДанныеЗаявкиНаВозврат.СоздатьМенеджерЗаписи();
МенеджерЗаписи.Заявка = ЗаявкаНаВозврат.Ссылка;
МенеджерЗаписи.Номенклатура = Номенклатура;
МенеджерЗаписи.Бренд = Номенклатура.Бренд;
МенеджерЗаписи.ПризнакАВС = ВыборкаДетальныеЗаписи.ПризнакАВС;
МенеджерЗаписи.Записать();
МенеджерЗаписи = РегистрыСведений.ПризнакиАВСБезЗаявки.СоздатьМенеджерЗаписи();
МенеджерЗаписи.ИдентификаторЗаявки = idЗаявки;
МенеджерЗаписи.Удалить();
ЗафиксироватьТранзакцию();
Иначе
ОтменитьТранзакцию();
КонецЕсли;
Исключение
ОтменитьТранзакцию();
ЗаписьЖурналаРегистрации("ru = 'ДобавлениеПризнакаАВСПриСозданииЗаявки'",
УровеньЖурналаРегистрации.Ошибка,
,
ЗаявкаНаВозврат.Ссылка,
ПодробноеПредставлениеОшибки(ИнформацияОбОшибке()));
КонецПопытки;
КонецЕсли;
КонецПроцедуры
вот ещё пример, кто-то написал такой код
как аккуратно привести его в нормальному виду?