modelsbuilder / ModelsBuilder.Original

The Community Models Builder for Umbraco
MIT License
114 stars 49 forks source link

GetModelContentType throws ArgumentNullException #222

Open ronaldbarendse opened 5 years ago

ronaldbarendse commented 5 years ago

While trying to get a property alias within Examines TransformingIndexValues event using Project.GetModelPropertyType(p => p.Products).Alias, I get an ArgumentNullException.

Looking at the source, this method requires an UmbracoContext that's not available while this event is fired. The fixme inject! comment probably says enough:

https://github.com/zpqrtbnk/Zbu.ModelsBuilder/blob/bf0da79aee8f52ec153bbb200131d1914ee95b2b/src/Umbraco.ModelsBuilder/Umbraco/PublishedModelUtility.cs#L27-L33

It should probably inject an IUmbracoContextAccessor somewhere and ensure the context exists, allow passing in a custom IPublishedSnapshot or at least return null instead of throwing this exception.

ronaldbarendse commented 5 years ago

So IUmbracoContextAccessor only returns the context and does not create it: it actually needs an IUmbracoContextFactory.

To work around this issue, I've now surrounded the call with an injected IUmbracoContextFactory myself:

string productsAlias = null;
using (umbracoContextFactory.EnsureUmbracoContext())
{
   productsAlias = Project.GetModelPropertyType(p => p.Products).Alias;
}
ronaldbarendse commented 5 years ago

Retrieving the property alias doesn't require an UmbracoContext, so I've created a PR to allow this: https://github.com/zpqrtbnk/Zbu.ModelsBuilder/pull/223.

It doesn't fix this issue, but at least allows you to retrieve the property alias in a generic/type safe way without all the overhead.

zpqrtbnk commented 5 years ago

It sure is bad to have to create a context to retrieve a property alias - see my answer on #223 - need to fix this.

As for why a context is needed to retrieve a property type - at publisher's level, content types as well as documents are accessed through a snapshot which guarantees that they'll remain stable for the duration of the snapshot.

For instance, if you retrieve a document 3 times in a row, you expect it to be the same all 3 times. Not to change, or worse, disappear. Same with content types. Things should be stable for the duration of a request, or, a snapshot. And the context creates and owns the snapshot. Reason why you need a context.

Now I am wondering, for these metadata methods... we could wrap their code in a using (umbracoContextFactory.EnsureUmbracoContext()) block. If a context already exists, it will use it, but if no context exists, it will create one. You would lose the stability of a snapshot, but... do you need it here?

ronaldbarendse commented 5 years ago

Thanks for the elaborate explanation @zpqrtbnk!

Having a way to get the property alias without the context and throwing a descriptive exception when using this method without one looks like the best solution then 👍