umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.54k stars 2.71k forks source link

IPublishedContent.HasValue() throws exception if segment is null #14954

Open benmurphycb opened 1 year ago

benmurphycb commented 1 year ago

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

10.4.2

Bug summary

We are using uMarketingSuite.

Our solution has a sitemap generator, where we loop through valid nodes and try to check if the IPublishedContent has a value:

var hideFromSitemap = content.HasValue("hideFromSitemap") 
    ? content.Value<bool>("hideFromSitemap")
    : false;

This throws an exception: Value cannot be null. (Parameter 'key1') Unless we specify the segment as an empty string when trying to use .HasValue / .Value

Specifics

I have managed to track this back in the Umbraco code: The issue is with the Umbraco.Cms.Infrastructure.PublishedCache.GetSourceValue(string? culture = null, string? segment = null) method - the return statement is:

return _sourceValues.TryGetValue(
    new CompositeStringStringKey(culture, segment),
    out SourceInterValue? sourceValue)
    ? sourceValue.SourceValue
    : null;

The issue is that when newing up the CompositeStringStringKey, the segment is still null and not an empty string. The CompositeStringStringKey is expecting a string, not a nullable string.

From what I can see - the segment should be set by this line: _content.VariationContextAccessor.ContextualizeVariation(_variations, _content.Id, ref culture, ref segment);

But this method (Umbraco.Extensions.VariationContextAccessorExtensions.ContextualizeVariation()) will not always assign this to an empty string if it cannot get the segment.

The null coalescing operator in this code:

segment = contentId == null
    ? publishedVariationContext?.Segment
    : publishedVariationContext?.GetSegment(contentId.Value);

means that it can still end up being null, which then causes the exception to be thrown with the CompositeStringStringKey initialization mentioned above.

Is this an issue with how the site is set up, or should the code be:

segment = contentId == null
    ? publishedVariationContext?.Segment ?? ""
    : publishedVariationContext?.GetSegment(contentId.Value) ?? "";

?

image

image

Steps to reproduce

Using Umbraco 10.4.2

  1. Create a site that has document types that vary by culture and segment.
  2. Create a sitemap generator that works as middleware that runs when the following route is hit "/sitemap.xml"
  3. This sitemap generator takes a start node and culture, then uses _umbracoContextFactory.EnsureUmbracoContext() to get the context, before recursively getting the relevant nodes and child nodes and adding these to a list.
  4. Iterate through this list, and try check if each node has the hideFromSitemap property: IPublishedContent.HasValue("hideFromSitemap")
  5. If it does, get this value as a bool
  6. If not, default to false

When using the .HasValue / .Value methods the exception should be thrown.

Expected result / actual result

The segment falls back to an empty string if it cannot be found so that it can be used in a CompositeStringStringKey.

github-actions[bot] commented 1 year ago

Hi there @benmurphycb!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

kjac commented 1 year ago

Hi @benmurphycb,

Thank you for reaching out and for your very detailed description and research 💪

However, I can't quite reproduce the issue.

First and foremost I haven't installed uMarketingSuite, just to make sure I'm only testing Umbraco.

Next, I have enabled segments in code and created a doctype with segments and various permutations of properties

image

image

Next I'm outputting the content using a template like this:

@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage
@{
    Layout = null;
}
<html lang="en">
<head>
    <title>Issue 14954 test template</title>
</head>
<body>
<ul>
    @foreach (var property in Model.ContentType.PropertyTypes)
    {
        <li>Property <b>@property.Alias</b> has value: <b>@Model.HasValue(property.Alias)</b></li>
    }
</ul>
<ul>
    @foreach (var property in Model.Properties.Where(property => property.HasValue()))
    {
        <li>Property <b>@property.Alias</b> value is: <b>@Model.Value(property.Alias)</b></li>
    }
</ul>
</body>
</html>

...and a RenderController that applies segments from a segment query parameter. It all seems to work just fine:

