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

Services.UserService returns null in UmbracoApiController #3593

Closed
rsoeteman opened this Issue Nov 12, 2018 · 28 comments

Comments

Projects
None yet
8 participants
@rsoeteman
Copy link
Contributor

rsoeteman commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Contributor

kjac commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Contributor

kjac commented Nov 12, 2018

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:

image

@JimBobSquarePants

This comment has been minimized.

Copy link
Contributor

JimBobSquarePants commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Member

dawoe commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Contributor

lars-erik commented Nov 12, 2018

All temporal couplings like that should be gone, yes.

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

zpqrtbnk commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 12, 2018

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

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

zpqrtbnk commented Nov 12, 2018

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

This comment has been minimized.

Copy link
Contributor

lars-erik commented Nov 12, 2018

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

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

zpqrtbnk commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor

JimBobSquarePants commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor

kjac commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor

zpqrtbnk commented Nov 13, 2018

@rsoeteman

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 13, 2018

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

@kjac

This comment has been minimized.

Copy link
Contributor

kjac commented Nov 13, 2018

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

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

zpqrtbnk commented Nov 13, 2018

k, will look into merging at some point.

@rsoeteman

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 13, 2018

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

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

zpqrtbnk commented Nov 13, 2018

PR #3621

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

@zpqrtbnk zpqrtbnk removed their assignment Nov 13, 2018

@JimBobSquarePants

This comment has been minimized.

Copy link
Contributor

JimBobSquarePants commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Contributor

lars-erik commented Nov 13, 2018

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

@Shazwazza

This comment has been minimized.

Copy link
Member

Shazwazza commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rsoeteman commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Contributor

JimBobSquarePants commented Nov 14, 2018

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

@warrenbuckley warrenbuckley self-assigned this Nov 27, 2018

@warrenbuckley

This comment has been minimized.

Copy link
Member

warrenbuckley commented Nov 27, 2018

All good @zpqrtbnk merging this in :)

@wafflebot wafflebot bot removed the state/review label Nov 27, 2018

@warrenbuckley warrenbuckley removed their assignment Nov 27, 2018

@lars-erik

This comment has been minimized.

Copy link
Contributor

lars-erik commented Nov 27, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.