Model building documentation does not show correct DI registration

[robertmclaws]

In several places throughout the documentation, there are examples of how to use protected override IServiceCollection ConfigureApi() to add model services. Each one of those examples shows the wrong order for registration. The docs typically show this:

        protected override IServiceCollection ConfigureApi(IServiceCollection services)
        {
            services.AddService<IModelBuilder, YOURMODELBUILDER>();
            return base.ConfigureApi(services)
        }

However, that will not register the service in the stack. If you try to debug a service registered this way, your breakpoints will not be hit. But the actual order is this:

        protected override IServiceCollection ConfigureApi(IServiceCollection services)
        {
            return base.ConfigureApi(services)
                .AddService<IModelBuilder, YOURMODELBUILDER>();
        }

If you could please assign this issue to me, I will make sure a complete explanation of how and why the DI functions this way, along with corrected examples, are added to the new docs.

This is related to Issue #432.

[chinadragon0515]

@advancedrei sure, I can assign the issue to you.

And one comments on the model builder, both example of customized model builder are correct depending on the needs.

Consider several examples,

1. User uses EF define the operations based on set from EF, and if user wants to log whole model, then he need register the customized model builder as last one of chain (second way in your example).
2. If user does not use EF, and define some operations based on his own entity set, he must build entity type and entity set first before operation model builder is called, and he need to register his own model builder as first one.
3. User uses EF and he has some entity set depends on EF set, also the operation will use his own entity set, then he will need more advance way to register which is described in section 4.3 (Expected this is rare case).

I just invite you as project member, after you accept, then I can assign issue to you.

[robertmclaws]

@chinadragon0515 Just so you know, the issue is not related to model builders at all. The issue is related to the DI framework in general. If you call services.AddService<ISomeService, CustomSomeService>() and then call base.ConfigureApi(services), whatever service you're trying to add does not get registered.

If you call base.ConfigureApi(services).AddService..., then it is fine.

Consider that the existing docs look like my first sample. When I pasted that into my API (and changed the types) it didn't work. When I looked at the TripPin sample and did it that way, it worked.

[rayao]

@advancedrei I think @chinadragon0515 's point is that for certain services it's necessary to register a custom instance before calling base.ConfigureApi. Of course it's not true for every service, and of course for certain services (those overridden in base.ConfigureApi) it's totally wrong.

[robertmclaws]

Oh, I gotcha. Sorry, I misread your explanation, @chinadragon0515 :).

In all the examples I've ever seen DI used in (and in all the ones I use myself), any service registrations across all custom components get called at the very beginning of application spin-up (often in a DependencyConfig.cs that gets called before WebApiConfig.cs).

In this case, overriding ConfigureApi in each class might not be the best option, because it's going to force the developer to understand your order of operations, and that shouldn't be necessary. It might be better to direct users to add all of their RESTier customizations before calling MapRestierRoute() instead. That way, the pipeline is already configured by the time you get to this point in the execution, and the internal order of operations won't matter.

If you'd like, I can put together a sample of what this would look like. :)

[chinadragon0515]

@advancedrei sure, we'd like to see your sample.
We also understand make user knows the sequence is not user friendly, we wants to allow user different options, and here is what we are targeted,

1. For most cases and most customers, we want user just use base.ConfigureApi(services).AddSingleton().
2. For some cases, we also allow user to register service before base.ConfigureApi, this is required for model builder case.
3. And we have request to customized model built from EF and operation model builder need this, so we need to allow user to add customized model builder between the two different services layer in RESTier (publisher and providers), so we have example in section 4.3.
4. We also have request to allow user disable conversion based service and he can define his own, so we separate conversion service register from other services.

Consider these requirements and also smooth for user move from basic usage (no his API class at all) to some customized (add just after base call) to most advanced customization (control all the services his wants to register), we have the design we have now.

If there is a better design, of course, we want to adopt and we welcome and appreciate contributions.

