nopCommerce — бесплатная платформа для создания интернет-магазинов с открытым исходным кодом, разработанная на базе ASP.NET Core. Сегодня мы узнаем, какие неоднозначные моменты таятся в коде платформы.

Пара слов о проекте

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

nopCommerce — открытая платформа электронной коммерции на основе ASP.NET Core. Она является одним из ведущих инструментов в этой области. Однако даже с отличной репутацией и широким распространением важными аспектами, требующими внимания и анализа, остаются вопросы качества кода в проектах.

Проверяемый исходный код соответствует коммиту. Для анализа был использован инструмент PVS-Studio версии 7.29.

Хочется отметить, что в статье будут фигурировать не только предупреждения, которые указывают на ошибки, но и некоторые неоднозначные срабатывания анализатора.

Приятного чтения!

Небольшое отступление

Из раза в раз мы сталкиваемся с огромным числом ошибок, связанных с NRE. Чтобы не быть голословным, оставлю ссылки на кейсы, о которых мы уже писали в статьях:

И это лишь часть диагностических правил, которые сигнализируют о потенциальном разыменовании нулевой ссылки.

Проект nopCommerce не стал исключением: большая часть предупреждений, рассмотренных в статье, связаны с NRE.

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

Подозрительный Equals

Последнее время довольно часто приходится сталкиваться с ошибками при реализации метода Equals. Один из таких кейсов мы уже ранее отразили в статье.

Что уж говорить, мы и сами допустили подобную ошибку при реализации диагностики. Кстати, ошибка была вовремя обнаружена и натолкнула на идею создания нового диагностического правила. Оно связано с проверкой неправильного типа при переопределении Equals.

Ошибка, представленная в этом примере, достаточно типична, но от этого менее опасной она не становится.

Фрагмент 1

public partial class CategoryKey
{
  ....

  public bool Equals(CategoryKey y)
  {
    if (y == null)
      return false;

    if (Category != null && y.Category != null)
      return Category.Id == y.Category.Id;

    if (....)
    {
      return false;
    }

    return Key.Equals(y.Key);
  }

  public override bool Equals(object obj)
  {
    var other = obj as CategoryKey;
    return other?.Equals(other) ?? false;        // <=
  }
}

Предупреждение PVS-Studio: V3062 An object 'other' is used as an argument to its own method. Consider checking the first actual argument of the 'Equals' method. ImportManager.cs 3392

Обратите внимание на вызов метода Equals в теле переопределённого Equals. Можно заметить, что метод вызван у переменной other. Она же передаётся в качестве аргумента. Получается, что аргумент сравнивается сам с собой. Сомневаюсь, что разработчики закладывали такую логику в работу метода Equals.

Для исправления ошибки можно передавать this, а не other, в качестве аргумента Equals.

Неиспользованные значения

Не всегда погрешности, связанные с неиспользованными значениями, приводят к возникновению исключений или изменению логики работы программы. Однако и такие ситуации могут возникнуть. В любом случае нужно избегать подобных моментов. Как минимум это позволит сделать код чище, как максимум — поможет не допустить некорректного поведения программы.

Далее представлены фрагменты кода, которые содержат неиспользованные значения.

Фрагмент 2

protected virtual async Task<....> PrepareCheckoutPickupPointsModelAsync(....)
{
  ....

  if (amount > 0)
  {
    (amount, _) = await 
       _taxService.GetShippingPriceAsync(amount, customer);

    amount = await
       _currencyService.ConvertFromPrimaryStoreCurrencyAsync(amount,
                                                             currentCurrency);

    pickupPointModel.PickupFee = await                              // <=
       _priceFormatter.FormatShippingPriceAsync(amount, true);
  }

  //adjust rate
  var (shippingTotal, _) = await
     _orderTotalCalculationService.AdjustShippingRateAsync(point.PickupFee,
                                                           cart,
                                                           true);
  var (rateBase, _) = await 
     _taxService.GetShippingPriceAsync(shippingTotal, customer);

  var rate = await
     _currencyService.ConvertFromPrimaryStoreCurrencyAsync(rateBase,
                                                           currentCurrency);

  pickupPointModel.PickupFee = await                                // <=
     _priceFormatter.FormatShippingPriceAsync(rate, true);

  ....
}

