Мы используем куки, чтобы пользоваться сайтом
было удобно.
Вебинар: Каждая идиома когда-то была проблемой - 11.09
Когда речь заходит об ORM, большинство C# разработчиков сразу вспоминают мощный Entity Framework Core или же легковесный Dapper. Но что насчёт RepoDB и SqlSugar? Несмотря на меньшую известность, эти ORM активно используются в реальных проектах и продолжают развиваться. Давайте посмотрим, какие проблемы удастся обнаружить в их исходном коде с помощью статического анализатора.
Сейчас трудно представить себе современное приложение без использования ORM. И не спроста. Эта технология облегчает жизнь разработчикам: позволяет избежать написания большого количества кода и необходимости работы с SQL.
Именно ORM отвечает за взаимодействие с базой данных: через него проходят запросы на чтение и запись, транзакции и преобразование объектов в SQL. Ошибка в таком инструменте может стоить дорого: от снижения производительности до некорректной работы приложения и даже проблем с безопасностью.
В мире .NET стандартом являются Entity Framework Core и Dapper. Однако есть большое количество менее популярных инструментов, которые находят применение в реальных проектах. Среди таких ORM — RepoDB и SqlSugar.
В этой статье мы посмотрим на них глазами статического анализатора PVS-Studio. Вместо сравнения возможностей API или производительности мы разберём исходный код проектов и поищем потенциальные ошибки и подозрительные конструкции.
Перейдём к обзору интересных частей кода SqlSugar. Исходники возьмём из этого коммита.
public Task<int> ExecuteCommandAsync(string sql, object parameters,
CancellationToken cancellationToken)
{
this.CancellationToken = CancellationToken;
return ExecuteCommandAsync(sql,parameters);
}Предупреждение PVS-Studio: V3005 The 'this.CancellationToken' variable is assigned to itself. AdoProvider.cs 1472
В автоматическое свойство CancellationToken записывается его собственное значение. Скорее всего, этому свойству необходимо присваивать значение параметра cancellationToken.
private static void AppColumns(SqlInfo result,
ISugarQueryable<object> queryable,
string columnName)
{
var selectPkName = queryable.SqlBuilder.GetTranslationColumnName(columnName);
if (result.IsSelectNav)
{
if ( result.SelectString != null
&& !result.SelectString
.ToLower()
.Contains($" {selectPkName.ToLower()}
AS {selectPkName.ToLower()}")) // <=
{
result.SelectString = result.SelectString + ","
+ (selectPkName + " AS " + selectPkName);
}
}
....
}Предупреждение PVS-Studio: V3122 Lowercase string is compared with a different mixed case string. NavigatManager.cs 1131
Давайте разбираться, в чём тут проблема. Сначала строку result.SelectString приводят к нижнему регистру с помощью метода ToLower, а затем проверяют, содержится ли в полученной строке $" {selectPkName.ToLower()} AS {selectPkName.ToLower()}". Можно заметить, что искомая подстрока содержит буквы в верхнем регистре (AS). Таким образом, метод Contains всегда будет возвращать false.
Чтобы исправить код, достаточно привести все символы к нижнему регистру:
!result.SelectString
.ToLower()
.Contains($" {selectPkName.ToLower()} as {selectPkName.ToLower()}"))public override string ToSqlString()
{
....
if (it.InsertServerTime || it.InsertSql.HasValue())
{
return GetDbColumn(it,null);
}
object value = null;
if (it.Value is DateTime)
{
....
}
else if ( it.Value is int
|| it.Value is long
|| it.Value is short // <=
|| it.Value is short // <=
|| it.Value is byte
|| it.Value is double)
{
return it.Value;
}
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'it.Value is short' to the left and to the right of the '||' operator. QuestDBInsertBuilder.cs 89
Дважды использовать подвыражение it.Value is short бессмысленно. Возможно, вместо short должен стоять какой-нибудь другой тип, например decimal или float.
public static Func<string, object> GetTypeConvert(object value)
{
if ( value is int
|| value is uint
|| value is int?
|| value is uint?)
{
return x => Convert.ToInt32(x);
}
else if ( value is short
|| value is ushort
|| value is short?
|| value is ushort?)
{
return x => Convert.ToInt16(x);
}
else if ( value is long
|| value is long? // <=
|| value is ulong?
|| value is long?) // <=
{
return x => Convert.ToInt64(x);
}
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'value is long?' to the left and to the right of the '||' operator. UtilMethods.cs 288
Здесь дважды написали условие value is long?. Исходя из кода выше можно предположить, что второе одинаковое условие следует заменить на value is ulong.
private string GetName(ExpressionParameter parameter,
MemberExpression expression,
bool? isLeft,
bool isSingle)
{
if (isSingle)
{
return GetSingleName(parameter, expression, IsLeft);
}
else
{
return GetMultipleName(parameter, expression, IsLeft);
}
}Предупреждение PVS-Studio: V3196 The 'isLeft' parameter is not utilized inside the method body, but an identifier with a similar name is used inside the same method. MemberExpressionResolve.cs 770
Параметр isLeft не был использован, однако в методе используется свойство, имя которого отличается от имени параметра только регистром первой буквы.
Возможно, вместо свойства IsLeft предполагалось использовать параметр isLeft метода GetName.
public string GetValue(Expression expression)
{
var numExp = (expression as MethodCallExpression).Arguments[0];
var num =1;
if (ExpressionTool.GetParameters(numExp).Any())
{
var copyContext = this.Context.GetCopyContextWithMapping();
copyContext.IsSingle = false;
copyContext.Resolve(numExp, ResolveExpressType.WhereMultiple);
copyContext.Result.GetString(); // <=
}
else
{
num = ExpressionTool.DynamicInvoke(numExp).ObjToInt();
}
var take = (expression as MethodCallExpression);
if (....)
{
return "TOP " + num;
}
else if (this.Context is OracleExpressionContext)
{
return (HasWhere ? "AND" : "WHERE") + " ROWNUM<=" + num;
}
else if (....)
{
return "limit " + num;
}
else if (this.Context.GetLimit() != null)
{
if (this?.Context?.Case != null)
{
this.Context.Case.HasWhere = this.HasWhere;
this.Context.Case.Num = num;
}
return this.Context.GetLimit();
}
else
{
return "limit " + num;
}
}Предупреждение PVS-Studio: V3010 The return value of function 'GetString' is required to be utilized. SubTake.cs 64
Анализатор сообщает, что возвращаемое значение метода GetString не используется. При этом по реализации GetString становится ясно, что метод только формирует и возвращает строковое представление _Result. Он не изменяет состояние объекта и не выполняет действий, результат которых использовался бы дальше в коде:
public string GetString()
{
if (_Result == null) return null;
if (IsUpper)
return
_Result.ToString()
.ToUpper()
.Replace(UtilConstants.ReplaceCommaKey,",")
.TrimEnd(',');
else
return _Result.ToString()
.Replace(UtilConstants.ReplaceCommaKey, ",")
.TrimEnd(',');
}Поэтому результат вызова GetString в текущем коде теряется. Вероятно, разработчик забыл использовать возвращаемое значение при вычислении num.
На это также указывает логика окружающего кода: во всех возвратах из метода так или иначе используется переменная num. Следуя этой логике, обе ветки первого условия должны вычислить, по сути, одно и то же значение, и записать его в переменную num.
Вероятно, корректный код должен выглядеть так:
copyContext.Resolve(numExp, ResolveExpressType.WhereMultiple);
num = copyContext.Result.GetString();Перейдём к обзору ошибок и странных мест в проекте RepoDB. Исходники взяты из этого коммита.
public static object GetValue(this ConditionalExpression expression)
{
var test = expression.Test.GetValue();
var trueValue = expression.IfTrue.GetValue();
if (expression.Test.NodeType == ExpressionType.Equal)
{
return test == trueValue ? trueValue : expression.IfFalse.GetValue();
}
else if (expression.Test.NodeType == ExpressionType.NotEqual)
{
return test != trueValue ? trueValue : expression.IfFalse.GetValue();
}
else if (expression.Test.NodeType > ExpressionType.GreaterThan)
{
....
}
else if (expression.Test.NodeType > ExpressionType.GreaterThanOrEqual)// <=
{
....
}
else if (expression.Test.NodeType > ExpressionType.LessThan) // <=
{
....
}
else if (expression.Test.NodeType > ExpressionType.LessThanOrEqual) // <=
{
....
}
throw new NotSupportedException(....);
}Предупреждения PVS-Studio:
В методе GetValue выражение expression.Test.NodeType последовательно сравнивается с различными значениями перечисления ExpressionType. Однако в последних четырёх проверках вместо оператора равенства используется оператор >.
Проблема заключается в том, что цепочка else if выполняется последовательно и значение элементов перечисления в нижних блоках больше, чем в верхних.
public enum ExpressionType
{
....
GreaterThan = 15,
GreaterThanOrEqual = 16,
....
LessThan = 20,
LessThanOrEqual = 21,
....
}Условие expression.Test.NodeType > ExpressionType.GreaterThan покрывает все последующие условия.
То есть, если expression.Test.NodeType > ExpressionType.GreaterThan будет true, то последующие условия не будут проверены, а если оно будет false, то и следующие условия будут false.
public override int GetHashCode()
{
// Make sure to return if it is already provided
if (this.hashCode != null)
{
return this.hashCode.Value;
}
// Get first the entity hash code
var hashCode = HashCode.Combine(base.GetHashCode(), Name, ".UpdateAll");
// Get the fields
if (Fields != null)
{
foreach (var field in Fields) // <=
{
hashCode = HashCode.Combine(hashCode, field);
}
}
// Get the qualifier <see cref="Field"/> objects
if (Fields != null) // <=
{
foreach (var field in Qualifiers)
{
hashCode = HashCode.Combine(hashCode, field);
}
}
....
}Предупреждение PVS-Studio: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'Qualifiers' variable should be used instead of 'Fields'. UpdateAllRequest.cs 124
Fields дважды проверяется на неравенство null. Скорее всего, во втором случае вместо Fields следует проверять Qualifiers, так как эта коллекция используется в foreach.
public static Task<int> UpdateAllAsync<TEntity>(....
Expression<Func<TEntity, object>> qualifiers,
....,
IEnumerable<Field> fields = null,
....)
where TEntity : class
{
return UpdateAllAsyncInternal<TEntity>(connection: connection,
tableName: tableName,
entities: entities,
qualifiers: fields, // <=
batchSize: batchSize,
fields: fields, // <=
hints: hints,
commandTimeout: commandTimeout,
traceKey: traceKey,
transaction: transaction,
trace: trace,
statementBuilder: statementBuilder,
cancellationToken: cancellationToken);
}Предупреждение PVS-Studio: V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. UpdateAll.cs 619
Здесь в метод UpdateAllAsyncInternal дважды передаётся параметр fields, а параметр qualifiers, в свою очередь, не используется.
Возможно, предполагалось передавать qualifiers следующим образом:
qualifiers: Field.Parse<TEntity>(qualifiers)public override string CreateBatchQuery(string tableName,
IEnumerable<Field> fields,
int page,
int rowsPerBatch,
IEnumerable<OrderField> orderBy = null,
QueryGroup where = null,
string hints = null)
{
// Ensure with guards
GuardTableName(tableName);
// Validate the hints
GuardHints(hints);
// There should be fields
if (fields?.Any() != true)
{
throw new MissingFieldsException(fields?.Select(f => f.Name));
}
....
}Предупреждение PVS-Studio: V3191 Iteration through the 'fields' collection makes no sense because it is always empty. SqlServerStatementBuilder.cs 70
Условие fields?.Any() != true будет истинным в том случае, если коллекция fields пустая или null. В таком случае, fields?.Select(f => f.Name) не имеет смысла, поскольку всегда будет возвращать пустую коллекцию или null.
public override Guid GetGuid(int i)
{
ThrowExceptionIfNotAvailable();
return Guid.Parse(GetValue(i)?.ToString());
}Предупреждение PVS-Studio: V3105 The result of null-conditional operator is passed as the first argument to the 'Parse' method and is not expected to be null. DataEntityDataReader.cs 405
Тут GetValue(i) может вернуть null, и из-за оператора ? этот null будет передан в метод Guid.Parse. В свою очередь метод Guid.Parse при передаче аргумента null выбросит исключение ArgumentNullException.
public static void AssertMembersEquality(
object obj,
IDictionary<string, object> dictionary)
{
....
var value1 = property.GetValue(obj);
var value2 = dictionary[property.Name];
if (value1 is byte[] b1 && value2 is byte[] b2)
{
....
else
{
var propertyType = property.PropertyType.GetUnderlyingType();
if (propertyType == typeof(TimeSpan) && value2 is DateTime dateTime)
{
value2 = dateTime.TimeOfDay;
}
else if (propertyType == typeof(string) && value2 is DateTime)
{
value1 = DateTime.Parse(value1?.ToString()); // <=
}
....
}Предупреждение PVS-Studio: V3105 The result of null-conditional operator is passed as the first argument to the 'Parse' method and is not expected to be null. Helper.cs 157
Опять же, если value1 окажется null, то будет выброшено исключение ArgumentNullException.
Несмотря на внушительный объем кода и богатый функционал, в ходе анализа удалось обнаружить сравнительно небольшое количество подозрительных мест. Это говорит о том, что оба проекта хорошо написаны и поддерживаются на высоком уровне.
Тем не менее идеального кода не существует. Даже в зрелых и популярных библиотеках встречаются copy-paste ошибки, забытые параметры, неточности в условиях и другие мелкие недочёты, которые легко пропустить во время разработки и тестирования. Именно здесь статический анализ становится особенно полезным: он позволяет обратить внимание на такие места ещё до того, как они превратятся в реальные проблемы.
Если вы хотите самостоятельно проверить проект с помощью PVS-Studio, то попробовать анализатор можно по ссылке.
Берегите себя и свой код!
0
0
0
0