[chinadragon0515]

@advancedrei do you have chance to work on this issue or you want us to take over this issue? thanks

[robertmclaws]

I have not had a chance to look at it yet, I've been bogged down with troubleshooting why installing the .NET Core RTM on my machine breaks RESTier (whether the assemblies are installed in your app, or not). As soon as I can figure that out, I'll get back to this.

In the meantime, I have been diving into it more, and calling base.ConfigureApi(services).AddSingleton() is definitely not the right way to implement the .NET Core DI Container. Hopefully by next week I can put together something for all of us to take a look at.

Thanks for reminding me, I'll get to it ASAP. :)

[chinadragon0515]

@advancedrei OK, I see, thanks.

[robertmclaws]

I've taken some time to create a self-contained sample application using RESTier 0.6. You can find it here: https://github.com/robertmclaws/RESTier-DI-Sample. In the comments across the various files are my comments on why certain parts are incorrect, and how to fix them. If you have any questions, please let me know, and I'm happy to explain. Thanks!

[chinadragon0515]

@robertmclaws I take a quick look at the sample code, I see several comments that the sample service can not be complied. I double check the sample services, it works well.

The difference is sample services refer to RESTier-1.0.0-beta version, it has some changes on the Api part. Also RESTier 1.0.0 is based on Odata .net lib 7.x and Web Api OData 6.x, both of them has adopted DI.

Before ConfigureApi method is not static method, and in ApiBase, when we get ApiContext, it will set Api instance for DI. Also it depends on ODL 6.x and WAO 5.x, none of them has DI supported.

From my view, the logic here is updated, and it is more clean and simple now.

ConfigureApi is static method now, and this allows us does not need any Api instance before DI container is built, it also makes Api class as a completely DI service which means user can make any service referred by Api as DI service.

Can you help to update the sample you give to me and let us know whether these changes ease your concerns?

[robertmclaws]

Why are the currently-shipping samples, that you are referring to in your documentation to cover for the user's inability to understand the docs, linking to code that is not shipping yet? Why isn't 1.0 work for the samples being done on another branch? Come on guys, this is basic stuff.

Regardless, I copied the code from https://github.com/OData/ODataSamples/blob/master/RESTier/SpatialSample/Spatial/Api/SpatialApi.cs for the UnusedSampleController in my sample project, so if that is coming from the RESTier 1.0 beta, you are still doing DI wrong.

[chinadragon0515]

1.0.0-beta was released on Sep. 5, refer to nuget to download https://www.nuget.org/packages/Microsoft.Restier/1.0.0-beta

And for sample services, it is always upgraded to newest version to help user migrate to newest status.

For the DI part, I see in your code, you expect end user to create a container, add service, then pass the resolver to the service.

We will discuss this, but from my view,

1. It requires too lots of code from end user, also user need to pick container for the code.
2. From backend code, it needs to know which resolver it is, and uses that resolver to get the service. It is not possible, and only a generic class can be referred by restier code.
3. In our code, we only expose one static method for user to override, user can add any services he wants. He does not to care about detail of the container.

[robertmclaws]

My apologies on the 1.0 code, it was not available on NuGet when I built the sample.

Just to be clear, I do not expect them to manage the container. The Autofac code is in there to show you what people using existing DI techniques already have to do, and how they expect to use it.

I showed code in the sample of how to do it with the Microsoft DI framework (which also allows you to plug in your own container framework if you desire). Check Startup.cs.

Static method overrides are not dependency injection. You are inventing something else entirely. If that is the direction you're going to go with, then DI is totally not necessary, and you should not be introducing that level of complexity. I point all of this out because you're not even using the DI container the way ASP.NET Core uses it... So you are going to cause all kinds of confusion moving forward. I'm just trying to save you from the headaches now.

[rayao]

        protected override IServiceCollection ConfigureApi(IServiceCollection services)
        {
            services.AddService<IModelBuilder, YOURMODELBUILDER>();
            return base.ConfigureApi(services)
        }

This works because many Restier services (including IModelBuilder) is chained. They're chained so that consuming side could optionally override or partially override those services. As you can see in EF ModelProducer class there's InnerModelBuilder, which will be YOURMODELBUILDER in this case.

[robertmclaws]

This is not how you are supposed to use Dependency Injection. I understand that the code functions. The way you are doing it is counter to the way everyone else does it. I don't know how else to explain it to you guys, and you guys can explain it all you want... It's still wrong. You register services at the very beginning... Not inside the class where you are doing the work.

[rayao]

Months ago I proposed a design, I think maybe that's what you want, in that design service registration is (at least could possibly be) outside of API classes. That design if rejected, I think the main reason is it's easier to understand if everything is put together, especially for those who aren't very familiar with DI.

[robertmclaws]

I know RESTier is supposed to be simple, but in trying to make the API easy to implement, you're creating an anti-pattern that caters to the lowest common denominator. In trying to reduce friction for DI newbies, you're increasing friction for people that know what they're doing. You should not be teaching people who don't know DI how to use it improperly. It's much better to do it right and have people grow; supporting that growth with clean, simple instructions on how to set it up.

Registering a custom ModelBuilder, for example, should be as simple as it is here: https://github.com/robertmclaws/RESTier-DI-Sample/blob/master/RESTier%20DI%20Sample/Startup.cs. Just add your own ModelBuilder instance before you tell RESTier to register the rest of its services. The EntityFrameworkApi.RegisterDependencies function would cycle through the existing registrations, add anything that is needed but missing, with all the "magic" you're currently looking for.

What's happening now is that you're taking Constructor Injection and implementing it the way someone who doesn't understand DI would use it. It's only going to confuse people. I promise the way I'm suggesting is far simpler, and it also happens to be architecturally correct.

But you don't have to take just my word for it. I think you should take the design to the ASP.NET Core team and have them review it. Because I think they will agree with me that the design should be in keeping with what the rest of Microsoft is doing with DI, especially if 1.1 will need to have ASP.NET Core support.

[chinadragon0515]

@robertmclaws

thanks for all great comments.

For restier, we want user to create service in a simplest way, minimum code effort.
Also we need to consider the ASP .NET we are using. (Note RESTier does not support ASP .NET core yet, and we do not have a solid plan for this yet as its dependencies OData .Net lib and Web Api OData has no solid plan yet).

In order to use DI,

1. We must have a container, either user set a container or RESTier create a container as ASP .Net we are using dose not have a container, we choose to create a container to simplify user effort.
2. We must registered RESTier services, and there are two cases,
   a). User does not need to extend API class for customization, then we want to be codeless for these users, in order word, we want to auto register RESTier services for end user.
   b). User wants to extends APi, he does not need to do anything.
3. We need to allow user to customize the services,
   Now user can extends APi, implement ConfigureApi method and call base class ConfigureApi to have RESTier services registered.
4. The service registered must be called during application start. We call during map route.

As ASP .Net does not have method like ConfigureService, also there is no default container, we can not do in the way you suggest. When we support ASP. Net core, we will consider the way you suggest.

If you suggest something like DependencyConfig in your sample, from my view, it is too complex and we also request user provides the container, need to work out how to get service from user's container, how to manage the service's scope and so on.

If you have any suggestion for non ASP .NET core implement (which current RESTier based on), help to let us know, thanks.

[robertmclaws]

There are so many flaws with this logic that I don't even know where to start.

Just because you're not on ASP.NET Core yet doesn't mean you code yourself into a corner. You took a dependency on the DI framework that ASP.NET Core uses (which you probably should not have done yet), and you're not using it properly.

Seriously, I don't understand why you guys keep explaining to me how DI works. You're ALREADY using DI in RESTier. I'm talking about how to register things WITH YOUR OWN STUFF, not tying into another DI container. I simply showed the Autofac stuff to show you how DI registration is supposed to work, using an outside framework example.

