Umbraco-cms: Services.UserService returns null in UmbracoApiController

[rsoeteman]

I have a UmbracoApiController and in a controller action I want to do the following where userId = 0
Services.UserService.GetUserById(userId);

This results in a Null Reference exception on the UserService.

Reproduction

1. Create a Controller that derives from UmbracoApiController
2. Create a Controller action and use Services.UserService.GetUserById(0);
3. You should see the exception.

I am using the latest code from the TempV8 branch

[kjac]

Interesting. Something is wrong with the DI.

The Services property on UmbracoApiControllerBase is null, despite being decorated with [Inject]. Same goes for UmbracoContext property.

[kjac]

A few more findings.

1. The DI doesn't work if the controller is dumped in App_Code.

2. The DI does work if the controller is part of the core assembly, e.g. hacked into Umbraco.Web:

namespace Umbraco.Web.Editors
{
    public class TestController : UmbracoApiController
    {
        public string GetSomething()
        {
            return Services.UserService.GetUserById(-1)?.Name ?? "No such user";
        }
    }
}

1. User 0 seems to be user -1 in V8:

[JimBobSquarePants]

Reads like a symptom of the temporal coupling I described here.

https://issues.umbraco.org/issue/U4-11427

Fixing the DI should be a priority imo.

[dawoe]

I know @lars-erik has spent an insame amout of time to make DI better. Maybe it's worth checking out his PR and see if the problem still exists : #2690

[lars-erik]

All temporal couplings like that should be gone, yes.

[zpqrtbnk]

Few things.

1. yes, the special user with ID 0 (zero) in version 7 becomes the special user with ID -1 in version 8. And it represents the "super" user. This is because, everywhere in our datalayer, we consider that an entity has an identity when its ID is not zero. For instance, when you instanciate a new Content object, its ID is zero - and when you save it, it becomes 1234. Having a _real_ user with ID zero was confusing.

2. fixing the DI is a priority but here... I think it's different. So, the dummy test controller has everything injected properly if it's part of the Umbraco solution. It does not have anything injected if it's in App_Code. Correct? Now what if it is compiled (not App_Code) but not part of the Umbraco solution (as in, third-party assembly)? Any chance you could test this? What I am suspecting is: we just don't register controllers that are in App_Code into DI.

3. the giant DI PR. it make things waaaay better, yes. work-in-progress to contain its scope and ensure it does not try to do too much - wanting to merge as soon as it makes sense - but again, prob not related to this

[rsoeteman]

Hi Stephan and others,

Thanks for getting back. My code was compiled, not in app_code.

And agree with you about database Id, but think you should add it to the list of breaking changes since all samples since 2008 came with the id=0 user.

[rsoeteman]

In case you want to test, I have my V8 branch just comitted https://github.com/rsoeteman/umbraco-admin-reset/tree/V8

[zpqrtbnk]

Thanks. Currently reviewing some code written in 2015 ;-) It may be that we only register controllers that live in Umbraco.Web at the moment, which would be pretty silly. Will update soon.

[lars-erik]

LightInject might do some magic, but I had to register mine. Can do autodiscovery in a composition tho.

[zpqrtbnk]

Update: it's nothing fancy temporal coupling or LightInject magic.

We don't rely on LightInject to scan all assemblies and find controllers, we take control of controllers registration. This is because we want to run our own assembly scanning (via TypeLoader). Assembly scanning (discovering types) is super expensive, and TypeLoader has some caching and tricks that help reduce that cost.

However, we have been a bit... aggressive... with our optimization. Currently we only scan Umbraco.Web for controllers, and nothing else. No controller outside of Umbraco.Web is auto-registered. Fast, but not very convenient.

Working on a fix. We should scan for Umbraco controllers (PluginController, UmbracoApiController...) in every assembly (your own, App_Code...). That can be fast. That would solve the original problem of this issue.