Предупреждение PVS-Studio: V3008 The 'pickupPointModel.PickupFee' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 210, 203. CheckoutModelFactory.cs 210

После присваивания в pickupPointModel.PickupFee некоторого значения свойство не используется до следующей перезаписи этого значения. Подобное присваивание может иметь смысл, если аксессор set свойства имеет какую-то особую логику. Однако в этом случае это не так, pickupPointModel.PickupFee — обычное автосвойство. Получается, что содержимое then-ветви оператора if никак не влияет на логику работы программы.

Фрагмент 3

public virtual async Task<....> GetOrderAverageReportLineAsync(....)
{
  ....

  if (!string.IsNullOrEmpty(orderNotes))
  {
    query = from o in query
            join n in _orderNoteRepository.Table on o.Id equals n.OrderId
            where n.Note.Contains(orderNotes)
            select o;

    query.Distinct();                          // <=
  }

  ....
}

Предупреждение PVS-Studio: V3010 The return value of function 'Distinct' is required to be utilized. OrderReportService.cs 342

Для удаления повторяющихся элементов коллекции можно использовать Distinct (метод LINQ). Так и хотели сделать разработчики в этом примере, но что-то пошло не так. Метод Distinct не модифицирует коллекцию, у которой вызывается. Следовательно, если не использовать возвращаемое значение метода, то вызов не имеет смысла. Именно такая ситуация возникла в рассматриваемом коде.

Скорее всего, результат выполнения Distinct должен присваиваться переменной query.

Проблемы с null

Здесь представлены классические, если их так можно назвать, ошибки. Особо добавить нечего, с NRE знакомы все.

Фрагмент 4

public async Task<....> GetTaxTotalAsync(TaxTotalRequest taxTotalRequest)
{
  ....

  var taxRates = transaction.summary
                            ?.Where(....)
                            .Select(....)
                            .ToList();

  foreach (var taxRate in taxRates)                              // <=
  {
    if (taxTotalResult.TaxRates.ContainsKey(taxRate.Rate))
      taxTotalResult.TaxRates[taxRate.Rate] += taxRate.Value;
    else
      taxTotalResult.TaxRates.Add(taxRate.Rate, taxRate.Value);
  }

  ....
}

Предупреждение PVS-Studio: V3105 The 'taxRates' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. AvalaraTaxProvider.cs 113

При получении значения для taxRates производится обращение к свойству transaction.summary с помощью оператора '?.'. Судя по всему, разработчик предполагал, что значение этого свойства может быть null. Если это так, то и в taxRates может быть присвоен null. Сразу после инициализации taxRates эта переменная используется в качестве коллекции, по которой производится итерирование в foreach. Если taxRates имеет значение null, то будет выброшено исключение типа NullReferenceException. Это произойдёт из-за того, что у коллекции неявно вызывается метод GetEnumerator.

Стоит отметить, что это достаточно распространённый паттерн ошибки. О нём мы уже рассказывали в статье.

Фрагмент 5

public async Task<....> GoogleAuthenticatorDelete(....)
{
  ....

  //delete configuration
  var configuration = 
    await _googleAuthenticatorService.GetConfigurationByIdAsync(model.Id);

  if (configuration != null)
  {
    await _googleAuthenticatorService
                     .DeleteConfigurationAsync(configuration);
  }

  var customer = await _customerService
                         .GetCustomerByEmailAsync(configuration.Customer) ??
                 await _customerService
                         .GetCustomerByUsernameAsync(configuration.Customer);

  ....
}

Предупреждение PVS-Studio: V3125 The 'configuration' object was used after it was verified against null. Check lines: 139, 135. GoogleAuthenticatorController.cs 139

Переменная configuration проверяется на null перед первым использованием, а при последующих нет. Стоит отметить, что метод GetConfigurationByIdAsync, с помощью которого было получено значение этой переменной, может вернуть null. Возможно, разработчики уверены, что в этом случае null не будет возвращён. Тогда не совсем понятно, зачем нужна проверка на null. В противном же случае возможно возникновение исключения из-за разыменования нулевой ссылки.

Фрагмент 6

