Пять забавных странностей в коде Entity Framework Core

Разработкой Entity Framework Core занимаются профессионалы, не допускающие ошибок благодаря опыту, ревью и мощной системе тестирования. Впрочем, даже в таком проекте не без приколов. Перед вами статья о странностях, укрывшихся от взгляда разработчиков высшего класса.

Мини-введение

Обычно мы пишем статьи, в которых разбираем целый список различных ошибок, найденных с помощью PVS-Studio в открытым проекте. Однако в этот раз такая статья не получится. Код в проекте действительно хорошо написан :). Так что, когда я писал про профессионализм, это был не сарказм.

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

Для поиска странностей и потенциальных ошибок я использовал анализатор PVS-Studio 7.26. Версия самого Entity Framework Core — 7.0.10. Топ составлялся совершено субъективно, поэтому если ваше мнение по какому-то вопросу отличается — пишите в комментариях, обсудим.

5 место. Несогласованность

protected virtual ModelBuilder VisitDatabaseModel(....)
{
  ....
  if (!string.IsNullOrEmpty(databaseModel.DatabaseName)) // <=
  {
    modelBuilder.Model.SetDatabaseName(
           !_options.UseDatabaseNames
        && !string.IsNullOrEmpty(databaseModel.DatabaseName) // <=
      ? _candidateNamingService.GenerateCandidateIdentifier(....)
      : databaseModel.DatabaseName
    );
  }
  ....
}

Первое срабатывание из топа сразу же отражает идею, лежащую в основе большинства странностей, которые я нашёл в Entity Framework Core. А именно — удивительная несогласованность кода, причём именно фрагментов, находящихся буквально по соседству.

Сначала проверяется, не является ли databaseModel.DatabaseName пустой строкой или null. Если там лежит вполне нормальное строковое значение, код решает проверить значение свойства ещё раз. Зачем? Чего разработчик хотел этим добиться?

И ведь что самое странное — разница между этими проверками буквально 3 строчки! Ну как так-то?

Для тех, кто решил посчитать разницу в строчках

В приведённом мною примере разница — 4 строчки (о да, это многое меняет :D). Однако это лишь оттого, что для статьи код пришлось слегка отформатировать. В "оригинале" вызовы _options.UseDatabaseNames и лишняя проверка DatabaseName записаны в одну строку.

Данный фрагмент найден с помощью следующего предупреждения анализатора:

V3063 A part of conditional expression is always true if it is evaluated: !string.IsNullOrEmpty(databaseModel.DatabaseName).

4 место. Модели нет, возьмите из неё схему

protected virtual IEnumerable<IReadOnlyModificationCommand> 
  GenerateModificationCommands(InsertDataOperation operation,
                               IModel? model)
{
  ....

  if (   operation.ColumnTypes != null
      && operation.Columns.Length != operation.ColumnTypes.Length)
  {
    throw new InvalidOperationException(
        RelationalStrings.InsertDataOperationTypesCountMismatch(
            ....,
            FormatTable(operation.Table,
                        operation.Schema ?? model?.GetDefaultSchema())));
  } 

  if (operation.ColumnTypes == null && model == null)
  {
    throw new InvalidOperationException(
        RelationalStrings.InsertDataOperationNoModel(
            FormatTable(operation.Table,
                        operation.Schema ?? model?.GetDefaultSchema())));
  }
  ....
}

Ещё одно замечательное проявление несогласованности и недостатка вдумчивости при написании кода. Действительно, какая там разница, про что вообще исключение, если можно просто скопипастить :). В проверке мы удостоверились, что model равна null, после чего кидаем исключение про то, что модели нет. Почему бы при его конструировании не попробовать обратиться к модели? :)

Погоди, там же использован оператор '?.', всё ок!

Дело не в том, что обращение к модели приведёт тут к выбрасыванию исключения. Дело в том, что программист (кажется) написал этот код, не задумываясь о его смысле. Учитывая, что мы говорим об одной из наиболее известных и широко используемых библиотек под .NET, подобный подход к разработке смущает.

Другая теория — возможно, в проверке допущена ошибка, и вместо '&&' нужно использовать '||'. Но это уже домыслы.

Описанную тут странность я нашёл благодаря предупреждению правила V3022:

Expression 'model' is always null. The operator '?.' is meaningless.

3 место. Двойной максимум

public virtual SqlExpression? Translate(....)
{
  if (SupportedMethods.TryGetValue(method, out var sqlFunctionName))
  {
    RelationalTypeMapping? typeMapping;
    List<SqlExpression>? newArguments = null;
    if (sqlFunctionName == "max" || sqlFunctionName == "max") // <=
    {
      typeMapping = ExpressionExtensions.InferTypeMapping(arguments[0], 
                                                          arguments[1]);
      newArguments = new List<SqlExpression>
      {
        ....
      };
    }
    else
    {
      typeMapping = arguments[0].TypeMapping;
    }

    var finalArguments = newArguments ?? arguments;

    return _sqlExpressionFactory.Function(....);
  }

  return null;
}

Вот тут уже может быть опаснее. Похоже, для функции max реализована какая-то особая обработка. Ну и подобное же хотели сделать для какой-то ещё функции, судя по всему. Предположу, что для функции min, но сложно сказать точно.

Увы, но сейчас код выглядит так, будто переспрашивает: "А может, имя функции всё-таки max?" (в случае если первое сравнение вернуло false). Не, ну а вдруг и правда что-то поменялось? :)