We will quite probably not scan for every controller, everywhere, though. That is expensive. So if you write your own pure IController implementation (not inheriting from Umbraco's base classes) you would have to register it.

Making sense?

[rsoeteman]

Hi Stephan,

I think it makes sense to scan for Assemblies that use Umbraco Controllers indeed. In other controllers you need to do the plumbing anyway.

Please let me know when you want me to test something.

Thanks!

[JimBobSquarePants]

We will quite probably not scan for every controller, everywhere, though. That is expensive. So if you write your own pure IController implementation (not inheriting from Umbraco's base classes) you would have to register it.

I would recommend making registration mandatory. Otherwise someone adds a large collection of binaries to the solution that are not on the ignore list and startup times go through the floor.

[kjac]

Possibly a bit late to the party here.

@zpqrtbnk you're right, the initial test was done with a controller in App_Code - which is dirty and whatnot, but also super handy for testing 😄 however - FWIW I just tested with a controller in its own assembly, and it doesn't work either.

using Umbraco.Web.WebApi;

namespace V8TestStuff
{
    public class TestStuffController : UmbracoApiController
    {
        public string GetStuff()
        {
            return $"UmbracoContext is {(UmbracoContext == null ? "not" : "")} set, Services is {(Services == null ? "not" : "")} set";
        }
    }
}

Invoking /umbraco/api/teststuff/getstuff returns UmbracoContext is not set, Services is not set.

[rsoeteman]

I would recommend making registration mandatory. Otherwise someone adds a large collection of binaries to the solution that are not on the ignore list and startup times go through the floor.

That makes sense for the Non Umbraco Controllers Stephan mentioned, but if you inherit from Umbracobase controllers you expect functionality is there. If you have an Umbraco controller in your solution it might be possible that you want to use Umbraco functionality out of the box.

[zpqrtbnk]

Want to have a look at https://github.com/umbraco/Umbraco-CMS/commits/temp8-controllers ?

[rsoeteman]

Yay I can access those services again. So works, thanks #h5yr

[kjac]

I can confirm that as well, given the above mentioned controller. And it also works for controllers App_Code 😁

[zpqrtbnk]

k, will look into merging at some point.

[rsoeteman]

Cool would be great then package devs can start thinking about merging packages to v8 #h5yr

[zpqrtbnk]

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

test: a random UmbracoApiController in a non-Umbraco assembly, or in App_Code, should still have services etc injected properly

[JimBobSquarePants]

That makes sense for the Non Umbraco Controllers Stephan mentioned, but if you inherit from Umbracobase controllers you expect functionality is there. If you have an Umbraco controller in your solution it might be possible that you want to use Umbraco functionality out of the box.

@rsoeteman Assembly scanning without constraint is a very bad idea. If you want any decent startup performance you should be explicitly declaring what assemblies to scan, not creating an ever-growing blacklist and scanning everything else in every possible location.

[lars-erik]

I think maybe a composition.RegisterContollersFromAssembly() might be in order.

[Shazwazza]

fwiw, we don't actually scan every assembly that is not black listed, we only scan assemblies that have references to assemblies that contain the type we are scanning for, this is similar to what aspnetcore does with it's scanning, though their's is even nicer optimized (so will steal that code eventually ;) We also don't re-scan, we scan once for IDiscoverable and then filter anything from there, all our plugins are IDiscoverable. We also don't actually scan at all on app restart if it's a soft restart (i.e. the /bin and some other things haven't changed) since we already know all types in the appdomain that we are looking for based on a cached file.

[rsoeteman]

Thanks @Shazwazza great to see optimizing is done and @JimBobSquarePants currently this works for Umbraco V7 and have always worked. This is the power of Umbraco that it is easy to extend. I think loading Content Cache is more of a problem then Assembly scanning. For plugin developers and people that implement a simple site some of this low level plumbing must exists.

[JimBobSquarePants]

Still means slow start up time and “magic” behaviour.

[warrenbuckley]

All good @zpqrtbnk merging this in :)

[lars-erik]

@JimBobSquarePants I'm sure auto-registering will be revisited. Ref. current probs with the ms.di adapter & scrutor. ;)