Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetModelContentType throws ArgumentNullException #222

Open
ronaldbarendse opened this issue Oct 4, 2019 · 4 comments
Open

GetModelContentType throws ArgumentNullException #222

ronaldbarendse opened this issue Oct 4, 2019 · 4 comments
Labels
state/backlog Backlog, To Do type/discuss Discussion, Question

Comments

@ronaldbarendse
Copy link

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
Copy link
Author

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
Copy link
Author

Retrieving the property alias doesn't require an UmbracoContext, so I've created a PR to allow this: #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 zpqrtbnk added state/backlog Backlog, To Do type/discuss Discussion, Question labels Oct 4, 2019
@zpqrtbnk
Copy link
Collaborator

zpqrtbnk commented Oct 4, 2019

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
Copy link
Author

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/backlog Backlog, To Do type/discuss Discussion, Question
Projects
None yet
Development

No branches or pull requests

2 participants