Использованный для поиска этой странности PVS-Studio даёт по этому поводу такой комментарий:

V3001 There are identical sub-expressions 'sqlFunctionName == "max"' to the left and to the right of the '||' operator.

2 место. Подозрительный успех

public override Expression Visit(Expression expression)
{
  ....
  if (   TryGetPartitionKeyProperty(entityType,
                                    out var partitionKeyProperty)
      && entityTypePrimaryKeyProperties.SequenceEqual(queryProperties)
      && (partitionKeyProperty == null || ....)
      && ....)
  {
    var propertyParameterList = queryProperties.Zip(....);

    var readItemExpression = new ReadItemExpression(entityType, 
                                                    propertyParameterList);

    return ....;
  }
  ....
}

Предлагаю внимательно вглядеться в код. Видите ли вы странность, скрытую в нём?

Ну, я тоже не вижу :). На самом деле какой-то ошибки тут, может, и правда нет, но всё-таки случай любопытный.

Необычность здесь состоит в вызове функции TryGetPartitionKeyProperty. По идее, она работает по обычному паттерну Try-методов — возвращает true, если операция успешна, и false в противном случае.

Если вы подумали так же, как и я, то поздравляю — код вас обманул. Ведь метод TryGetPartitionKeyProperty выглядит так:

static bool TryGetPartitionKeyProperty(IEntityType entityType,
                                       out IProperty partitionKeyProperty)
{
  var partitionKeyPropertyName = entityType.GetPartitionKeyPropertyName();
  if (partitionKeyPropertyName is null)
  {
    partitionKeyProperty = null;
    return true;
  }

  partitionKeyProperty = entityType.FindProperty(partitionKeyPropertyName);
  return true;
}

Ну как бы да, функции, написанные успешными разработчиками, всегда отрабатывают успешно :).

Спорный момент тут состоит в следующем. Если значение partitionKeyPropertyName не получено, то в out-параметр явно записывается null. При этом всё равно считается, что метод TryGetPartitionKeyProperty отработал успешно (что звучит странно, если вчитаться). На этот момент можно взглянуть с двух сторон.

С одной стороны, тут ничего страшного, ведь вызывающий код вполне готов к тому, что в out-параметр будет записан null:

if (   TryGetPartitionKeyProperty(entityType,
                                  out var partitionKeyProperty)
    && entityTypePrimaryKeyProperties.SequenceEqual(queryProperties)
    && (partitionKeyProperty == null || ....)
    && ....)

Да, метод можно было бы переписать так, чтобы он просто возвращал IProperty (безо всяких Try, out-параметров и возвращения логического значения). Но как бы вроде и не страшно.

С другой стороны, не совсем понятно, на что именно рассчитывает эта проверка на null. К примеру, она могла быть добавлена на случай, если null вернёт вызов entityType.FindProperty(partitionKeyPropertyName).

Я склоняюсь к тому, что тут что-то не так. Ну очень уж странно, что метод с названием TryGetPartitionKeyProperty возвращает *true *(как бы говоря: "Да, мы всё успешно получили!"), но при этом сам выставляет null в соответствующий out-параметр.

На этот спорный случай я наткнулся благодаря предупреждению PVS-Studio:

V3063 A part of conditional expression is always true if it is evaluated: TryGetPartitionKeyProperty(entityType, out var partitionKeyProperty).

1 место. Да кому нужен этот ef.exe?

protected override int Execute(string[] _)
{
  ....
  var targetPlatformIdentifier = startupProject.TargetPlatformIdentifier!;
  if (   targetPlatformIdentifier.Length != 0
      && !string.Equals(targetPlatformIdentifier, "Windows", ....))
  {
    executable = Path.Combine(
            toolsPath,
            "net461",
            startupProject.PlatformTarget switch
            {
              "x86" => "win-x86",
              "ARM64" => "win-arm64",
              _ => "any"
            },
            "ef.exe");
  }

  executable = "dotnet";
  args.Add("exec");
  args.Add("--depsfile");
  args.Add(depsFile);

  ....

  return Exe.Run(executable, args, startupProject.ProjectDir);
}

Я бы сказал, что этот фрагмент позабавил больше других. Сначала идёт суровая серьёзная проверка различных значений, после чего идёт не менее серьёзный функционал формирования пути с помощью сложного вызова Path.Combine. А затем результаты всего этого просто затираются :). Теперь просто 'dotnet' и всё.

И я ничего не имею против, но выглядит странно и забавно. Если тут нет ошибки, то опять имеем какую-то дикую несогласованность логики, будто в код вставляются рандомные строчки без оглядки на то, что происходит по соседству. Возможно, это последствия неудачного слияния веток.

Нашёл я этот фрагмент благодаря сообщению от правила V3008:

The 'executable' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 139, 127.

Мини-заключение

Несмотря на все мои замечания, стоит признать, что Entity Framework и правда пишут мощные профессионалы. Почти ни одно из представленных предупреждений не выглядит на 100% ошибкой, а я, можете поверить, очень старался найти самое лучшее.

Конечно, можно сказать, что тут просто анализатор не очень справился, но как-то так вышло, что в других проектах он вполне находит немало более очевидных проблем. Кстати, если будет интересно попробовать этот инструмент, то его можно бесплатно получить здесь.

Ну а меня на этом всё, надеюсь, вам было интересно :).

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. 5 amusing oddities in the code of Entity Framework Core.