# Invincible null: digging into nopCommerce source code

The nopCommerce project is a free open\-source platform on ASP\.NET Core to create online stores\. Today we'll find out what ambiguities lurk in its code\.

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

## Few words about project

In software development, code quality plays a key role in the reliability, security, and performance of software products\.

The nopCommerce project is an open\-source eCommerce solution based on ASP\.NET Core\. It's one of the leading tools in the field\. Even though projects may have an excellent reputation and wide distribution, it's still important to pay attention and analyze to code quality issues\.

I took the code from this [commit](https://github.com/nopSolutions/nopCommerce/tree/45d416f9936eb3a446d5193c61930f0a3b3c9a79) and used [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) 7\.29 for analysis\.

Here, in the article, you can explore not only error but also some moot analyzer warnings\.

Enjoy the reading\!

## Brief aside

Our team frequently encounters [NRE](https://pvs-studio.com/en/blog/terms/6694/)\-related bugs\. Here are the links to the cases we've previously covered in our articles:

* [the V3080 diagnostic rule cases](https://pvs-studio.com/en/blog/examples/v3080/)
* [the V3095 diagnostic rule cases](https://pvs-studio.com/en/blog/examples/v3095/)
* [the V3105 diagnostic rule cases](https://pvs-studio.com/en/blog/examples/v3105/)\.

These are just some of the diagnostic rules that indicate potential null dereference\.

As with many other projects, most of the nopCommerce warnings mentioned here are NRE\-related\.

In some cases, a null dereference may be not a big issue\. Developers may handle these cases or just catch an exception during testing\. Let's be honest, tests may not cover every possible scenario of the program operation\. An error may occur in code that is rarely executed, but users will eventually find it\. I suppose the scenario is highly unwanted\. That's why it's better to consider the project safety carefully to prevent such cases\.

## Suspicious Equals

Recently, we have often encountered errors related to the implementation of the _Equals_ method\. We've previously shown one such case in the [article](https://pvs-studio.com/en/blog/posts/csharp/1032/)\.

In fact, we've made such a mistake too when implementing the diagnostic rule\. By the way, my colleagues have found the error in time and used it as the resource for a new diagnostic rule\. It's about checking for the incorrect type in the overridden _Equals_\.

The error in the example is common but still dangerous\.

**Fragment 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;        // <=
  }
}
```

The PVS\-Studio warning: [V3062](https://pvs-studio.com/en/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

Look at the call to the _Equals_ method in the overridden _Equals_ body\. You can see that the method is called for the _other_ variable_\._ It's also passed as a parameter\. It means that the argument is compared to itself\. I doubt that the developers have supposed the _Equals_ method to operate like that\. 

To fix the error, we can pass _this_ instead of _other_ as the _Equals_ argument\.

## Unused values

Unused values don't always cause errors that lead to exceptions or changes the program logic\. However, these issues can arise as well\. In any case, we need to avoid them\. At least it'll make the code cleaner and may help prevent incorrect program behavior\.

Below are code fragments that contain unused values\.

**Fragment 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);

  ....
}
```

The PVS\-Studio warning: [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) The 'pickupPointModel\.PickupFee' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 210, 203\. CheckoutModelFactory\.cs 210

After assigning a value to _pickupPointModel\.PickupFee_, the property isn't used until the next time the value is overwritten\. Such an assignment may make sense if the _set_ property accessor has special logic\. However, this is not the case here: _pickupPointModel\.PickupFee_ is a usual auto property\. It turns out that the content of the _then_ branch of the _if_ statement doesn't affect the program logic in any way\.

**Fragment 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();                          // <=
  }

  ....
}
```

The PVS\-Studio warning: [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Distinct' is required to be utilized\. OrderReportService\.cs 342

You can use _Distinct_ \(the LINQ method\) to delete repeating collection items\. That's what the developers wanted to do in this code, but something went wrong\. The _Distinct_ method doesn't modify the collection for which it's called\. So, if you don't use the return value of the method, the call is meaningless\. This is exactly the case of the code snippet\.

Most likely, the result of the _Distinct_ execution should be assigned to the _query_ variable\.

## Issues with null

Here are the classic errors \(if they can be called that\)\. There isn't much to add\. Everyone knows NRE\.

**Fragment 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);
  }

  ....
}
```

The PVS\-Studio warning: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'taxRates' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. AvalaraTaxProvider\.cs 113

When getting a value for _taxRates_, the _transaction\.summary_ property is accessed using the '?\.' operator\. The developer may have suggested that the property value could be _null_\. If this is the case, _null_ can be assigned to _taxRates_\. After initializing _taxRates_, the variable is used as a collection and the collection is iterated over in _foreach_\. If _taxRates_ is _null_, _NullReferenceException_ will be thrown\. It happens because the _GetEnumerator_ method is called on the collection implicitly\.

It's worth noting that this error pattern is fairly common\. We've already discussed it in the [article](https://pvs-studio.com/en/blog/posts/csharp/0832/)\.

**Fragment 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);

  ....
}
```

The PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'configuration' object was used after it was verified against null\. Check lines: 139, 135\. GoogleAuthenticatorController\.cs 139

The _configuration_ variable is checked for _null_ before the first use\. However, it isn't checked for _null_ on subsequent uses\. Note that the _GetConfigurationByIdAsync_ method used to get the variable value may return _null_\. The developers may have thought that _null_ wouldn't be returned here\. Then it's not quite clear why the check for _null_ is needed\. Otherwise, a null dereference may cause an exception\.

**Fragment 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);
  ....
}
```

The PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'refundPaymentRequest\.Order' object was used before it was verified against null\. Check lines: 597, 600\. CyberSourceService\.cs 597

Pay attention to the _refundPaymentRequest\.Order_ property\. It was checked for _null_ six times and used seven times\. Something doesn't add up\. It's suspicious that _refundPaymentRequest\.Order_ is called without '?\.' in the _if_ statement\. Maybe, the statement can't be _null_ in the context of the method\. Then it's worth deleting the check in other cases\. If _refundPaymentRequest\.Order_ may be _null_, then sooner or later the _RefundAsync_ call will cause _NullReferenceException_\.

## No more than one iteration\.

How often do you use _while_ instead of _if_? Rarely, I think\.

Here is a very unusual example of using _while_\.

**Fragment 7**

```cpp
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;                                            // <=
  }

  ....
}
```

The PVS\-Studio warning: [V3020](https://pvs-studio.com/en/docs/warnings/v3020/) An unconditional 'return' within a loop\. ConcurrentTrie\.cs 230

The analyzer doubts whether the loop is correctly implemented\. Let's find out what's wrong\. The loop body contains the _return_ operator issued without a condition\. This is not always an error, as there may be the _continue_ statements before _return_\. Because of this, _return_ won't necessarily be executed at the first loop iteration\. However, there is no _continue_\. Exiting the loop will always be done at the first iteration\.

There are options for exiting the loop:

* with _return_, which is in the body of one of the _if_ statements;
* with _break_ \(also in the _if_ body\);
* with _return_ at the very end of the loop\.

It's hard to say under what condition the loop should actually exit\. But we can definitely say that a loop that has no more than one iteration looks very strange\.

It may be a typo, and the _continue_ statement should be used instead of _break_\. There is even a hint in the comments: "try again"\.

## Suspicious check

It's a classic copy\-paste error\.

**Fragment 8**

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

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

  ....
}
```

The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\(conditionAttributeXml\)' is always false\. AttributeParser\.cs 499

Note the second _if_ statement\. It checks the value of the _conditionAttributeXml_ field\. This looks quite odd, since the previous _if_ checked the same field\. Obviously, in one of such cases, the _selectedAttributesXml_ parameter should be the argument of the _IsNullOrEmpty_ method\.

## Are they false positives?

We can't say that all analyzer warnings are necessarily false or, on the contrary, that all of them definitely indicate an error\. There are the cases that will be discussed here\.

If the analyzer is uncertain about the code, it will likely confuse the programmers who will maintain it\. Such warnings are a good reason for refactoring\. By the way, we have [an article about it](https://pvs-studio.com/en/blog/posts/cpp/0968/)\.

**Fragment 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;
  }

  ....
}
```

The PVS\-Studio warnings: 

* [V3005](https://pvs-studio.com/en/docs/warnings/v3005/) The 'priceModel\.OldPriceValue' variable is assigned to itself\. ProductModelFactory\.cs 503
* [V3005](https://pvs-studio.com/en/docs/warnings/v3005/) The 'priceModel\.PriceValue' variable is assigned to itself\. ProductModelFactory\.cs 505

The analyzer reports that _priceModel\.OldPriceValue_ and _priceModel\.PriceValue_ are assigned to themselves\. Most likely, there is no error here, but the analyzer warning cannot be called false either\. How did that happen? The point is that the code chunk is redundant\. If you delete assignments to the _priceModel\.OldPriceValue_ and _priceModel\.PriceValue_ variables, the program logic won't change\. 

Now the question arises: have developers intended that properties should be assigned the current value or not? If so, why only these properties?

To reduce the number of questions, there are two things you can do: 

* delete redundant assignments;
* leave a comment confirming that the assignments are correct\.

Both options will make the code a little better :\)

**Fragment 10**

```cpp
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()
  {
  }

  ....
}
```

The PVS\-Studio warning: [V3068](https://pvs-studio.com/en/docs/warnings/v3068/) Calling overrideable class member 'PostInitialize' from constructor is dangerous\. BaseNopValidator\.cs 20

We can't say that the warning indicates an error\. Why? To answer the question, we need to understand the essence of the warning\. 

Let's imagine that we have a child of the _BaseNopValidator_ class, for example, _TestValidator_, which overrides the _PostInitialize_ method:

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

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

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

If we create an object of the _TestValidator_ type, _NullReferenceException_ will be thrown\. This will happen because when an object is created, the base class constructor will be executed first, and then the _TestValidator_ constructor\. So, when the _Log_ method is called, the _\_logger_ field will be _null_\.

However, none of the classes overrides the _PostInitialize_ method in the project\. Hence, no exception arises\. But that's for now\.

## Conclusion

So, we can say that the code is quite clean but still not perfect :\)

I think it would be great if the developers paid attention to the issues described in the article\. Please note that the warnings I selected for the article are the most interesting\. The code has more issues than described\.

You can [try PVS\-Studio for free](https://pvs-studio.com/en/pvs-studio/try-free/) to check the project you're interested in\.