public async Task<....> RefundAsync(.... refundPaymentRequest)  
{
  ....
   var clientReferenceInformation = 
         new Ptsv2paymentsClientReferenceInformation(Code: refundPaymentRequest
                                                                    .Order
                                                                    ?.OrderGuid
                                                                    .ToString(),
                                                                    ....);
  ....
  if (refundPaymentRequest.Order.AllowStoringCreditCardNumber)            // <=
  {
    var cardInformation = new Ptsv2paymentsidrefundsPaymentInformationCard(
     Number: CreditCardHelper.RemoveSpecialCharacters(
                                _encryptionService
                                        .DecryptText(refundPaymentRequest
                                                         .Order
                                                         ?.CardNumber)),

     ExpirationMonth: _encryptionService.DecryptText(refundPaymentRequest
                                                         .Order
                                                         ?.CardExpirationMonth),

     ExpirationYear: _encryptionService.DecryptText(refundPaymentRequest
                                                        .Order
                                                        ?.CardExpirationYear));
    ....

  }
  ....
  var result = await apiInstance.RefundCaptureAsync(
                                   refundCaptureRequest: requestObj,
                                   id:       refundPaymentRequest
                                                 .Order
                                                 ?.CaptureTransactionId 
                                         ??
                                             refundPaymentRequest
                                                 .Order
                                                 ?.AuthorizationTransactionId);
  ....
}

Предупреждение PVS-Studio: V3095 The 'refundPaymentRequest.Order' object was used before it was verified against null. Check lines: 597, 600. CyberSourceService.cs 597

Обратите внимание на свойство refundPaymentRequest.Order. Оно было проверено на null шесть раз, а использовалось семь. Что-то тут не сходится. Подозрительно, что в условии if обращение к refundPaymentRequest.Order производится без использования '?.'. Возможно, оно не может иметь значение null в контексте этого метода. Тогда стоит убрать проверку в остальных случаях. Если же refundPaymentRequest.Order может быть null, то рано или поздно вызов RefundAsync приведёт к выбрасыванию исключения типа NullReferenceException.

Не больше одной итерации

Как часто вы используете while вместо if? Думаю, что достаточно редко.

Здесь представлен весьма необычный пример использования while.

Фрагмент 7

protected virtual TrieNode GetOrAddNode(ReadOnlySpan<char> key,
                                        TValue value,
                                        bool overwrite = false)
{
  ....

  while (!node.IsDeleted && node.Children.TryGetValue(c, out nextNode))
  {
    var label = nextNode.Label.AsSpan();
    var i = GetCommonPrefixLength(label, suffix);

    // suffix starts with label?
    if (i == label.Length)
    {
      // if the keys are equal, the key has already been inserted
      if (i == suffix.Length)
      {
        if (overwrite)
          nextNode.SetValue(value);

        return nextNode;
      }

      // structure has changed since last; try again
      break;
    }

    ....

    return outNode;                                            // <=
  }

  ....
}

Предупреждение PVS-Studio: V3020 An unconditional 'return' within a loop. ConcurrentTrie.cs 230

Анализатор сомневается в правильности реализации цикла. Давайте разбираться, что тут не так. В теле цикла находится оператор return, который срабатывает без условия. Это не всегда является ошибкой, так как перед return могут быть операторы continue. Вследствие этого return не обязательно будет отрабатывать при первой итерации цикла. Однако в этом случае continue нет. Выход из цикла всегда будет производиться при первой итерации.

Варианты выхода из цикла:

либо с помощью return, который находится в теле одного из if;
либо с помощью break (также в теле if);
либо с помощью return в самом конце цикла.

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

Как вариант, перед нами, возможно, опечатка, и вместо break должен использоваться оператор continue. На это намекает фраза "try again" в комментарии.

Подозрительная проверка

Классическая ошибка, связанная с Copy-Paste.

Фрагмент 8

public async Task<bool?> IsConditionMetAsync(string conditionAttributeXml, 
                                             string selectedAttributesXml)
{
  if (string.IsNullOrEmpty(conditionAttributeXml))
    return null;

  if (string.IsNullOrEmpty(conditionAttributeXml))
    //no condition
    return null;

  ....
}

