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

RegisterHttpRequestMessage requires a ContainerBuilder but doesn't use it #22

Closed
bobmeyers5 opened this issue Jan 28, 2017 · 4 comments
Closed

Comments

@bobmeyers5
Copy link

All this method really needs is an HttpConfiguration object, and as such it should really be an extension method on HttpConfiguration, similar to MapHttpAttributeRoutes or other built-in extension methods, not an extension method on ContainerBuilder, which it doesn't use.

Also, because it doesn't use ContainerBuilder, there's no meaningful guidance on what container builder to call it on. I'm currently calling it on a throwaway instance just to avoid the argument null exception.

@bobmeyers5
Copy link
Author

FYI, the reason I'm not using the primary ContainerBuilder at the application root is because that builder has already been built by the time I'm configuring WebAPI, so it would be odd/misleading to use it at that point.

@alexmg
Copy link
Member

alexmg commented Jan 29, 2017

The RegisterHttpRequestMessage extension method will have been applied to ContainerBuilder for discoverability, as this is almost always the starting point for performing container related configuration. I admit that it would be a bit surprising to find that the builder isn't actually required.

It's definitely too late to just move the extension method but there are a couple of options:

  • Add an extension method to HttpConfiguration and redirect calls to the existing extension method to the new one.

  • Make the DelegatingHandler implementation public so that it can be added manually. It's currently named CurrentRequestHandler and would need to be given a better name before being made public.

  • Leave things how they are and except that for this particular case a work around is required.

I'm leaning towards the first option. Naming wise I don't think RegisterHttpRequestMessage makes sense in the context of HttpConfiguration. Perhaps something like UpdateScopeWithHttpRequestMessage would be better (I got this from the name of the private method in the DelegatingHandler that does the actual update).

@tillig
Copy link
Member

tillig commented Jan 29, 2017

The first option is what I was thinking as well.

@dmorganb
Copy link
Contributor

The ContainerBuilder isn't required because the CurrentRequestHandler (the DelegatingHandler that does the update) gets the current LifetimeScope from the HttpRequestMessage (using the GetDependencyScope and GetRequestLifetimeScope extension methods) and then does the update against the ComponentRegistry of the LifetimeScope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants