Custom Processor Contexts only persist a single .As()

[drooks]

Per this blog https://jamiepollock.github.io/blog/2016/06/15/getting-started-with-ditto-processors

When passing a custom context as processorContexts to the root .As call, the custom processor is getting null values from the context. It appears that the processorContexts isn't being passed to subsequent .As() calls specifically at DittoFactoryAttribute.cs(line:112).

[mattbrailsford]

Good find!

I'm wondering if we may need to create an additional "Context" object for the overall conversion as a whole and stash them there. We currently have contexts for the individual conversions, but as we see here, it's possible for a single conversion to cause multiple conversions, so I think a context object for the entire conversion sequence would help so that we can store and access these values throughout.

[drooks]

Thanks!

I forked a version and rolled the DittoMultiProcessorContext forward as the processorContexts that I assembled from ContextCache. Problem being it isn't a true stack of processorContexts and includes all cached contexts, which could break if the context type is used differently in the process flow. Needless to say, a fix for this would be fantastic so I can throw away my hack of a fork.

[Nicholas-Westby]

Just thought I'd mention the fork for quick reference, which is here: https://github.com/rhythmagency/umbraco-ditto

The commit is here: https://github.com/rhythmagency/umbraco-ditto/commit/52a7868f38c464f7b834d1337aa2098799fe4bc1

[jamiepollock]

I believe I had a similar situation when using a custom DittoFactoryAttribute. Passing a context in at controller level when we call model.As<TPoco>(contexts) but the context didn't pull through to the found POCO class after the factory resolved the type name.

I wasn't sure if this was related, but it might be given the fact that it was being used with Nested Content and the DittoFactoryAttribute would trigger another model.As<TPoco>() off the UmbracoPropertyAttribute. :)

[Nicholas-Westby]

@mattbrailsford FYI, I just gave this a test with MyGet and it didn't seem to fix the issue (some of my data was missing). I downgraded back to the custom build of Ditto created by @drooks and the data was fine again.

Here's the MyGet feed I was using: https://www.myget.org/gallery/umbraco-ditto

Latest version, which is 0.11.0-alpha-000685.

[mattbrailsford]

Would be useful if you could tell us your model setup so we can replicate it with a unit test.

On 1 Feb 2017 2:49 a.m., "Nicholas-Westby" notifications@github.com wrote:

@mattbrailsford https://github.com/mattbrailsford FYI, I just gave this a test with MyGet and it didn't seem to fix the issue (some of my data was missing). I downgraded back to the custom build of Ditto created by @drooks https://github.com/drooks and the data was fine again.

Here's the MyGet feed I was using: https://www.myget.org/gallery/ umbraco-ditto

Latest version, which is 0.11.0-alpha-000685.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/leekelleher/umbraco-ditto/issues/201#issuecomment-276559085, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgLyRPNrbZw1cXpwk0YrH08mhEI1X2Sks5rX_K_gaJpZM4Le9r2 .

[Nicholas-Westby]

@mattbrailsford I created a repo that demonstrates the bug: https://github.com/Nicholas-Westby/ditto-context-bug

Clone, run the website from Visual Studio 2015, and the homepage should look like this:

Interestingly, I set a breakpoint here: https://github.com/Nicholas-Westby/ditto-context-bug/blob/2b555f80ef456c0b4daa917193eb955d951ab981/App/DataContentAsAttribute.cs#L28

And I noticed there was an instance of ContentDataContext, but that its DataContent property was null. Maybe it's being reinstantiated rather than using the previously instantiated instance of ContentDataContext? Here's where it gets instantiated initially: https://github.com/Nicholas-Westby/ditto-context-bug/blob/2b555f80ef456c0b4daa917193eb955d951ab981/App/MappingHelper.cs#L11

If you want to log into Umbraco, the username is "admin" and the password is "adminadmin". To save you a bit of time, here's what the homepage looks like on the back office:

FYI, I am aware there are other ways to accomplish what I'm doing in this particular case, but this is just a contrived example to demonstrate the bug.

[mattbrailsford]

Ok, I've found the reason for this.

So the way the processor caching works is, when .As is called, it asks Ditto to create a new ChainContext (this is where we cache the processor contexts). Each inner .As call also asks to create one, but what happens is only the first request creates one, all inner requests just increment a counter. When each .As call completes, a request is made to tell Ditto it is done. What this does internally is to decrements the count, and only when it hits 0 does it destroy the ChainContext, this way all .As calls made inside another .As call gets access to the same processor contexts.

