HttpRequestExtensions in Menes.Hosting.AspNetCore is in the wrong namespace
idg10 opened this issue · 6 comments
The Menes.Hosting.AspNetCore component defines an HttpRequestExtensions class that defines the HandleRequestAsync extension method for IOpenApiHost<HttpRequest, IActionResult> (which is typically the entry point to Menes for Azure Functions and ASP.NET Core apps). Inexplicably, this type is defined in the Microsoft.Extensions.DependencyInjection namespace.
This is weird in its own right, and it's also inconsistent with the location of the source file
https://github.com/menes-dotnet/Menes/blob/master/Solutions/Menes.Hosting.AspNetCore/Menes/HttpRequestExtensions.cs which is in the Menes folder. So it's slightly odd that we're not seeing any code analysis complaints about this.
Expected behavior
The type should be in either the Menes namespace, or some nested namespace underneath Menes.
The 'obvious' fix is to make the namespace consistent with the folder, i.e. move it into Menes. But I'm wondering if both HttpRequestExtensions and its sibling SimpleOpenApiContext are in the wrong place.
In particular, there's nothing at all about the name of SimpleOpenApiContext that would clue you into the fact that this is an ASP.NET-Core-specific type. Arguably it's in the wrong library completely—possibly it should be in Menes.Abstractions. I initially wondered if the one justification for it being in here is that it defines a CurrentPrincipal property as a ClaimsPrincipal, and while that's not necessarily unique to ASP.NET Core, it's a thing ASP.NET Core does. But then I realised that IOpenApiContext requires all contexts to do that. So I'm now not sure that the context class belongs in this project at all. I can't see anything specific either to hosting or to ASP.NET Core.
So that leaves the HttpRequestExtensions class. It's definitely concerned with hosting, so arguably it belongs in Menes.Hosting. However, the same could be said of IOpenApiHost, and that lives in the top-level Menes namespace. So either that's wrong and needs to be moved, or HttpRequestExtensions belongs in Menes.
There's some oddities in this whole OpenApiContext.
It is, of itself, not supposed to contain anything that can't be represented generically. So, for example, I don't believe that IOpenApiContext should expose that ClaimsPrincipal. I think that there should be an IOpenApiClaimsContext or similar, which exposes that property, and anything that expects the context to support it can cast it up (and handle the case where it doesn't offer the property).
Regardless, the SimpleOpenApiContext then should not belong in here at all. It is clearly a hangover from putting it "somewhere" and the usual desire not to create another assembly dependency without much content. But it could just as easily (probably better) live in Abstractions.
Thinking about it - this may be a legacy of when the CurrentTenantId was not a string and it was CurrentTenant - an actualized instance.
But I am unsure that the context should have even the CurrentTenantId in it anyway - it is fundamentally a part of the API and comes through that route, which allows building links etc in an API-specific way; it is intrinsically part of the API description, not some "orthogonal" thing, which is how it was originally conceived.
Which leads me to wonder what the purpose of the Context is at all.
I had conceived of it originally as a place to stash e.g. the original HttpRequest - something very specifically "hosting context" related, rather than OpenAPI framework related.
The one thing Menes itself actually relies on the context for is to be the ability to populate audit records. (Specifically, it records a UserId based on the CurrentPrincipal, and it also stores the tenant ID.) Maybe we should find a way to separate that out, because embodying this cryptically in the context is a little obscure. And arguably, only applications can know what should and shouldn't be stored persistently for audit purposes
In theory, the context offers one distinctive feature for clients of Menes: an extensibility point through the notion IOpenApiContextBuilderComponent<TRequest>. It provides a way you can plug in something that gets to look at the request early on, and then attach stuff to this context object that then gets flowed through the rest of processing. While this sounds plausibly like the sort of thing that might be useful, nothing appears actually to be using this particular mechanism in our current code for anything other than tenancy which, as you say, needs to be modelled differently anyway.
The one other thing in existing code that consumes the pre-Menes version of all this seems to use is the CurrentPrincipal. In fact that's pretty heavily used. (I'm fairly sure nothing relying on Menes is using it yet because as far as I can tell there's no code in Menes that ever sets the CurrentPrincipal...)
So this has me wondering if the right thing to do is basically remove everything from the IOpenApiContext and make it more like a dictionary—a bag of stuff (so as to be able to represent everything generically, as you say). Something like the Request Feature collection in ASP.NET Core.
ASP.NET Core implements HttpContext.User in terms of features. It more or less boils down to code like this:
User => _features.Get<IHttpAuthenticationFeature>()(It's more complex in reality because of caching, but that's the heart of the idea.)
So in a Menes-based service you might have something like this:
public OpenApiResult SomeOp(IOpenApiContext context, string arg)
{
IOpenApiClaimsContext claimsContext = context.Get<IOpenApiClaimsContext>();
ClaimsPrincipal user = claimsContext.User;
...You could even envisage something like this:
public OpenApiResult SomeOp(
[FromOpenApiContext] IOpenApiClaimsContext claimsContext, string arg)
{
ClaimsPrincipal user = claimsContext.User;
...so that we have a declarative way for an operation to indicate what it's expecting from the context.
But that could come later.
So to enable access to a ClaimsPrincipal (the main thing users of Menes' predecessor wanted from the context) we could provide an IOpenApiContextBuilderComponent<HttpRequest> as part of the ASP.NET Core hosting that you can optionally enable (or I guess we could enable it by default, and make it opt-out since this is apparently what most things want from the context) that adds the IOpenApiClaimsContext (or whatever we call it) to the context.
That would validate that the context was able to support the main thing most code in the past has wanted from it, but based on a generic mechanism.
If we did this, I'm not sure there's actually very much point in making the concrete type of the context a type parameter like it is today. If the interesting things are the services in it, its type is of no real relevance, and should just be an implementation detail of Menes.Abstractions.