digging into the source code of nopCommerce

digging into the source code of nopCommerce

nopCommerce is a free and open-source online store building platform built on ASP.NET Core. Today we will find out what ambiguous moments are hidden in the code of the platform.

A few words about the project

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

nopCommerce is an open e-commerce platform based on ASP.NET Core. It is one of the leading tools in this field. However, even with an excellent reputation and widespread adoption, issues of code quality in projects remain important aspects that require attention and analysis.

A transition code corresponds to a commit. The PVS-Studio version 7.29 tool was used for the analysis.

It should be noted that the article will contain not only warnings that indicate errors, but also some ambiguous behavior of the analyzer.

Happy reading!

A small digression

Every time we come across a lot of errors related to NRE. In order not to be verbose, I will leave links to cases that we have already written about in articles:

And this is only part of the diagnostic rules that signal potential null reference dereferencing.

The nopCommerce project was no exception: most of the warnings discussed in the article are related to NREs.

Maybe in some cases dereferencing a null reference isn’t a big deal. This may be due to the fact that handling is written for such situations, or an exception will be detected at the testing stage. But let’s be honest, the tests do not always cover absolutely all scenarios of the program’s operation. An error can occur in code that is rarely executed and will be discovered by the user. In my opinion, such a scenario is extremely undesirable. That is why the project should be protected as much as possible from the development of events.

Suspicious Equals

Recently, quite often we have to face errors during the implementation of the method Equals. We have already shown one of such cases in the article.

Needless to say, we ourselves made such a mistake when implementing diagnostics. By the way, the error was discovered in time and led to the idea of ​​creating a new diagnostic rule. It has to do with checking for the wrong type when overriding Equals.

The error presented in this example is quite typical, but it does not become less dangerous.

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

Warning PVS-Studio: V3062 Another object ‘other’ is used as an argument to its method. The first live argument of ‘Equals’ method is believed to be tested. ImportManager.cs 3392

Note the method call Equals in the body of the overdetermined Equals. You can see that the method is called by a variable others It is passed as an argument. It turns out that the argument is compared with itself. I doubt that the developers put such logic into the operation of the method Equals.

To correct the error can be transmitted thisbut not otheras an argument Equals.

Unused values

Errors related to unused values ​​do not always lead to exceptions or changes in the logic of the program. However, such situations may arise. In any case, such moments should be avoided. At a minimum, this will make the code cleaner, and at a maximum, it will help prevent incorrect program behavior.

The following are code snippets that contain unused values.

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

  ....
}

Warning PVS-Studio: V3008 The ‘pickupPointModel.PickupFee’ variable is assigned values ​​​​twice successively. Perhaps this is a mistake. Check lines: 210, 203. CheckoutModelFactory.cs 210

After assignment to pickupPointModel.PickupFee some property value is unused until that value is overwritten. A similar assignment can make sense if the accessor set properties has a special logic. However, this is not the case, pickupPointModel.PickupFee – Normal autoproperty. It turns out that the content then– Branches of the operator if does not affect the logic of the program.

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

  ....
}

Warning PVS-Studio: V3010 Restoring the ‘Distinct’ function value is required for use. OrderReportService.cs 342

To remove duplicate elements of the collection, you can use Distinct (LINQ method). That’s what the developers wanted to do in this example, but something went wrong. Method Distinct does not modify the collection being called. So, if you don’t use the method’s return value, the call makes no sense. This is exactly the situation that occurred in the code under consideration.

Most likely, the result of execution Distinct must be assigned to a variable query.

Problems with null

Here are the classic, if you can call them, mistakes. Nothing to add in particular, everyone is familiar with NRE.

Fragment 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 Warning: V3105 The ‘taxRates’ variable was used after it was defined via the null-conditional operator. A NullReferenceException is possible. AvalaraTaxProvider.cs 113

When getting the value for taxRates an appeal to quality is made transaction.summary using the ‘?.’ operator. All things considered, the developer assumed that the value of this property could be null. If this is so, then in taxRates can be assigned null. Immediately after initialization taxRates this variable is used as the collection to iterate over foreach. If taxRates does matter nullthen an exception of type will be thrown NullReferenceException. This will happen because the collection implicitly calls the method GetEnumerator.

This is a fairly common error pattern. We have already talked about him in the article.

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

  ....
}

Warning PVS-Studio: V3125 The ‘configuration’ object was used after it was implemented after null. Check lines: 139, 135. GoogleAuthenticatorController.cs 139

