-
Notifications
You must be signed in to change notification settings - Fork 54
#163 В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL. #1153
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
base: master
Are you sure you want to change the base?
#163 В запросе отсутствует проверка на NULL для поля, которое может потенциально содержать NULL. #1153
Changes from all commits
b547c5f
2c0823f
a4c994b
78d0085
4511f76
f30a194
e5f4c09
575cb5b
9b24531
d7193dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,197 @@ | ||
/******************************************************************************* | ||
* 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.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. монитор нужно проверять в циклах и после выполнения тяжелых действий - см. код ниже |
||
|
||
List<CommonExpression> orderFields = getOrderFields(sourceTable); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. если тут вернули пустую коллекцию - то дальше можно не продолжать. |
||
List<QuerySchemaOperator> operators = sourceTable.getOperators(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Если оператор выборки 1 единственный - дальше можно не продолжать |
||
for (QuerySchemaOperator operator : operators) | ||
{ | ||
List<QuerySchemaExpression> fields = operator.getSelectFields(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
List<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, | ||
List<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(List<CommonExpression> orderFields, CommonExpression commonExpression, | ||
String fieldName) | ||
{ | ||
String paramName = commonExpression.getFullContent(); | ||
List<CommonExpression> unique = new ArrayList<>(); | ||
unique.addAll(orderFields); | ||
for (CommonExpression orderField : unique) | ||
{ | ||
String fieldFullName = orderField.getFullContent(); | ||
if (paramName.equals(fieldFullName) || fieldName.equals(fieldFullName)) | ||
{ | ||
orderFields.remove(orderField); | ||
} | ||
} | ||
} | ||
|
||
private List<CommonExpression> getOrderFields(QuerySchemaSelectQuery sourceTable) | ||
{ | ||
List<QuerySchemaOrderExpression> orderExpressions = sourceTable.getOrderExpressions(); | ||
List<CommonExpression> orderFields = new ArrayList<>(); | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Это видимо код который хотел делать через параметры но забыл? надо переделать на две константы |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
ВЫБРАТЬ | ||
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка, | ||
ЕСТЬNULL(ЗапасыОстатки.КоличествоОстаток, 0) КАК КоличествоОстаток | ||
ИЗ | ||
Справочник.Номенклатура КАК СправочникНоменклатура | ||
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК | ||
ЗапасыОстатки | ||
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка) | ||
|
||
УПОРЯДОЧИТЬ ПО | ||
ЗапасыОстатки.КоличествоОстаток; | ||
|
||
ВЫБРАТЬ | ||
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка, | ||
ЕСТЬNULL(ЗапасыОстатки.КоличествоОстаток, 0) КАК КоличествоОстаток | ||
ИЗ | ||
Справочник.Номенклатура КАК СправочникНоменклатура | ||
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК | ||
ЗапасыОстатки | ||
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка) | ||
|
||
УПОРЯДОЧИТЬ ПО | ||
КоличествоОстаток; | ||
|
||
ВЫБРАТЬ | ||
СправочникНоменклатура.Ссылка КАК НоменклатураСсылка, | ||
ВЫБОР КОГДА ЗапасыОстатки.КоличествоОстаток ЕСТЬ НЕ NULL ТОГДА 0 ИНАЧЕ ЗапасыОстатки.КоличествоОстаток КАК КоличествоОстаток | ||
ИЗ | ||
Справочник.Номенклатура КАК СправочникНоменклатура | ||
ЛЕВОЕ СОЕДИНЕНИЕ РегистрНакопления.Запасы.Остатки КАК | ||
ЗапасыОстатки | ||
ПО (ЗапасыОстатки.Номенклатура = СправочникНоменклатура.Ссылка) | ||
|
||
УПОРЯДОЧИТЬ ПО | ||
КоличествоОстаток |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
нужно пустую строку после подзаголовка