Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#163 В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL. #1153

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- Добавление типизированного значения в не типизированную коллекцию

#### Запросы
- В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL
Comment on lines 22 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

нужно пустую строку после подзаголовка



#### Права ролей
Expand Down
50 changes: 50 additions & 0 deletions bundles/com.e1c.v8codestyle.ql/markdown/ql-query-field-isnull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# The query is missing a NULL check for a field that could potentially contain NULL.

2. When you order a query result by fields that might contain NULL, take into account that the sorting priority of NULL varies in different DBMS.

## Noncompliant Code Example

```bsl
SELECT
CatalogProducts.Ref AS ProductRef,
InventoryBalance.QuantityBalance AS QuantityBalance
FROM
Catalog.Products AS CatalogProducts
LEFT JOIN AccumulationRegister.Inventory.Balance AS InventoryBalance
BY (InventoryBalance.Products = CatalogProducts.Ref)

ORDER BY
QuantityBalance
```

## Compliant Solution

```bsl
SELECT
CatalogProducts.Ref AS ProductRef,
ISNULL(InventoryBalance.QuantityBalance, 0) AS QuantityBalance
FROM
Catalog.Products AS CatalogProducts
LEFT JOIN AccumulationRegister.Inventory.Balance AS InventoryBalance
BY (InventoryBalance.Products = CatalogProducts.Ref)

ORDER BY
QuantityBalance;

SELECT
Comment on lines +31 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут надо разбить на 2 примера - это же не один запрос ведь...

CatalogProducts.Ref AS ProductRef,
CASE WHEN InventoryBalance.QuantityBalance IS NOT NULL THEN 0 ELSE InventoryBalance.QuantityBalance AS QuantityBalance
FROM
Catalog.Products AS CatalogProducts
LEFT JOIN AccumulationRegister.Inventory.Balance AS InventoryBalance
BY (InventoryBalance.Products = CatalogProducts.Ref)

ORDER BY
QuantityBalance;
```

## See