Look, It's already annoying enough that, if I'm already using a DI container in my app, that you're making me use two different DI containers. But, I imagine you didn't think about that use case either.

I feel like you're too deep into the architecture to get how someone who is just getting started with your platform would use it, and something is getting lost in translation when I try to explain it.

I updated the sample to show you how it should work, with no other DI containers involved except the one you are already using. Look at Startup.cs. Then check the UnusedSampleController to see my comments on all the code that is now unnecessary, if you do it right.

[chinadragon0515]

Do you have time to have a call? We can discuss in the call to understand each other better, then see what we can improve.
I will schedule a call first, if you do not have time to make the call, I will cancel it, and if you want a new time slot, help to let me know.

[jasoncouture]

I really have to agree that this is an absolutely disastrous implementation of dependency injection.
I have a simple requirement, that is turning out to be quite a headache to implement. I'm thinking I was better off just using ODataConventionModelBuilder instead.

Issues:

1: Cannot replace IContainerBuilderFactory, must reimplement MapRestierRoute(...), and many of the dependencies of this call require internal class access, so those also have to be reimplemented.
2: Restier overwrites any pre-registered items in DI, it should be calling HasService and registering only if that returns false.
3: Static methods, as stated before. This is not DI, this is madness.

Static method overrides are not dependency injection. You are inventing something else entirely. If that is the direction you're going to go with, then DI is totally not necessary, and you should not be introducing that level of complexity. I point all of this out because you're not even using the DI container the way ASP.NET Core uses it... So you are going to cause all kinds of confusion moving forward. I'm just trying to save you from the headaches now.

[0xced]

@robertmclaws Do you plan to address this "dependency injection" madness?

[jspuij]

Let me chime in here. I tried replacing the DI container with Autofac, and eventually succeeded, but it took me a week and a lot of reflection to call inner api's and work around quirks. This must be the worst implementation of DI in a library (which is debatable in the first place, libraries should not require DI, let alone a specific container, just be DI friendly). I realize that some of the issues raised are actually in the OData libraries itself, not in Restier, but I thought splitting this in a few comments across all repositories does not make it any clearer.

Some observations, not necessarily a comprehensive list, just to illustrate my point:

• The code is littered with the service locator anti-pattern. This makes the code impossible to run, even when it compiles after you've implemented the necessary interfaces.
• Container creation, registration of services, building of the container should be done by the application developer. It's all about inversion of control. Restier ties you to an implementation, creates the container itself and registration happens behind the scenes, deep down in it's own extension methods. Even the "Building" (building as in locking for further registrations, and start of the resolve phase) of the container is taken away from the application developer and tied to the mapping of the OData route.
• The (odata) documentation mentions the same container used by Restier and Odata. In the end this is true, but deep down Restier creates it's own container builder per mapped restier api route, which is not apparent from either code or documentation. Good luck thinking that you replaced the default OData ContainerBuilder with your own, because Restier will register it's own and replace your's again.
• The poor choice of DI abstraction in asp.net core has been described at length by better men than me so I won't add to that here, but by not updating your dependency on Microsoft.Extensions.DependencyInjection you are almost committing a crime. Not only am I forced to conform to the lowest common denominator conceived by The Wise Men from MS, but by not updating the dependency I'm now forced to use version 1.0.0, and it won't be long before some other dependency requires a later version and I am royally screwed.
• Because the code was never properly tested with another DI container, it depends on all kinds of intricate implementation details that are hidden behind the IServiceProvider interface:
  • DI implementations currently have to return null when a type is not registered, not throw an exception. More importantly if the object registered depends on another object that is not registered, creation should proceed of the object with the dependency null as well.
  • DI implementations currently should not check for lifetime scope issues in the hierarchy. (This is actually a mess, the number of singleton registrations that take dependencies on scoped or transient objects is enormous.)
  • DI implementations currently should not revisit the factory delegate for a singleton object but return a cached object instead. (Or you end up with really nasty stack overflows because of recursive dependencies because again the use of the service locator anti-pattern. Proper constructor injection and correct scoping would have been able to catch this kind of madness at container building / verification).
  • DI implementations currently should be able to return a list of implementations by order of registration because of the chaining madness taking place in yet another extension method. The number of ways the registrations of IModelBuilder and subsequent chaining and resolving of instances can lead to either recursive loops or exceptions is astonishing.
  • DI implementations are required to allow re-registration of a service type by another implementation without warning and should always return the latest registration, unless enumerated.