Changeable configuration is checked on null before the first use, but not after the following ones. It is worth noting that the method GetConfigurationByIdAsyncwith which the value of this variable was obtained, can return null. Perhaps the developers are sure that in this case null will not be returned. Then it is not entirely clear why a check is needed null. Otherwise, an exception may occur due to the denaming of a null reference.

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

Warning PVS-Studio: V3095 The ‘refundPaymentRequest.Order’ object was used before it was committed to null. Check lines: 597, 600. CyberSourceService.cs 597

Pay attention to the property refundPaymentRequest.Order. It was tested on null six times, and seven were used. Something is not right here. It is suspicious that under the condition if address to refundPaymentRequest.Order is carried out without using ‘?.’. Maybe it can’t matter null in the context of this method. Then you should remove the check in other cases. If refundPaymentRequest.Order may be nullsooner or later a challenge RefundAsync will cause a type exception to be thrown NullReferenceException.

No more than one iteration

How often do you use while instead if? I think that is quite rare.

Here is a very unusual use case while.

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

  ....
}

Warning PVS-Studio: V3020 Excellent ‘return’ with pointer. ConcurrentTrie.cs 230

The analyzer doubts the correctness of the implementation of the cycle. Let’s understand what is wrong here. The statement is in the body of the loop return, which works without conditions. This is not always a mistake, as before return can be operators continue. Therefore return will not necessarily work at the first iteration of the cycle. However, in this case continue No. The loop will always exit at the first iteration.

Options for exiting the cycle:

  • or with the help of returnwhich is in the body of one of if;

  • or with the help of break (also in the body if);

  • or with the help of return at the end of the cycle.

It is difficult to say under what condition the cycle should actually end. But we can definitely say that a loop that has no more than one iteration looks very strange.

Alternatively, we may have a typographical error, and instead break operator must be used continue. This is hinted at by the phrase “try again” in the comment.

Suspicious check

A classic error is related to Copy-Paste.

Fragment 8

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

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

  ....
}

Warning PVS-Studio: V3022 Expression ‘string.IsNullOrEmpty(conditionAttributeXml)’ is always false. AttributeParser.cs 499

Pay attention to the second condition if. It checks the value of the field conditionAttributeXml. It looks very strange because in the previous one if this field was checked. Obviously, in one of the cases is an argument for the method IsNullOrEmpty must be a parameter selectedAttributesXml.

False positives?

Not every analyzer warning can be said to be false or, on the contrary, indicate an error. Such cases will be reflected in this section.

If the analyzer doubts the code, such code will almost certainly cause surprise in the programmers who will accompany it. Such warnings are a good reason for refactoring. By the way, we have a note on this.

Fragment 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 warning:

  • V3005 The ‘priceModel.OldPriceValue’ variable is assigned to itself. ProductModelFactory.cs 503

  • 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 operation of the analyzer cannot be called erroneous either. How did it happen? The fact is that part of the code is redundant. If you remove the assignment to variables priceModel.OldPriceValue and priceModel.PriceValuethe work logic will not change.

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

To reduce the number of questions (or none at all), you can do it in two ways:

  • remove unnecessary assignments;

  • leave a comment confirming the correctness of the current assignments.

Both options will make the code a little better 🙂

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

  ....
}

Warning PVS-Studio: V3068 Calling overrideable class member ‘PostInitialize’ from constructor is dangerous. BaseNopValidator.cs 20

Nor can this warning be said to indicate an error. Why? To answer this question, you need to understand the essence of the operation.

Suppose we have an inheritor class BaseNopValidatorexample, TestValidatorwhich overrides the method PostInitialize:

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

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

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

If you create an object of type TestValidatortype exception will be thrown NullReferenceException. This will happen due to the fact that when creating an object, the constructor of the base class will work first, and then the constructor TestValidator. So, when calling the method Log field _logger will matter null.

However, in the project under test, none of the classes override the method PostInitialize. Therefore, no exception can arise here. But that’s it for now.

Conclusion

According to the results of the check, we can say that the code turned out to be quite clean, but not ideal.

It seems to me that the developers should pay attention to the problems described in the article. It is worth noting that the most interesting reactions were selected for publication. So, there are a few more potential problems than described here.

You can try PVS-Studio for free to test the project you are interested in.

If you would like to share this article with an English-speaking audience, please use the translation link: Nikita Panevin. Optional null: Digging into nopCommerce source code.

Related posts