правило UsageWriteLogEventWhenProcessingException

тесты + документация + precommit
artbear · Dec 4, 2023 · 7f6617f · 7f6617f
1 parent 1c83f52
commit 7f6617f
Show file tree

Hide file tree

Showing 8 changed files with 545 additions and 0 deletions.
diff --git a/docs/diagnostics/UsageWriteLogEventWhenProcessingException.md b/docs/diagnostics/UsageWriteLogEventWhenProcessingException.md
@@ -0,0 +1,157 @@
+# Использование ЗаписьЖурналаРегистрации внутри блока Исключение (UsageWriteLogEventWhenProcessingException)
+
+<!-- Блоки выше заполняются автоматически, не трогать -->
+
+## Описание диагностики
+
+<!-- Описание диагностики заполняется вручную. Необходимо понятным языком описать смысл и схему работу -->
+Если имеется некоторая серверная бизнес-логика, которая вызывается с клиента при интерактивной работе пользователя:
+
+```
+&НаСервере
+Процедура ВыполнитьОперацию()
+ // код, приводящий к вызову исключения
+ ....
+КонецПроцедуры
+```
+
+то неправильно маскировать от пользователя и администратора исходную проблему:
+
+```bsl
+// на клиенте
+Попытка
+    ВыполнитьОперацию();
+Исключение
+    ПоказатьПредупреждение(,НСтр("ru = 'Операция не может быть выполнена.'"));
+КонецПопытки;
+```
+
+Правильно записывать в журнал регистрации подробное представление исключения, а краткое представление добавлять в текст
+сообщения пользователю:
+
+```bsl
+&НаСервере
+Процедура ВыполнитьОперацию()
+    Попытка
+        // код, приводящий к вызову исключения
+        ....
+    Исключение
+        // Запись события в журнал регистрации для системного администратора.
+        ЗаписьЖурналаРегистрации(НСтр("ru = 'Выполнение операции'"),
+            УровеньЖурналаРегистрации.Ошибка,,,
+            ОбработкаОшибок.ПодробноеПредставлениеОшибки(ИнформацияОбОшибке()));
+        ВызватьИсключение;
+    КонецПопытки;
+КонецПроцедуры
+```
+
+и тогда на клиенте:
+
+```bsl
+Попытка
+    ВыполнитьОперацию();
+Исключение
+    ТекстСообщения = КраткоеПредставлениеОшибки(ИнформацияОбОшибке());
+    ПоказатьПредупреждение(,НСтр("ru = 'Операция не может быть выполнена по причине:'") + Символы.ПС + ТекстСообщения);
+КонецПопытки;
+```
+
+Не следует использовать функцию **ОписаниеОшибки**, т.к. она неинформативна для разработчика, потому что не возвращает
+стек в тексте ошибки.
+
+3.3. Если имеется некоторая клиентская бизнес-логика (код выполняется полностью на клиенте):
+
+```bsl
+&НаКлиенте
+Процедура СоздатьФайлНаДиске()
+    // код, приводящий к вызову исключения
+    ....
+КонецПроцедуры
+```
+
+то рекомендуется делать дополнительный серверный вызов для протоколирования неудачного результата операции в журнал
+регистрации:
+
+```bsl
+Попытка
+    // клиентский код, приводящий к вызову исключения
+    СоздатьФайлНаДиске();
+Исключение
+    ТекстСообщения = КраткоеПредставлениеОшибки(ИнформацияОбОшибке());
+    ПоказатьПредупреждение(,НСтр("ru = 'Операция не может быть выполнена по причине:'") + Символы.ПС + ТекстСообщения);
+    ЗаписатьОшибкуРаботыСФайлами(ОбработкаОшибок.ПодробноеПредставлениеОшибки(ИнформацияОбОшибке())));
+КонецПопытки;
+
+&НаСервереБезКонтекста
+Процедура ЗаписатьОшибкуРаботыСФайлами(...)
+    ЗаписьЖурналаРегистрации(НСтр("ru = 'Выполнение операции'"),
+        УровеньЖурналаРегистрации.Ошибка,,,
+        ПодробноеПредставлениеОшибки);
+КонецПроцедуры
+```
+
+3.4. Недопустимо перехватывать любые исключения, бесследно для системного администратора:
+
+```bsl
+Попытка
+    // код, приводящий к вызову исключения
+    ....
+Исключение // перехват любых исключений
+КонецПопытки;
+```
+
+Как правило, подобная конструкция скрывает реальную проблему, которую впоследствии невозможно диагностировать.
+Правильно:
+
+```bsl
+Попытка
+    // код, приводящий к вызову исключения
+    ....
+Исключение
+    // Пояснение причин перехвата всех исключений "незаметно" от пользователя.
+    // ....
+    // И запись события в журнал регистрации для системного администратора.
+    ЗаписьЖурналаРегистрации(НСтр("ru = 'Выполнение операции'"),
+        УровеньЖурналаРегистрации.Ошибка,,,
+        ОбработкаОшибок.ПодробноеПредставлениеОшибки(ИнформацияОбОшибке()));
+КонецПопытки;
+```
+
+## Примеры
+
+<!-- В данном разделе приводятся примеры, на которые диагностика срабатывает, а также можно привести пример, как можно исправить ситуацию -->
+Пример удаления временного файла:
+
+```bsl
+ИмяПромежуточногоФайла = ПолучитьИмяВременногоФайла("xml");
+Данные.Записать(ИмяПромежуточногоФайла);
+
+// Работа с файлом
+...
+
+// Удаляем временный файл
+Попытка
+   УдалитьФайлы(ИмяПромежуточногоФайла);
+Исключение
+   ЗаписьЖурналаРегистрации(НСтр("ru = 'Мой механизм.Действие'"), УровеньЖурналаРегистрации.Ошибка, , , ОбработкаОшибок.ПодробноеПредставлениеОшибки(ИнформацияОбОшибке()));
+КонецПопытки;
+```
+
+## Источники
+
+<!-- Необходимо указывать ссылки на все источники, из которых почерпнута информация для создания диагностики -->
+<!-- Примеры источников
+
+* Источник: [Стандарт: Тексты модулей](https://its.1c.ru/db/v8std#content:456:hdoc)
+* Полезная информация: [Отказ от использования модальных окон](https://its.1c.ru/db/metod8dev#content:5272:hdoc)
+* Источник: [Cognitive complexity, ver. 1.4](https://www.sonarsource.com/docs/CognitiveComplexity.pdf) -->
+
+* Источник: [Стандарт: Перехват исключений в коде](https://its.1c.ru/db/v8std#content:499:hdoc)
+* Источник: [Стандарт: Доступ к файловой системе из кода конфигурации](https://its.1c.ru/db/v8std/content/542/hdoc)
+*
+
+Источник: [1C:Fresh - Рекомендации по подготовке расширений конфигурации, дополнительных отчетов и обработок](https://1cfresh.com/articles/so_addprocess_fastaudit#misc)
+
+*
+
+Источник: [Описание правила "Конструкция Попытка...Исключение...КонецПопытки не содержит кода в исключении" (MissingCodeTryCatchEx)](https://1c-syntax.github.io/bsl-language-server/diagnostics/MissingCodeTryCatchEx)
diff --git a/docs/en/diagnostics/UsageWriteLogEventWhenProcessingException.md b/docs/en/diagnostics/UsageWriteLogEventWhenProcessingException.md
@@ -0,0 +1,20 @@
+# Using "WriteLogEvent" inside the block "Exception" (UsageWriteLogEventWhenProcessingException)
+
+<!-- Блоки выше заполняются автоматически, не трогать -->
+
+## Description
+
+<!-- Описание диагностики заполняется вручную. Необходимо понятным языком описать смысл и схему работу -->
+
+## Examples
+
+<!-- В данном разделе приводятся примеры, на которые диагностика срабатывает, а также можно привести пример, как можно исправить ситуацию -->
+
+## Sources
+
+<!-- Необходимо указывать ссылки на все источники, из которых почерпнута информация для создания диагностики -->
+<!-- Примеры источников
+
+* Источник: [Стандарт: Тексты модулей](https://its.1c.ru/db/v8std#content:456:hdoc)
+* Полезная информация: [Отказ от использования модальных окон](https://its.1c.ru/db/metod8dev#content:5272:hdoc)
+* Источник: [Cognitive complexity, ver. 1.4](https://www.sonarsource.com/docs/CognitiveComplexity.pdf) -->
diff --git a/...x/bsl/languageserver/diagnostics/UsageWriteLogEventWhenProcessingExceptionDiagnostic.java b/...x/bsl/languageserver/diagnostics/UsageWriteLogEventWhenProcessingExceptionDiagnostic.java
@@ -0,0 +1,215 @@
+/*
+ * This file is a part of BSL Language Server.
+ *
+ * Copyright (c) 2018-2023
+ * Alexey Sosnoviy <[email protected]>, Nikita Fedkin <[email protected]> and contributors
+ *
+ * SPDX-License-Identifier: LGPL-3.0-or-later
+ *
+ * BSL Language Server is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3.0 of the License, or (at your option) any later version.
+ *
+ * BSL Language Server is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with BSL Language Server.
+ */
+package com.github._1c_syntax.bsl.languageserver.diagnostics;
+
+import com.github._1c_syntax.bsl.languageserver.context.DocumentContext;
+import com.github._1c_syntax.bsl.languageserver.context.symbol.MethodSymbol;
+import com.github._1c_syntax.bsl.languageserver.context.symbol.Symbol;
+import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticMetadata;
+import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticSeverity;
+import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticTag;
+import com.github._1c_syntax.bsl.languageserver.diagnostics.metadata.DiagnosticType;
+import com.github._1c_syntax.bsl.languageserver.references.ReferenceIndex;
+import com.github._1c_syntax.bsl.languageserver.references.model.Reference;
+import com.github._1c_syntax.bsl.languageserver.utils.Ranges;
+import com.github._1c_syntax.bsl.parser.BSLParser;
+import com.github._1c_syntax.utils.CaseInsensitivePattern;
+import org.antlr.v4.runtime.tree.ParseTree;
+import org.eclipse.lsp4j.Range;
+import org.eclipse.lsp4j.SymbolKind;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.function.Predicate;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+@DiagnosticMetadata(
+  type = DiagnosticType.CODE_SMELL,
+  severity = DiagnosticSeverity.MAJOR,
+  minutesToFix = 5,
+  tags = {
+    DiagnosticTag.BADPRACTICE,
+    DiagnosticTag.SUSPICIOUS,
+  }
+
+)
+public class UsageWriteLogEventWhenProcessingExceptionDiagnostic extends AbstractVisitorDiagnostic {
+  private static final Pattern WRITELOGEVENT = CaseInsensitivePattern.compile(
+    "записьжурналарегистрации|writelogevent"
+  );
+  private final ReferenceIndex referenceIndex;
+  private final Set<String> methodsWithRequiredCall = new HashSet<>();
+  private final List<CodeBlockData> blocksForChecking = new ArrayList<>();
+  //  private boolean insideException;
+  private boolean hasRequiredCallInsideExceptBlock;
+  private String currentMethodName;
+  private List<Reference> referenceToMethodCalls = Collections.emptyList();
+
+  public UsageWriteLogEventWhenProcessingExceptionDiagnostic(ReferenceIndex referenceIndex) {
+    this.referenceIndex = referenceIndex;
+  }
+
+  private static boolean isRightMethod(BSLParser.GlobalMethodCallContext context) {
+    return WRITELOGEVENT.matcher(context.methodName().getText()).matches();
+  }
+
+  private static boolean isMethodSymbol(Symbol symbol) {
+    return symbol.getSymbolKind() == SymbolKind.Method;
+  }
+
+  @Override
+  public ParseTree visitProcedure(BSLParser.ProcedureContext ctx) {
+    currentMethodName = ctx.procDeclaration().subName().getText();
+    return super.visitProcedure(ctx);
+  }
+
+  @Override
+  public ParseTree visitFunction(BSLParser.FunctionContext ctx) {
+    currentMethodName = ctx.funcDeclaration().subName().getText();
+    return super.visitFunction(ctx);
+  }
+
+  @Override
+  public ParseTree visitExceptCodeBlock(BSLParser.ExceptCodeBlockContext ctx) {
+//    insideException = true; // для отбрасывания ненужных узлов
+    hasRequiredCallInsideExceptBlock = false;
+
+    // TODO проверить вложенные блоки Попытка-Попытка-Исключение-КонецПопытки-Исключение-КонецПопытки
+    super.visitExceptCodeBlock(ctx); // обработка всех вложенных вызовов
+//    insideException = false;
+
+    if (!hasRequiredCallInsideExceptBlock) {
+      if (!hasRequiredCallInsideMethods(Ranges.create(ctx))) {
+        fireIssue(ctx);
+      } else {
+        blocksForChecking.add(new CodeBlockData(documentContext, ctx, getReferenceToMethodCalls()));
+      }
+      hasRequiredCallInsideExceptBlock = false;
+    }
+
+    return super.defaultResult(); // внутрь вызова уже сходили
+  }
+
+  private boolean hasRequiredCallInsideMethods(Range range) {
+    return getReferenceToMethodCalls().stream()
+      .anyMatch(reference -> Ranges.containsRange(range, reference.getSelectionRange()));
+  }
+
+  private List<Reference> getReferenceToMethodCalls() {
+    if (referenceToMethodCalls.isEmpty()) {
+      referenceToMethodCalls = referenceIndex.getReferencesFrom(documentContext.getUri()).stream()
+        .filter(reference -> isMethodSymbol(reference.getSymbol()))
+        .collect(Collectors.toList());
+    }
+    return referenceToMethodCalls;
+  }
+
+  private void fireIssue(BSLParser.ExceptCodeBlockContext ctx) {
+    final var tryStatementContext = (BSLParser.TryStatementContext) ctx.getParent();
+    diagnosticStorage.addDiagnostic(tryStatementContext.ENDTRY_KEYWORD());
+  }
+
+  @Override
+  public ParseTree visitGlobalMethodCall(BSLParser.GlobalMethodCallContext context) {
+    if (!hasRequiredCallInsideExceptBlock && isRightMethod(context)) {
+      hasRequiredCallInsideExceptBlock = true;
+      methodsWithRequiredCall.add(currentMethodName);
+    }
+    return super.defaultResult(); // внутрь вызова нет смысла идти
+  }
+
+  @Override
+  public ParseTree visitFile(BSLParser.FileContext ctx) {
+//    referenceMethodCalls = Collections.emptyList();
+
+    super.visitFile(ctx); // обработка всего модуля - получение всех вызовов ЗаписьЖурналаРегистрации
+
+    blocksForChecking.forEach(this::checkCodeBlock);
+
+    referenceToMethodCalls = Collections.emptyList();
+    blocksForChecking.clear();
+    methodsWithRequiredCall.clear();
+
+    return super.defaultResult();
+  }
+
+  private void checkCodeBlock(CodeBlockData dto) {
+    if (hasRequiredCallInsideLocalMethods(dto.invokedLocalMethods.stream())) {
+      return;
+    }
+    final var hasRequiredCallInsideLocalMethods = dto.invokedLocalMethods.stream()
+      .anyMatch(this::hasRequiredCallForFirstLevelOfNestingMethods);
+    if (hasRequiredCallInsideLocalMethods) {
+      return;
+    }
+    fireIssue(dto.context);
+  }
+
+  private boolean hasRequiredCallInsideLocalMethods(Stream<MethodSymbol> stream) {
+    return stream
+      .anyMatch(methodSymbol -> methodsWithRequiredCall.contains(methodSymbol.getName()));
+  }
+
+  private boolean hasRequiredCallForFirstLevelOfNestingMethods(MethodSymbol methodSymbol) {
+    final var methodSymbolStream = referenceIndex.getReferencesFrom(methodSymbol).stream()
+      .map(Reference::getSymbol)
+      .filter(UsageWriteLogEventWhenProcessingExceptionDiagnostic::isMethodSymbol)
+      .filter(MethodSymbol.class::isInstance)
+      .map(MethodSymbol.class::cast);
+    return hasRequiredCallInsideLocalMethods(methodSymbolStream);
+  }
+
+  private static class CodeBlockData {
+    final BSLParser.ExceptCodeBlockContext context;
+    final Collection<MethodSymbol> invokedLocalMethods;
+    final Collection<MethodSymbol> invokedExternalMethods;
+
+    public CodeBlockData(DocumentContext documentContext, BSLParser.ExceptCodeBlockContext context,
+                         List<Reference> referenceMethodCalls) {
+      this.context = context;
+
+      final Predicate<Reference> referencePredicate = reference -> reference.getUri().equals(documentContext.getUri());
+
+      final var localReferenceStream = referenceMethodCalls.stream().filter(referencePredicate);
+      this.invokedLocalMethods = getMethodList(context, localReferenceStream);
+
+      final var externalReferenceStream = referenceMethodCalls.stream().filter(referencePredicate.negate());
+      this.invokedExternalMethods = getMethodList(context, externalReferenceStream);
+    }
+
+    private static List<MethodSymbol> getMethodList(BSLParser.ExceptCodeBlockContext exceptCodeBlockContext,
+                                                    Stream<Reference> referenceStream) {
+      return referenceStream
+        .filter(reference -> Ranges.containsRange(Ranges.create(exceptCodeBlockContext), reference.getSelectionRange()))
+        .map(Reference::getSymbol)
+        .filter(MethodSymbol.class::isInstance)
+        .map(MethodSymbol.class::cast)
+        .collect(Collectors.toList());
+    }
+  }
+}