# Непобедимый null: копаемся в исходном коде nopCommerce nopCommerce — бесплатная платформа для создания интернет\-магазинов с открытым исходным кодом, разработанная на базе ASP\.NET Core\. Сегодня мы узнаем, какие неоднозначные моменты таятся в коде платформы\. ![1109_NopCommerce_ru/image1.png](https://import.viva64.com/docx/blog/1109_NopCommerce_ru/image1.png) ## Пара слов о проекте При разработке ПО качество кода играет решающую роль в надёжности, безопасности и производительности программных продуктов\. nopCommerce — открытая платформа электронной коммерции на основе ASP\.NET Core\. Она является одним из ведущих инструментов в этой области\. Однако даже с отличной репутацией и широким распространением важными аспектами, требующими внимания и анализа, остаются вопросы качества кода в проектах\. Проверяемый исходный код соответствует [коммиту](https://github.com/nopSolutions/nopCommerce/tree/45d416f9936eb3a446d5193c61930f0a3b3c9a79)\. Для анализа был использован инструмент [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) версии 7\.29\. Хочется отметить, что в статье будут фигурировать не только предупреждения, которые указывают на ошибки, но и некоторые неоднозначные срабатывания анализатора\. Приятного чтения\! ## Небольшое отступление Из раза в раз мы сталкиваемся с огромным числом ошибок, связанных с [NRE](https://pvs-studio.ru/ru/blog/terms/6694/)\. Чтобы не быть голословным, оставлю ссылки на кейсы, о которых мы уже писали в статьях: * [кейсы диагностики V3080](https://pvs-studio.ru/ru/blog/examples/v3080/); * [кейсы диагностики V3095](https://pvs-studio.ru/ru/blog/examples/v3095/); * [кейсы диагностики V3105](https://pvs-studio.ru/ru/blog/examples/v3105/)\. И это лишь часть диагностических правил, которые сигнализируют о потенциальном разыменовании нулевой ссылки\. Проект nopCommerce не стал исключением: большая часть предупреждений, рассмотренных в статье, связаны с NRE\. Возможно, в ряде случаев разыменование нулевой ссылки не представляет большой проблемы\. Это может быть обусловлено тем, что для подобных ситуаций написана обработка, или исключение будет обнаружено на этапе тестирования\. Но будем честны, не всегда тесты покрывают абсолютно все сценарии работы программы\. Ошибка может затесаться в коде, который выполняется редко, и обнаружит её уже пользователь\. Как мне кажется, такой сценарий крайне нежелателен\. Именно поэтому следует максимально обезопасить проект от подобного развития событий\. ## Подозрительный Equals Последнее время довольно часто приходится сталкиваться с ошибками при реализации метода _Equals_\. Один из таких кейсов мы уже ранее отразили в [статье](https://pvs-studio.ru/ru/blog/posts/csharp/1032/)\. Что уж говорить, мы и сами допустили подобную ошибку при реализации диагностики\. Кстати, ошибка была вовремя обнаружена и натолкнула на идею создания нового диагностического правила\. Оно связано с проверкой неправильного типа при переопределении _Equals_\. Ошибка, представленная в этом примере, достаточно типична, но от этого менее опасной она не становится\. **Фрагмент 1** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Distinct' is required to be utilized\. OrderReportService\.cs 342 Для удаления повторяющихся элементов коллекции можно использовать _Distinct_ \(метод LINQ\)\. Так и хотели сделать разработчики в этом примере, но что\-то пошло не так\. Метод _Distinct_ не модифицирует коллекцию, у которой вызывается\. Следовательно, если не использовать возвращаемое значение метода, то вызов не имеет смысла\. Именно такая ситуация возникла в рассматриваемом коде\. Скорее всего, результат выполнения _Distinct_ должен присваиваться переменной _query_\. ## Проблемы с null Здесь представлены классические, если их так можно назвать, ошибки\. Особо добавить нечего, с NRE знакомы все\. **Фрагмент 4** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_\. Стоит отметить, что это достаточно распространённый паттерн ошибки\. О нём мы уже рассказывали в [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)\. **Фрагмент 5** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp protected virtual TrieNode GetOrAddNode(ReadOnlySpan 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp public async Task IsConditionMetAsync(string conditionAttributeXml, string selectedAttributesXml) { if (string.IsNullOrEmpty(conditionAttributeXml)) return null; if (string.IsNullOrEmpty(conditionAttributeXml)) //no condition return null; .... } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\(conditionAttributeXml\)' is always false\. AttributeParser\.cs 499 Обратите внимание на условие второго _if_\. В нём проверяется значение поля _conditionAttributeXml_\. Это выглядит весьма странно, так как в предыдущем _if_ проверялось это же поле\. Очевидно, что в одном из случаев аргументом для метода _IsNullOrEmpty_ должен быть параметр _selectedAttributesXml_\. ## Ложные срабатывания? Не про каждое предупреждение анализатора можно сказать, что оно является ложным или, напротив, указывает на ошибку\. Именно такие случаи будут отражены в этом разделе\. Если анализатор сомневается в коде, то такой код почти наверняка вызовет недоумение и у программистов, которые будут его сопровождать\. Такие предупреждения — хороший повод для рефакторинга\. Кстати, у нас есть [заметка на этот счёт](https://pvs-studio.ru/ru/blog/posts/cpp/0968/)\. **Фрагмент 9** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'priceModel\.OldPriceValue' variable is assigned to itself\. ProductModelFactory\.cs 503 * [V3005](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'priceModel\.PriceValue' variable is assigned to itself\. ProductModelFactory\.cs 505 Анализатор сообщает, что _priceModel\.OldPriceValue_ и _priceModel\.PriceValue_ присваиваются сами себе\. Скорее всего, ошибки здесь нет, однако и срабатывание анализатора ложным назвать нельзя\. Как же так получилось? Всё дело в том, что часть кода избыточна\. Если убрать присваивания переменным _priceModel\.OldPriceValue_ и _priceModel\.PriceValue_, логика работы не изменится\. Сейчас же возникает вопрос: было ли задумано, что свойствам должно присваиваться текущее значение или нет? Если да, то почему только этим свойствам? Чтобы вопросов возникало меньше \(или вообще не было\), можно поступить двумя способами: * убрать лишние присваивания; * оставить комментарий, подтверждающий корректность текущих присваиваний\. Оба варианта позволят сделать код чуть лучше :\) **Фрагмент 10** ```cpp public abstract partial class BaseNopValidator : AbstractValidator where TModel : class { protected BaseNopValidator() { PostInitialize(); } /// /// Developers can override this method in /// custom partial classes in order to add /// some custom initialization code to constructors /// protected virtual void PostInitialize() { } .... } ``` Предупреждение PVS\-Studio: [V3068](https://pvs-studio.ru/ru/docs/warnings/v3068/) Calling overrideable class member 'PostInitialize' from constructor is dangerous\. BaseNopValidator\.cs 20 Про это предупреждение также нельзя сказать, что оно указывает на ошибку\. Почему? Для ответа на этот вопрос нужно разобраться в сути срабатывания\. Представим, что у нас есть наследник класса _BaseNopValidator_, например, _TestValidator_, который переопределяет метод _PostInitialize_: ```cpp public class TestValidator : BaseNopValidator { Logger _logger; public TestValidator(Logger logger) { _logger = logger; } protected override void PostInitialize() { _logger.Log("Initializing"); } } ``` Если создать объект типа _TestValidator_, будет выброшено исключение типа _NullReferenceException_\. Это произойдёт из\-за того, что при создании объекта сначала отработает конструктор базового класса, а уже потом конструктор _TestValidator_\. Следовательно, при вызове метода _Log_ поле _\_logger_ будет иметь значение _null_\. Однако в проверяемом проекте ни один из классов не переопределяет метод _PostInitialize_\. Следовательно, никакого исключения здесь возникнуть не может\. Но это пока\. ## Заключение По итогу проверки можно сказать, что код оказался достаточно чистым, но не идеальным :\) Мне кажется, разработчикам стоит обратить внимание на те проблемы, которые описаны в статье\. Стоит отметить, что для публикации были отобраны наиболее интересные срабатывания\. Следовательно, потенциальных проблем несколько больше, чем описано здесь\. Для проверки интересующего вас проекта можно [бесплатно попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.