- [Ordering query results](https://support.1ci.com/hc/en-us/articles/360011120859-Ordering-query-results)
- [Using the CASE operation](https://support.1ci.com/hc/en-us/articles/360007870794-Query-language#using_the_isnull___function)
- [Appendix 8. Features of operating with different DBMS](https://support.1ci.com/hc/en-us/articles/6347699838098-8-3-IBM-Db2)
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL.

1.2. При сортировке по полю запроса, которое может потенциально содержать NULL, следует учитывать, что в разных СУБД порядок сортировки по этому полю может отличаться.

## Неправильно

```bsl
ВЫБРАТЬ
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка,
ЗапасыОстатки.КоличествоОстаток КАК КоличествоОстаток
ИЗ
Справочник.Номенклатура КАК СправочникНоменклатура
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК
ЗапасыОстатки
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка)

УПОРЯДОЧИТЬ ПО
КоличествоОстаток
```

## Правильно

При использовании в тексте запроса оператора ПОДОБНО и СПЕЦСИМВОЛ допустимо использовать только константные строковые литералы или параметры запроса.

```bsl
ВЫБРАТЬ
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка,
ЕСТЬNULL(ЗапасыОстатки.КоличествоОстаток, 0) КАК КоличествоОстаток
ИЗ
Справочник.Номенклатура КАК СправочникНоменклатура
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК
ЗапасыОстатки
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка)

УПОРЯДОЧИТЬ ПО
КоличествоОстаток;

ВЫБРАТЬ
Comment on lines +35 to +38
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут надо разбить на 2 примера - это же не один запрос ведь...

СправочникНоменклатура.Ссылка КАК НоменклатураСсылка,
ВЫБОР КОГДА ЗапасыОстатки.КоличествоОстаток ЕСТЬ НЕ NULL ТОГДА 0 ИНАЧЕ ЗапасыОстатки.КоличествоОстаток КАК КоличествоОстаток
ИЗ
Справочник.Номенклатура КАК СправочникНоменклатура
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК
ЗапасыОстатки
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка)

УПОРЯДОЧИТЬ ПО
КоличествоОстаток;
```

## См.

- [Упорядочивание результатов запроса](https://its.1c.ru/db/v8std#content:412:hdoc:1.2)
- [Использование операции ВЫБОР](https://its.1c.ru/db/metod8dev/content/2653/hdoc)
- [Приложение 8. Особенности работы с различными СУБД.](http://its.1c.ru/db/v83doc#bookmark:dev:TI000001285)
4 changes: 4 additions & 0 deletions bundles/com.e1c.v8codestyle.ql/plugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
category="com.e1c.v8codestyle.ql"
class="com.e1c.v8codestyle.ql.check.ConstantsInBinaryOperationCheck">
</check>
<check
category="com.e1c.v8codestyle.ql"
class="com.e1c.v8codestyle.ql.check.QueryFieldIsNullCheck">
</check>
</extension>

</plugin>
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ final class Messages
public static String JoinToSubQuery_description;
public static String JoinToSubQuery_Query_join_to_sub_query_not_allowed;
public static String JoinToSubQuery_title;
public static String QueryFieldIsNullCheck_description;
public static String QueryFieldIsNullCheck_Query_missing_NULL_check_for_field_potentially_contain_NULL;
public static String QueryFieldIsNullCheck_title;
public static String TempTableHasIndex_description;
public static String TempTableHasIndex_Exclude_table_name_pattern;
public static String TempTableHasIndex_New_temporary_table_should_have_indexes;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
/*******************************************************************************
* Copyright (C) 2022, 1C-Soft LLC and others.
*
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* 1C-Soft LLC - initial API and implementation
* Denis Maslennikov - issue #163
*******************************************************************************/
package com.e1c.v8codestyle.ql.check;

import static com._1c.g5.v8.dt.ql.model.QlPackage.Literals.COMMON_EXPRESSION__CONTENT;

import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.emf.ecore.EObject;
import org.eclipse.xtext.EcoreUtil2;

import com._1c.g5.v8.dt.ql.model.AbstractExpression;
import com._1c.g5.v8.dt.ql.model.CaseBody;
import com._1c.g5.v8.dt.ql.model.CaseOperationExpression;
import com._1c.g5.v8.dt.ql.model.CommonExpression;
import com._1c.g5.v8.dt.ql.model.FunctionExpression;
import com._1c.g5.v8.dt.ql.model.FunctionInvocationExpression;
import com._1c.g5.v8.dt.ql.model.IsNullOperatorExpression;
import com._1c.g5.v8.dt.ql.model.MultiPartCommonExpression;
import com._1c.g5.v8.dt.ql.model.QuerySchemaExpression;
import com._1c.g5.v8.dt.ql.model.QuerySchemaOperator;
import com._1c.g5.v8.dt.ql.model.QuerySchemaOrderExpression;
import com._1c.g5.v8.dt.ql.model.QuerySchemaSelectQuery;
import com.e1c.g5.v8.dt.check.CheckComplexity;
import com.e1c.g5.v8.dt.check.ICheckParameters;
import com.e1c.g5.v8.dt.check.settings.IssueSeverity;
import com.e1c.g5.v8.dt.check.settings.IssueType;
import com.e1c.g5.v8.dt.ql.check.QlBasicDelegateCheck;
import com.e1c.v8codestyle.check.StandardCheckExtension;
import com.e1c.v8codestyle.internal.ql.CorePlugin;

/**
* This class checks if null is checked when sorting by query field.
*
* @author Denis Maslennikov
*/
public class QueryFieldIsNullCheck
extends QlBasicDelegateCheck
{

private static final String CHECK_ID = "ql-query-field-isnull"; //$NON-NLS-1$
private static final String METHOD_DELIMITER = ","; //$NON-NLS-1$
private static final String ISNULL_METHODS = "ISNULL,ЕСТЬNULL"; //$NON-NLS-1$

@Override
public String getCheckId()
{
return CHECK_ID;
}

@Override
protected void configureCheck(CheckConfigurer builder)
{
builder.title(Messages.QueryFieldIsNullCheck_title)
.description(Messages.QueryFieldIsNullCheck_description)
.complexity(CheckComplexity.NORMAL)
.severity(IssueSeverity.MINOR)
.issueType(IssueType.PORTABILITY)
.extension(new StandardCheckExtension(412, getCheckId(), CorePlugin.PLUGIN_ID))
.delegate(QuerySchemaSelectQuery.class);
}

@Override
protected void checkQlObject(EObject object, QueryOwner owner, IQlResultAcceptor resultAceptor,
ICheckParameters parameters, IProgressMonitor monitor)
{
QuerySchemaSelectQuery sourceTable = (QuerySchemaSelectQuery)object;

if (monitor.isCanceled())
{
return;
}
Comment on lines +83 to +86
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

монитор нужно проверять в циклах и после выполнения тяжелых действий - см. код ниже


Set<CommonExpression> orderFields = getOrderFields(sourceTable);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

если полей упорядочения нет - дальше можно не продолжать.

List<QuerySchemaOperator> operators = sourceTable.getOperators();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если оператор выборки 1 единственный - дальше можно не продолжать

for (QuerySchemaOperator operator : operators)
{
List<QuerySchemaExpression> fields = operator.getSelectFields();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

наверное нужно проверять только операторы с левым/правым соединением, или полным. Внутренние соединение как раз не может давать null в полях

for (QuerySchemaExpression field : fields)
{
AbstractExpression abstractExpression = field.getExpression();
String fieldName = field.getAlias();
if (abstractExpression instanceof FunctionInvocationExpression)
{
removeFieldsWithIsNullMethod((FunctionInvocationExpression)abstractExpression, orderFields,
fieldName);
}
if (abstractExpression instanceof CaseOperationExpression)
{
removeFieldsWithWhenExpression((CaseOperationExpression)abstractExpression, orderFields, fieldName);
}
}
}
for (CommonExpression orderField : orderFields)
{
String message = Messages.QueryFieldIsNullCheck_Query_missing_NULL_check_for_field_potentially_contain_NULL;
resultAceptor.addIssue(message, orderField, COMMON_EXPRESSION__CONTENT);
}
}

private void removeFieldsWithWhenExpression(CaseOperationExpression caseInvocationExpression,
Set<CommonExpression> orderFields, String fieldName)
{
List<CaseBody> caseBodies = caseInvocationExpression.getBody();
for (CaseBody caseBody : caseBodies)
{
AbstractExpression whenExpression = caseBody.getWhen();
if (whenExpression instanceof IsNullOperatorExpression)
{
IsNullOperatorExpression nullOperator = (IsNullOperatorExpression)whenExpression;
for (EObject operatorContent : nullOperator.eContents())
{
if (operatorContent instanceof CommonExpression)
{
removeFieldByName(orderFields, (CommonExpression)operatorContent, fieldName);
}
}
}
}
}

private void removeFieldsWithIsNullMethod(FunctionInvocationExpression functionInvocationExpression,
Set<CommonExpression> orderFields, String fieldName)
{
FunctionExpression f = functionInvocationExpression.getFunctionType();
List<String> isNullMethods = getIsNullMethods();
if (isNullMethods.contains(f.getName()))
{
List<AbstractExpression> params = functionInvocationExpression.getParams();

if (params.get(0) instanceof CommonExpression)
{
removeFieldByName(orderFields, (CommonExpression)params.get(0), fieldName);
}

}
}

private void removeFieldByName(Set<CommonExpression> orderFields, CommonExpression commonExpression,
String fieldName)
{
String paramName = commonExpression.getFullContent();
for (CommonExpression orderField : orderFields)
{
String fieldFullName = orderField.getFullContent();
if (paramName.equals(fieldFullName) || fieldName.equals(fieldFullName))
{
orderFields.remove(orderField);
}
}
}

private Set<CommonExpression> getOrderFields(QuerySchemaSelectQuery sourceTable)
{
List<QuerySchemaOrderExpression> orderExpressions = sourceTable.getOrderExpressions();
Set<CommonExpression> orderFields = new HashSet<>();

for (QuerySchemaOrderExpression orderExpression : orderExpressions)
{
List<CommonExpression> commonExpressions =
EcoreUtil2.getAllContentsOfType(orderExpression, CommonExpression.class);
orderFields.addAll(commonExpressions);
for (CommonExpression commonExpression : commonExpressions)
{
if (commonExpression instanceof MultiPartCommonExpression)
{
orderFields.remove(((MultiPartCommonExpression)commonExpression).getSourceTable());
}
Comment on lines +177 to +185
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а тут разве не более правильно сделать? вроде других вариантов нет, но при этом удаляешь 2 лишних цикла.

if (orderExpression.getItem().getExpression() instanceof CommonExpression)
{
  orderFields.add(orderExpression.getItem().getExpression())
 }

}
}
return orderFields;
}

private List<String> getIsNullMethods()
{
List<String> isNullMethods = Arrays.asList(ISNULL_METHODS.split(METHOD_DELIMITER));
isNullMethods.replaceAll(String::trim);
return isNullMethods;
Comment on lines +191 to +195
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это видимо код который хотел делать через параметры но забыл? надо переделать на две константы

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ JoinToSubQuery_description = Query join with sub query

JoinToSubQuery_title = Query join with sub query

QueryFieldIsNullCheck_description = The query is missing a NULL check for a field that could potentially contain NULL

QueryFieldIsNullCheck_Query_missing_NULL_check_for_field_potentially_contain_NULL = The query is missing a NULL check for a field that could potentially contain NULL
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут нужно вставить имя поля, иначе в тексте его не найти.


QueryFieldIsNullCheck_title = The query is missing a NULL check for a field that could potentially contain NULL

TempTableHasIndex_Exclude_table_name_pattern = Exclude table name pattern

TempTableHasIndex_New_temporary_table_should_have_indexes = New temporary table should have indexes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ JoinToSubQuery_description = Соединение запроса с подзап

JoinToSubQuery_title = Соединение запроса с подзапросом

QueryFieldIsNullCheck_description = В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL

QueryFieldIsNullCheck_Query_missing_NULL_check_for_field_potentially_contain_NULL = В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL

QueryFieldIsNullCheck_title = В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL

TempTableHasIndex_Exclude_table_name_pattern = Выражения исключения имени таблицы

TempTableHasIndex_New_temporary_table_should_have_indexes = Новая временная таблица должна содержать индексы
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
ВЫБРАТЬ
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка,
ЕСТЬNULL(ЗапасыОстатки.КоличествоОстаток, 0) КАК КоличествоОстаток
ИЗ
Справочник.Номенклатура КАК СправочникНоменклатура
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК
ЗапасыОстатки
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка)

УПОРЯДОЧИТЬ ПО
ЗапасыОстатки.КоличествоОстаток;

ВЫБРАТЬ
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка,
ЕСТЬNULL(ЗапасыОстатки.КоличествоОстаток, 0) КАК КоличествоОстаток
ИЗ
Справочник.Номенклатура КАК СправочникНоменклатура
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК
ЗапасыОстатки
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка)

УПОРЯДОЧИТЬ ПО
КоличествоОстаток;

ВЫБРАТЬ
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка,
ВЫБОР КОГДА ЗапасыОстатки.КоличествоОстаток ЕСТЬ НЕ NULL ТОГДА 0 ИНАЧЕ ЗапасыОстатки.КоличествоОстаток КАК КоличествоОстаток
ИЗ
Справочник.Номенклатура КАК СправочникНоменклатура
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК
ЗапасыОстатки
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка)

УПОРЯДОЧИТЬ ПО
КоличествоОстаток