# Что скрывают популярные ORM для C\#: проверка проектов RepoDB и SqlSugar

Когда речь заходит об ORM, большинство C\# разработчиков сразу вспоминают мощный Entity Framework Core или же легковесный Dapper\. Но что насчёт RepoDB и SqlSugar? Несмотря на меньшую известность, эти ORM активно используются в реальных проектах и продолжают развиваться\. Давайте посмотрим, какие проблемы удастся обнаружить в их исходном коде с помощью статического анализатора\.

![1411_dotnet_orm_errors_ru/image1.png](https://import.viva64.com/docx/blog/1411_dotnet_orm_errors_ru/image1.png)

## Введение

Сейчас трудно представить себе современное приложение без использования ORM\. И не спроста\. Эта технология облегчает жизнь разработчикам: позволяет избежать написания большого количества кода и необходимости работы с SQL\. 

Именно ORM отвечает за взаимодействие с базой данных: через него проходят запросы на чтение и запись, транзакции и преобразование объектов в SQL\. Ошибка в таком инструменте может стоить дорого: от снижения производительности до некорректной работы приложения и даже проблем с безопасностью\.

В мире \.NET стандартом являются [Entity Framework Core](https://github.com/dotnet/efcore) и [Dapper](https://github.com/DapperLib/Dapper)\. Однако есть большое количество менее популярных инструментов, которые находят применение в реальных проектах\. Среди таких ORM — [RepoDB](https://github.com/mikependon/RepoDB) и [SqlSugar](https://github.com/DotNetNext/SqlSugar)\.

В этой статье мы посмотрим на них глазами статического анализатора PVS\-Studio\. Вместо сравнения возможностей API или производительности мы разберём исходный код проектов и поищем потенциальные ошибки и подозрительные конструкции\.

## SqlSugar

Перейдём к обзору интересных частей кода SqlSugar\. Исходники возьмём из этого [коммита](https://github.com/DotNetNext/SqlSugar/tree/d221c7b14ab4bc4c81ce53b6a450c96e70091168)\.

### Забытый CancellationToken

#### Фрагмент кода 1

```cpp
public Task<int> ExecuteCommandAsync(string sql, object parameters, 
  CancellationToken cancellationToken) 
{
  this.CancellationToken = CancellationToken;
  return ExecuteCommandAsync(sql,parameters);
}
```

Предупреждение PVS\-Studio: [V3005](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'this\.CancellationToken' variable is assigned to itself\. [AdoProvider\.cs 1472](https://github.com/DotNetNext/SqlSugar/blob/d221c7b14ab4bc4c81ce53b6a450c96e70091168/Src/Asp.Net/SqlSugar/Abstract/AdoProvider/AdoProvider.cs#L1472)

В автоматическое свойство `CancellationToken` записывается его собственное значение\. Скорее всего, этому свойству необходимо присваивать значение параметра `cancellationToken`\.

### Бессмысленная проверка строки

#### Фрагмент кода 2

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3122/) Lowercase string is compared with a different mixed case string\. [NavigatManager\.cs 1131](https://github.com/DotNetNext/SqlSugar/blob/d221c7b14ab4bc4c81ce53b6a450c96e70091168/Src/Asp.Net/SqlSugar/Abstract/QueryableProvider/NavigatManager.cs#L1131)

Давайте разбираться, в чём тут проблема\. Сначала строку `result.SelectString` приводят к нижнему регистру с помощью метода `ToLower`, а затем проверяют, содержится ли в полученной строке `$" {selectPkName.ToLower()} AS {selectPkName.ToLower()}"`\. Можно заметить, что искомая подстрока содержит буквы в верхнем регистре \(`AS`\)\. Таким образом, метод `Contains` всегда будет возвращать `false`\.

Чтобы исправить код, достаточно привести все символы к нижнему регистру:

```cpp
!result.SelectString
       .ToLower()
       .Contains($" {selectPkName.ToLower()} as {selectPkName.ToLower()}"))
```

### Повторяющиеся условия

#### Фрагмент кода 3

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'it\.Value is short' to the left and to the right of the '\|\|' operator\. [QuestDBInsertBuilder\.cs 89](https://github.com/DotNetNext/SqlSugar/blob/d221c7b14ab4bc4c81ce53b6a450c96e70091168/Src/Asp.Net/SqlSugar/Realization/QuestDB/SqlBuilder/QuestDBInsertBuilder.cs#L89)

Дважды использовать подвыражение `it.Value is short` бессмысленно\. Возможно, вместо `short` должен стоять какой\-нибудь другой тип, например `decimal` или `float`\.

#### Фрагмент кода 4

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'value is long?' to the left and to the right of the '\|\|' operator\. [UtilMethods\.cs 288](https://github.com/DotNetNext/SqlSugar/blob/d221c7b14ab4bc4c81ce53b6a450c96e70091168/Src/Asp.Net/SqlSugar.MySqlConnector/Tools/UtilMethods.cs#L288)

Здесь дважды написали условие `value is long?`\. Исходя из кода выше можно предположить, что второе одинаковое условие следует заменить на `value is ulong`\.

### Неиспользованный параметр

#### Фрагмент кода 5

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/DotNetNext/SqlSugar/blob/d221c7b14ab4bc4c81ce53b6a450c96e70091168/Src/Asp.Net/SqlSugar/ExpressionsToSql/ResolveItems/MemberExpressionResolve.cs#L770)

Параметр `isLeft` не был использован, однако в методе используется свойство, имя которого отличается от имени параметра только регистром первой буквы\.

Возможно, вместо свойства `IsLeft` предполагалось использовать параметр `isLeft` метода `GetName`\.

### Потерянный num

#### Фрагмент кода 6

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'GetString' is required to be utilized\.  [SubTake\.cs 64](https://github.com/DotNetNext/SqlSugar/blob/d221c7b14ab4bc4c81ce53b6a450c96e70091168/Src/Asp.Net/SqlSugar/ExpressionsToSql/Subquery/Items/SubTake.cs#L64)

Анализатор сообщает, что возвращаемое значение метода `GetString` не используется\. При этом по реализации `GetString` становится ясно, что метод только формирует и возвращает строковое представление `_Result`\. Он не изменяет состояние объекта и не выполняет действий, результат которых использовался бы дальше в коде:

```cpp
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`\.

Вероятно, корректный код должен выглядеть так:

```cpp
copyContext.Resolve(numExp, ResolveExpressType.WhereMultiple);
num = copyContext.Result.GetString();
```

## RepoDB

Перейдём к обзору ошибок и странных мест в проекте RepoDB\. Исходники взяты из этого [коммита](https://github.com/mikependon/RepoDB/tree/58004d4b05a99b0332e8eb3bbe74d366030b8924)\.

### Бесполезные проверки

#### Фрагмент кода 1

```cpp
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:

* V3022 Expression 'expression\.Test\.NodeType \> ExpressionType\.GreaterThanOrEqual' is always false\. [ExpressionExtension\.cs 456](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.Core/RepoDb/Extensions/ExpressionExtension.cs#L456)
* V3022 Expression 'expression\.Test\.NodeType \> ExpressionType\.LessThan' is always false\. [ExpressionExtension\.cs 460](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.Core/RepoDb/Extensions/ExpressionExtension.cs#L460)
* V3022 Expression 'expression\.Test\.NodeType \> ExpressionType\.LessThanOrEqual' is always false\. [ExpressionExtension\.cs 464](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.Core/RepoDb/Extensions/ExpressionExtension.cs#L464)

В методе `GetValue` выражение `expression.Test.NodeType` последовательно сравнивается с различными значениями перечисления `ExpressionType`\. Однако в последних четырёх проверках вместо оператора равенства используется оператор `>`\.

Проблема заключается в том, что цепочка `else if` выполняется последовательно и значение элементов перечисления в нижних блоках больше, чем в верхних\. 

```cpp
public enum ExpressionType
{
  ....
  GreaterThan = 15,
  GreaterThanOrEqual = 16,
  ....
  LessThan = 20,
  LessThanOrEqual = 21,
  ....
}
```

Условие `expression.Test.NodeType > ExpressionType.GreaterThan` покрывает все последующие условия\. 

То есть, если `expression.Test.NodeType > ExpressionType.GreaterThan` будет `true`, то последующие условия не будут проверены, а если оно будет `false`, то и следующие условия будут `false`\.

### Сopy\-paste ошибка

#### Фрагмент кода 2

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'Qualifiers' variable should be used instead of 'Fields'\. [UpdateAllRequest\.cs 124](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.Core/RepoDb/Requests/UpdateAllRequest.cs#L124)

`Fields` дважды проверяется на неравенство `null`\. Скорее всего, во втором случае вместо `Fields` следует проверять `Qualifiers`, так как эта коллекция используется в `foreach`\.

#### Фрагмент кода 3

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3038/) The argument was passed to method several times\. It is possible that other argument should be passed instead\. [UpdateAll\.cs 619](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.Core/RepoDb/Operations/DbConnection/UpdateAll.cs#L619)

Здесь в метод `UpdateAllAsyncInternal` дважды передаётся параметр `fields`, а параметр `qualifiers`, в свою очередь, не используется\.

Возможно, предполагалось передавать `qualifiers` следующим образом:

```cpp
qualifiers: Field.Parse<TEntity>(qualifiers)
```

### Пустая коллекция

#### Фрагмент кода 4

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3191/) Iteration through the 'fields' collection makes no sense because it is always empty\. [SqlServerStatementBuilder\.cs 70](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.SqlServer/RepoDb.SqlServer/StatementBuilders/SqlServerStatementBuilder.cs#L70)

Условие `fields?.Any() != true` будет истинным в том случае, если коллекция `fields` пустая или `null`\. В таком случае, `fields?.Select(f => f.Name)` не имеет смысла, поскольку всегда будет возвращать пустую коллекцию или `null`\.

### Коварный null

#### Фрагмент кода 5

```cpp
public override Guid GetGuid(int i)
{
  ThrowExceptionIfNotAvailable();
  return Guid.Parse(GetValue(i)?.ToString());
}
```

Предупреждение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.Core/RepoDb/DataEntityDataReader.cs#L405)

Тут `GetValue(i)` может вернуть `null`, и из\-за оператора `?` этот `null` будет передан в метод `Guid.Parse`\. В свою очередь метод `Guid.Parse` при передаче аргумента `null` выбросит исключение `ArgumentNullException`\.

#### Фрагмент кода 6

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/mikependon/RepoDB/blob/58004d4b05a99b0332e8eb3bbe74d366030b8924/RepoDb.SqLite/RepoDb.SqLite.IntegrationTests/Helper.cs#L157)

Опять же, если `value1` окажется `null`, то будет выброшено исключение `ArgumentNullException`\.

## Заключение

Несмотря на внушительный объем кода и богатый функционал, в ходе анализа удалось обнаружить сравнительно небольшое количество подозрительных мест\. Это говорит о том, что оба проекта хорошо написаны и поддерживаются на высоком уровне\.

Тем не менее идеального кода не существует\. Даже в зрелых и популярных библиотеках встречаются copy\-paste ошибки, забытые параметры, неточности в условиях и другие мелкие недочёты, которые легко пропустить во время разработки и тестирования\. Именно здесь статический анализ становится особенно полезным: он позволяет обратить внимание на такие места ещё до того, как они превратятся в реальные проблемы\.

Если вы хотите самостоятельно проверить проект с помощью PVS\-Studio, то попробовать анализатор можно по [ссылке](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.

Берегите себя и свой код\!