Предупреждение PVS-Studio: V3022 Expression 'string.IsNullOrEmpty(conditionAttributeXml)' is always false. AttributeParser.cs 499

Обратите внимание на условие второго if. В нём проверяется значение поля conditionAttributeXml. Это выглядит весьма странно, так как в предыдущем if проверялось это же поле. Очевидно, что в одном из случаев аргументом для метода IsNullOrEmpty должен быть параметр selectedAttributesXml.

Ложные срабатывания?

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

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

Фрагмент 9

protected virtual async Task 
            PrepareSimpleProductOverviewPriceModelAsync(Product product, 
                                                        .... priceModel)
{
  ....

  if (product.IsRental)
  {
    //rental product
    priceModel.OldPrice = await _priceFormatter
                                  .FormatRentalProductPeriodAsync(....);

    priceModel.OldPriceValue = priceModel.OldPriceValue;

    priceModel.Price = await _priceFormatter
                               .FormatRentalProductPeriodAsync(....);

    priceModel.PriceValue = priceModel.PriceValue;
  }

  ....
}

Предупреждения PVS-Studio:

V3005 The 'priceModel.OldPriceValue' variable is assigned to itself. ProductModelFactory.cs 503
V3005 The 'priceModel.PriceValue' variable is assigned to itself. ProductModelFactory.cs 505

Анализатор сообщает, что priceModel.OldPriceValue и priceModel.PriceValue присваиваются сами себе. Скорее всего, ошибки здесь нет, однако и срабатывание анализатора ложным назвать нельзя. Как же так получилось? Всё дело в том, что часть кода избыточна. Если убрать присваивания переменным priceModel.OldPriceValue и priceModel.PriceValue, логика работы не изменится.

Сейчас же возникает вопрос: было ли задумано, что свойствам должно присваиваться текущее значение или нет? Если да, то почему только этим свойствам?

Чтобы вопросов возникало меньше (или вообще не было), можно поступить двумя способами:

убрать лишние присваивания;
оставить комментарий, подтверждающий корректность текущих присваиваний.

Оба варианта позволят сделать код чуть лучше :)

Фрагмент 10

public abstract partial class BaseNopValidator<TModel> 
             : AbstractValidator<TModel> where TModel : class
{
  protected BaseNopValidator()
  {
    PostInitialize();
  }

  /// <summary>
  /// Developers can override this method in 
  /// custom partial classes in order to add 
  /// some custom initialization code to constructors
  /// </summary>
  protected virtual void PostInitialize()
  {
  }

  ....
}

Предупреждение PVS-Studio: V3068 Calling overrideable class member 'PostInitialize' from constructor is dangerous. BaseNopValidator.cs 20

Про это предупреждение также нельзя сказать, что оно указывает на ошибку. Почему? Для ответа на этот вопрос нужно разобраться в сути срабатывания.

Представим, что у нас есть наследник класса BaseNopValidator, например, TestValidator, который переопределяет метод PostInitialize:

public class TestValidator : BaseNopValidator<object>
{
  Logger _logger;

  public TestValidator(Logger logger)
  {
    _logger = logger;
  }

  protected override void PostInitialize()
  {
    _logger.Log("Initializing");
  }
}

Если создать объект типа TestValidator, будет выброшено исключение типа NullReferenceException. Это произойдёт из-за того, что при создании объекта сначала отработает конструктор базового класса, а уже потом конструктор TestValidator. Следовательно, при вызове метода Log поле _logger будет иметь значение null.

Однако в проверяемом проекте ни один из классов не переопределяет метод PostInitialize. Следовательно, никакого исключения здесь возникнуть не может. Но это пока.

Заключение

По итогу проверки можно сказать, что код оказался достаточно чистым, но не идеальным :)

Мне кажется, разработчикам стоит обратить внимание на те проблемы, которые описаны в статье. Стоит отметить, что для публикации были отобраны наиболее интересные срабатывания. Следовательно, потенциальных проблем несколько больше, чем описано здесь.

Для проверки интересующего вас проекта можно бесплатно попробовать PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Panevin. Invincible null: digging into nopCommerce source code.

Непобедимый null: копаемся в исходном коде nopCommerce