umbraco / Umbraco.Commerce.Issues

17 stars 2 forks source link

IProductSnapshot.TryCalculatePrice() throws NullReferenceException #516

Open robertjf opened 5 months ago

robertjf commented 5 months ago

Describe the bug We've come across a situation where IProductSnapshot.TryCalculatePrice() throws a NullReferenceException on occasion. It seems most common for bots, but also some non-bot users are experiencing the same (this is being reported by RayGun).

The relevant stack trace is reporting the exception on line 179 of Umbraco.Commerce.Extensions.IProductSnapshotExtensions:

Message: Object reference not set to an instance of an object.
Umbraco.Commerce.Extensions.IProductSnapshotExtensions.TryCalculatePrice(IProductSnapshot product, SessionManagerAccessor sessionAccessor, IProductCalculator calculator, ITaxSourceFactory taxSourceFactory):179

Umbraco Commerce version:

Umbraco Version: 13.3.2

Umbraco.Commerce: v13.1.4

Installed Payment Providers

Installed Shipping Providers

mattbrailsford commented 5 months ago

Hmm, there is no line 179 for IProductSnapshotExtensions so DotNet is being super useful there 🤦‍♂️

Can you share the code that is calling TryCalculatePrice?

robertjf commented 5 months ago

@mattbrailsford here you go:


    public static IProductSnapshot AsCommerceProduct(this IPartCommerce content)
    {
        if (content is not IPublishedContent page)
            return null;

        var store = page.GetStore();

        return CommerceApi.Instance.GetProduct(store.Id, content.GetProductReference(), Thread.CurrentThread.CurrentCulture.Name);
    }

    public static Price? CalculatePrice(this IPartCommerce content)
    {
        try {
            return content.AsCommerceProduct()?.TryCalculatePrice().Result;
        } catch (NullReferenceException) {
            // Something went wrong in the calculation - most likely in SessionManagerAccessor (i.e., there is no Session!).
            return null;
        }
    }

IPartCommerce is a composition doctype that contains the price and stock properties etc. It is always going to be associated with a product, so the null check is more for convenience...

These lines start at line 35 in the file.

mattbrailsford commented 5 months ago

Hmm, looks ok to me.

Are you able to identify what makes the non-bot users see this? I could really do with knowing some replication steps so that I can run a test.