14954.webm

It also works fine with content that is non-variant (outputs the same across all of the segments).

I'm testing on 10.7, but I doubt that things around segment handling have changed from 10.4. Question is... did I misunderstand your issue, or would the above approach yield the same error on your site?

kasparboelkjeldsen commented 10 months ago

We are seeing the same error on some properties on an Umbraco 8 upgraded to Umbraco 13.

HasValue gets called with culture = "" and segment = null and fails out on ContextualizeVariation If I set segment to "" while debugging we get through just fine and receive the expected null value.

No extensions or UMarketingSuite

SATC-Ben commented 9 months ago

We are also getting this issue in V13.0.3 when upgrading from 11 and after adding culture setting's to an existing site. It occurs when we run a custom indexing process and trying to access .Value<>

if(node.Value<bool>(_publishedValueFallback, "hideFromSearch"))

The error:

System.ArgumentNullException: Value cannot be null. (Parameter 'key1')
   at Umbraco.Cms.Core.Collections.CompositeStringStringKey..ctor(String key1, String key2)
   at Umbraco.Cms.Infrastructure.PublishedCache.Property.CacheValues.For(String culture, String segment)
   at Umbraco.Cms.Infrastructure.PublishedCache.Property.GetValue(String culture, String segment)
   at Umbraco.Extensions.PublishedPropertyExtension.Value[T](IPublishedProperty property, IPublishedValueFallback 
   ublishedValueFallback, String culture, String segment, Fallback fallback, T defaultValue)
   at Umbraco.Extensions.PublishedContentExtensions.Value[T](IPublishedContent content, IPublishedValueFallback 
   ublishedValueFallback, String alias, String culture, String segment, Fallback fallback, T defaultValue)
   at ...Models.Generated.BasePageComp.GetTags(IBasePageComp that, IPublishedValueFallback publishedValueFallback) in 
   ..\Models\Generated\BasePageComp.generated.cs:line 455
   at ...Models.Generated.Home.get_Tags() in ...\Models\Generated\Home.generated.cs:line 223
   at ...Components.Indexing.SearchContentIndexer.SetTagField(IndexingItemEventArgs e, IPublishedContent node) in 
   ..\Components\Indexing\SearchContentIndexer.cs:line 352

No extensions or UMarketingSuite either.

SlawekSkiba commented 7 months ago

Hi, Looks like I have the same exception after upgrading site from version 10.8.4 with uMarketingSuite to latest 13.2.2.

I'm extending an index, so while accessing value in specified language item.Value<string>(SearchResultFieldNames.PageMetaTitle, language) I'm getting following exception: item.Value<string>(SearchResultFieldNames.PageMetaTitle, language)' threw an exception of type 'System.ArgumentNullException' Data: {System.Collections.ListDictionaryInternal} HResult: -2147467261 HelpLink: null InnerException: null Message: "Value cannot be null. (Parameter 'key2')" ParamName: "key2" Source: "Umbraco.Core" StackTrace: " at Umbraco.Cms.Core.Collections.CompositeStringStringKey..ctor(String key1, String key2)\r\n at Umbraco.Cms.Infrastructure.PublishedCache.Property.GetSourceValue(String culture, String segment)\r\n at Umbraco.Cms.Infrastructure.PublishedCache.Property.HasValue(String culture, String segment)\r\n at Umbraco.Extensions.PublishedContentExtensions.Value[T](IPublishedContent content, IPublishedValueFallback publishedValueFallback, String alias, String culture, String segment, Fallback fallback, T defaultValue)\r\n at Umbraco.Extensions.FriendlyPublishedContentExtensions.Value[T](IPublishedContent content, String alias, String culture, String segment, Fallback fallback, T defaultValue)" TargetSite: {Void .ctor(System.String, System.String)}

Do someone have any solution for this?

SlawekSkiba commented 5 months ago

Any news about this problem?