The problem is, when a processor returns an un-evaluated Enumerable (such as from a Select statement, which in this instance is your very problem https://github.com/leekelleher/umbraco-ditto/blob/develop/src/Our.Umbraco.Ditto/ComponentModel/Processors/DittoFactoryAttribute.cs#L110) the calls to the inner .As statements doesn't occur in sequence. In this instance, the parent .As statement is completing, thus deleting the processor context cache, and THEN you enumerate the collection, causing evaluation which then triggers the inner .As calls, however by this time, the context is gone, so it's creating a new one which no longer contains your passed in contexts.

Now, a simple fix for you issue is we can call .ToList on that very select statement causing the collection to instantly evaluate in the correct context, however my concern is that other may return Enumerables from other processors which will ultimately cause the same issue.

Right now, I'm not sure the best course of action as I know of no way that we could "cache" those items bound to the enumerable such that they are available at the point of enumeration, and even if we do, it's doing it in such a way that works for other processors without needing them to do something special.

Anyone got any suggestions?

Matt

[mattbrailsford]

Thinking about it, this is probably also going to be an issue for our virtual properties proxying too, however, given we are in control of those proxies, I'm guessing we should be able to somehow extend the proxy to stash the contexts. @JimBobSquarePants does this sound possible? and do you have any thoughts for how to handle Enumerables?

[Nicholas-Westby]

I've actually been noticing a bit of weirdness (i.e., non-deterministic mapping) lately that has seemed to be resolved by calling ToList() or ToArray() (which I did under the assumption that deferred execution was causing some subtle problems).

I don't fully understand the ChainContext concept/purpose. Is it to ensure some objects get destroyed once there are no more references?

[mattbrailsford]

The purpose of the chain context is to maintain context across an entire processors chain (which is the issue reported here). So if you call .As inside a processor, it should have access to all the registered processors contexts passed in to the first .As call.

I guess the issue is I'm trying to prevent the need for devs to pass though the contexts manually, ie, if you wrote a processor, I'd like you to be able to just do .As(type) rather than having to pass through contexts like .As(type, processorContexts:ProcessorContexts) as people will just forget to do it and it only takes one low lever processor to forget this and it screws up the entire chain, but this may have to be the route we take if I can't think of a fix to my current solution.

[Nicholas-Westby]

I see. I think I understand a little better now. One thing to consider is that this ChainContext may cause unintended contamination of other mapping contexts.

For example, suppose I am mapping some widgets using As(), and that may in turn cause some nested calls to As(). In that case, this ChainContext should work as expected.

However, assume that one of my processors in turn calls some static function, GetSomeSetting(). Maybe that static function also happens to call As() too. Ideally, that GetSomeSetting() call to As() should have no concept of what calls it, but assuming ChainContext functions as I'm interpreting it to function, it would actually assume that the call to As() in GetSomeSetting() is a nested call to As(), and so it would essentially inherit the processor contexts from the widget mapping process. That may lead to unintended side effects.

If this is correct, I'd suggest that maybe processor contexts should be passed to As() explicitly.

[mattbrailsford]

@Nicholas-Westby see PR #207 for a more explicit implementation. I've tested it against your demo project and it works as you'd expect. If anyone else wants to take a look and let me know their thoughts.

[JimBobSquarePants]

I'm guessing we should be able to somehow extend the proxy to stash the contexts. @JimBobSquarePants does this sound possible?

Honestly @mattbrailsford I have no idea and I don't think that should be the responsibility of a proxy class.

Truth be told I don't like the processor contexts as I feel they open Ditto up to abuse. The example given in the blog post I believe does that.

I feel strongly that Ditto is now suffering badly from feature creep. Ditto, in my opinion, should either return a 1-1 model of the back office node or a subset of those properties in the back office. Trying to create a hybrid ViewModel of data from different context should be a definite no-no. If you want to combine data from different source, use a controller and a view model combining your separate models. The C in MVC is important. I'd hate to see the performance metrics of sites doing this kind of stuff.

I've used Ditto to build every Umbraco site for years now (A lot of sites, some incredibly complicated) and I've never required this kind of functionality. Why? because I keep logic where it belongs. The sites are fast, the code base is easily maintainable and logic isn't obscured.

In short, use Ditto to map your back-office model only. Keep any other logic in the controller.

Don't get me started on mutating properties... That StringLengthProcessor in the aforementioned blog post gives me the heebie-jeebies.

[jamiepollock]

Not going too off point but @JimBobSquarePants I agree with you to an extent. That blog post which I wrote was from just after 0.9 was released. I certainly wasn't best practices at all, in fact I barely used it in anger. The point was to get something out there as Processors were new and exciting.

Maybe there is a call to simplify Ditto to an extent. I certainly felt that way with #204 as adding things in like Umbraco's ServiceContext allow access to CRUD operations which shouldn't be. I mean it's there to be abused but hopefully people won't use them.

Thanks for your input. I'm glad I was able to read the words heebie-jeebies, it made me smile ;)

[JimBobSquarePants]

Good to hear @jamiepollock 😄

Processors are definitely the best thing since sliced bread. I just worry now that they are being used to mix concerns in a way we shouldn't encourage and certainly shouldn't extend the library to allow.

[mattbrailsford]

@JimBobSquarePants Having slept on it, I think you are right about the proxy stuff. I think going the explicit route is the way to go. We'll just have to document it's use.

I hear what you are saying about feature creep, but the ability for processors to perform logic is very much intentional. Their intent allows logic to be easily encapsulated helping maintain SOC. I agree, this can be taken too far and for tasks to be broken down too granular, but it's possible for people to write bad MVC as well. That said, the beauty of Ditto is it's flexible to be used in both ways.

[leekelleher]

Closing issue, as PR #207 has now been merged in.