`CacheValues.For` should null check culture + segment parameters

[mattbrailsford]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

v13

Bug summary

CacheValues.For accepts nullable string culture and segment parameters but only performs an empty string check on them, not a null or empty check. This results in an exception because it then attempts to create a CompositeStringStringKey with the culture and segment but this object doesn't accept null arguments.

Specifics

I had an issue raised on the Umbraco Commerce issue tracker https://github.com/umbraco/Umbraco.Commerce.Issues/issues/536 which ultimately boils down to a call to PublishedCache.Property.GetValue(string culture, string segment) failing. In my instance I have no culture or segment data and so null is passed for both (which based on the method signature should be valid).

Internaly, GetValue calls GetCacheValues(PropertyType.CacheLevel).For(culture, segment) passing the culture and segment along. Again, this method states it can accept null values. With the For method call however it only performs an empty string check and so allows null values to pass through to the line which attempts to create a CompositeStringStringKey

public CacheValue For(string? culture, string? segment)
{
    if (culture == string.Empty && segment == string.Empty)
    {
        return this;
    }

    if (_values == null)
    {
        _values = new Dictionary<CompositeStringStringKey, CacheValue>();
    }

    var k = new CompositeStringStringKey(culture, segment);
    if (!_values.TryGetValue(k, out CacheValue? value))
    {
        _values[k] = value = new CacheValue();
    }

    return value;
}

This attempt now throws a null exception because the CompositeStringStingKey class does not accept null arguments.

I believe this relates to this issue also https://github.com/umbraco/Umbraco-CMS/issues/14954

Steps to reproduce

Take a look at the Umbraco Commerce issue tracker page.

Expected result / actual result

I would expect CacheValues.For to do a null or empty check on the cultute / segment and not just an empty string check.

---

This item has been added to our backlog AB#43585

[mattbrailsford]

Hmm, in fact, looking through the whole Umbraco.Cms.Infrastructure.PublishedCache.Property.cs file which this code resides in there are a number of locations where culture and segment are passed and only an empty string check is performed whilst all the method signatures accept nullable values.

I do believe all empty string checks should be updated to string.IsNullOrEmpty checks instead

[Migaroez]

Hey @mattbrailsford I will bring this up on our next team meeting.

[nikolajlauridsen]

Fixed in #17024 😄