U4-10128 - Empty Node Key (Guid) in content

Created by Alessandro Calzavara 07 Jul 2017, 13:36:25 Updated by Stephan 28 Jul 2017, 07:44:06

Installed umbraco 7.6.3 with fanoe.

I tried to print the Key of the current node on a view, but it's always empty

@Model.Content.GetKey() // this prints all zeroes, 00000000-0000-0000-0000-000000000000

var withKey = Model.Content as IPublishedContentWithKey; // withKey is null

Same behavior with nodes from Umbraco.TypedContent(idPage)

The only way to get the correct key was to hit the db via ContentService

Comments

Sebastiaan Janssen 07 Jul 2017, 13:46:50

Try this:

https://github.com/Shazwazza/Articulate/blob/master/src/Articulate/Models/PublishedContentExtensions.cs#L52


Alessandro Calzavara 07 Jul 2017, 14:07:01

Thanks, '''unwrapping it before calling GetKey() worked'''. Is there any documentation (apart source code itself) or post on what is this wrap/unwrap about?


Dan Booth 19 Jul 2017, 08:04:32

@sebastiaan This should be treated as a bug, IMO. If you call the GetKey() method on IPublishedContent you should get a key, not an empty Guid. The fact there is some obtuse and undocumented workaround really isn't the point.

If Umbraco is to move to using Guids then it needs this to be as painless as using Ids. Key should just be a property, like Id.


Dan Booth 19 Jul 2017, 08:43:23

For anyone else stumbling on this, I found this three extension methods make working with GUIDs and UDI much less painful. I'd hope something similar would find their way into core?

///

/// Gets the published content with a guaranteed key /// /// The published content /// A published content with key public static IPublishedContentWithKey GetContentWithKey(this IPublishedContent content) { var withKey = content as IPublishedContentWithKey;

        if (withKey != null)
            return withKey;

        var wrapped = content as PublishedContentWrapped;

        if (wrapped != null)
            return GetContentWithKey(wrapped.Unwrap());

        return null;
    }

The following method should guarantee to return a GUID key from IPublishedContent

///

/// Guaranteed to return a GUID /// /// The published content /// A Guid public static Guid GetKeyGuaranteed(this IPublishedContent content) { var contentWithKey = content.GetContentWithKey(); return contentWithKey != null ? contentWithKey.GetKey() : Guid.Empty; }

The following extension method should return a UDI from IPublishedContent

///

/// Gets an Umbraco UDI /// /// The published content /// The type of UDI to create /// An Umbraco Document Identifier public static Udi GetUdi(this IPublishedContent content, UmbracoObjectTypes objectType) { string type = Constants.UdiEntityType.FromUmbracoObjectType(objectType); return Udi.Create(type, content.GetKeyGuaranteed()); }


Stephan 19 Jul 2017, 09:57:08

''Is there any documentation (apart source code itself) or post on what is this wrap/unwrap about?''

Few things:

The content cache internally produces IPublishedContent instances. Then we decided to add the Key property to published content, but due to backward compatibility we could not modify IPublishedContent and had to create the IPublishedContentWithKey interface.

Then, models (created with models builder) come on top of the IPublishedContent returned by the content cache. They ''wrap'' the original IPublishedContent and add some sugar (strongly typed properties, etc). The PublishedContentWrapped (and -WithKey) class is used as a base class to wrap, ie as a base class to models.

Because of all this, getting the Key of an IPublishedContent is tricky. We need to see if the IPublishedContent is, in reality, an IPublishedContentWithKey. But we also need to unwrap down to the very original IPublishedContent coming from the content cache (before models were created) to get the key.

And GetKey wasn't doing it properly. Should be fixed with the following PR:

PR https://github.com/umbraco/Umbraco-CMS/pull/2058

Review: PublishedContentTests.GetKey() should pass


Alessandro Calzavara 19 Jul 2017, 10:05:52

@zpqrtbnk Now that makes sense, thanks for the explanation!


Sebastiaan Janssen 19 Jul 2017, 10:26:58

Thanks @DanDiplo for keeping me sharp, should indeed not have closed this! Should all work in 7.6.5!


Dan Booth 19 Jul 2017, 10:38:22

@sebastiaan No worries - I'm using it all in anger now, so there'll probably be more :p Thanks for fixing so fast!


Dan Booth 28 Jul 2017, 07:31:58

@zpqrtbnk

"Then we decided to add the Key property to published content, but due to backward compatibility we could not modify IPublishedContent and had to create the IPublishedContentWithKey interface."

I've been thinking about this. You obviously are privy to a lot more knowledge than this, so forgive my probably uneducated comments, but it feels like this was a mistake. It seems better to bite the bullet now and extend the interface now rather than forever be stuck with two similar implementations.

Adding a Key property to IPublishedContent would kept everything working as people currently expect. I'm not sure what backward compatibility would have broken - I'm pretty sure that in 99% of cases the only code that directly references the interface is your own code. For the small fraction of developers who do implement their own classes on IPublishedContent directly (rather than a derived class) then I'm sure they could add a new property (even if it returned an empty GUID) and recompile.


Stephan 28 Jul 2017, 07:44:06

Ah, you are entirely forgiven, for a rather sensible comment. The decision was not an obvious one, and I certainly would not argue that the current situation is "the best". Backward compatibility is a pain.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.6.3

Due in version: 7.6.5

Sprint: Sprint 63

Story Points: 1

Cycle: 3