Improve (or remove) global error mapping

[jongeorge1]

As part of Menes initialisation, we provide the ability to globally map exceptions to specific response codes. Client libraries can write code like this:

config.Exceptions.Map<TenantNotFoundException>(404);

as part of the initialisation callback.

Whilst this is useful in reducing boilerplate exception handling code in operation methods, it is at heart a sprinkling of magic pixie dust that is not visible from the code that's doing the actual work, and that you have to know is there to understand the full set of potential behaviours of your code.

The corollary to this is that a developer that's unaware of the mapping could add code that invalidates the assumption made in the mapping - i.e. that the specified exception always maps to the given status code.

It would be good to revisit this and see if we can find a better balance between discoverability and the elimination of boilerplate exception handling in client code.

[jongeorge1]

The simplest approach would be to just use try...catch blocks in methods, but this potentially results in quite a lot of boilerplate - which was the whole reason this global exception mapping was implemented in the first place. In the use case which has the TenantNotFoundException above, literally every operation that makes up the service would need to handle the exception and return the 404. This would make it much more obvious that it was a possibility, but would clutter up the codebase.

One idea @idg10 and I came up with was to introduce a new attribute for operations, something like this:

        [OperationId(GetContentOperationId)]
        [OnException(typeof(TenantNotFoundException), 404)]
        public async Task<OpenApiResult> GetContent(IOpenApiContext context, string slug, string contentId, string ifNoneMatch)
        {
            IContentStore contentStore = await this.contentStoreFactory.GetContentStoreForTenantAsync(context.CurrentTenantId).ConfigureAwait(false);
            Content result = await contentStore.GetContentAsync(contentId, slug).ConfigureAwait(false);
...

This would give us a few nice things:

• We could drop the global exception handling without having to implement a load of boilerplate try...catch in each operation
• It's a lot more obvious that we are intending a particular mapping to apply to this specific operation. Rather than smearing code which affects the outcome across multiple locations, it's consolidated into one place.
• In cases where we only expect a subset of methods to potentially throw an exception, we don't have to worry about the possibility that adding a global mapping will obscure a really unexpected failure from one of the methods we never thought would throw that exception
• It opens up a new avenue for validating the OpenAPI document - i.e. if you add a mapping for a specific exception & status code to a certain operation, it would be reasonable to expect that the OpenAPI schema defines that status code in it's responses section, and to throw a configuration exception if it doesn't.

[jamesbroome]

I think this is a good approach, except I'd argue that we shouldn't drop the global exception handling approach at the same time. Sensible global conventions are useful - aiding productivity, and enabling the "pit of success" (eg. ASP.NET MVC routing).

Allowing global mapping conventions to be configured centrally, as well as individual cases and exceptions to be configured through attributes provides the best of both worlds.

So - the change in mindset would be to move away from global mappings as the default approach, and instead use attribute based mappings. But, in scenarios where global mappings make sense, then they can still be used.