• OData insists on creating it's own request scopes directly from the root container by requesting an IServiceScopeFactory implementation (yep, good luck trying to find that and it's corresponding registration that you will want to override). But what if you've already setup a nice request scope with your necessary registrations (e.g. inside some Owin middleware)? You have to resort to a thread safe / async aware singleton that keeps track of all request scopes outside OData and retrieve them inside the newly created OData scope.

And this is just off the top of my head, I've probably forgotten a few problems I encountered last week. Long story short: I will revert the code that I've written. Nice that I was able to pull it off, but this will be a future source of problems and maintenance hell that I cannot pass on to other members of my team.
I'm left with putting my own scoped container into the Restier container and retrieving it from the IServiceProvider instance of my ApiBase derived Api class. Yes, my own service locator anti-pattern on top of yours, but it probably has the greatest chance of actually working in the future.

Now that the rant's over, I'm more than willing to invest some time to sort some of these issues out and creating a few PRs that will help moving forward, but only if any of it is truly appreciated. I've been reading issues 2 to 3 years old with signals that Restier will be picked up by the Odata team members and will get the love it so desperately needs. It would be a waste of my time when next week a PR will be merged that addresses all these issues, updates Restier to the latest Odata and DI, and supports .NET and EF core.

@mikepizzo sorry for the rant, but is there any interest in some progress?
@robertmclaws I've seen that you've been vocal about some of these things and eventually forked Restier. Is the fork actively maintained and worth looking into for me?

[robertmclaws]

@jspuij I forked it only to fix a couple issues that were preventing my code from working. Honestly, I have so many branches in my fork to try to submit PRs that it's been hard to keep track. My hope is that @robward-ms and team will be able to get to Restier in a few weeks, and hopefully we can iron out this, as well as A NUMBER of other architectural issues with the platform. I'm now using Restier extensively at a US state government org that I contract for, and I'm personally invested in getting the thing to 1.0 quality.

HTH!

[jspuij]

@robertmclaws Thanks for the response and your efforts so far. We only started using Restier recently, I suspected from the issues that it wasn't the only thing we'd encounter. The thing I'm working on is a large project as well though, and writing a few hundred OData controllers ourselves or generating them does not seem like an attractive option either. I'll see what I can do.

[robertmclaws]

So, there are a couple things that will make your life easier.

1. You can modify the EF6 code generation T4 templates to generate Restier Interceptors. That takes a crap-ton of work out right there.
2. You can use my Breakdance library (https://github.com/CloudNimble/Breakdance) to run Restier tests that show you, based on your model, which Interceptors are expected, and which ones were found. If you find an Interceptor isn't being hit, it's your best way to find out if Restier even saw it.

HTH!

[robertmclaws]

We will resolve these issues in v2, and the pattern to fix registration is linked in #643. @jspuij When we go to rewrite RESTier for v2 running on .NET Core 3.0, I would love your help getting DI right this time.

Thanks all!

[jspuij]

Willing to help. There is a serious amount of DI madness in OData already though. What's the goal? To at least not make it any worse or to completely remove the dependency on Microsoft.Extensions.DependencyInjection and to have every class correctly announce it's dependencies, i.e. no Service Locator Anitpattern.

To do it right, Odata.net needs to be fixed as well.