Aspnetcore.docs: ExceptionFilterAttribute with parameterised constructor?

[ggeurts]

Attributes and dependency injection don't go together, so the ExceptionFilterAttribute sample class is rather confusing. It either should not be an attribute or it shouldn't rely on dependency injection.

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: a70a955f-dd5a-c46b-dfe8-21521e619320
• Version Independent ID: ebb299e0-6c1c-7077-689d-ac711352e418
• Content: Filters in ASP.NET Core
• Content Source: aspnetcore/mvc/controllers/filters.md
• Product: aspnet-core
• GitHub Login: @ardalis
• Microsoft Alias: riande

[Rick-Anderson]

@dougbu please review

[dougbu]

@ggeurts how does ExceptionFilterAttribute "rely on dependency injection"? It (see source) is an abstract class for those who wish to extend MVC with their own exception filters and it has no dependencies on any service.

On the other hand, subclasses may use context.HttpContext.RequestServices to get services from DI in the method they choose to override.

[ggeurts]

The comment applies to the exception filter sample code that defines a CustomExceptionFilterAttribute class. This class should either not have the Attribute postfix or not have a parameterised constructor. Even better, both scenarios (filter attribute without DI and non-attribute filter with DI) should be shown and evaluated.

[dougbu]

sample code that defines a CustomExceptionFilterAttribute class.

Ah, that's fine too. It's used in a [TypeFilter] and that will instantiate the filter using DI for any constructor arguments.

[Rick-Anderson]

@ggeurts Thanks for contacting us. We believe that the question you've raised have been answered. If you still feel a need to continue the discussion, feel free to reopen it and add your comments.

[ggeurts]

@dougbu I am not contesting that this code will work. My comment is that the sample code is confusing, which I presume is something to avoid as much as possible in the official API documentation.

The common expectation is that classes with Attribute postfix can be used as custom attributes. This is not the case when they rely on DI in the constructor. So the sample class should either not have the Attribute postfix, or it should not require DI in the constructor. Ideally, the documentation should explain that filters can either be defined as custom attributes (so filter class has Attribute postfix and does not use DI in constructor) or as a regular .NET class (filter class has no Attribute postfix and can use DI in constructor) that is registered during the MVC configuration phase.

[dougbu]

In this case, the class is an Attribute primarily because it subclasses ExceptionFilterAttribute. It is implemented that way to reuse what's in ExceptionFilterAttribute and not because the Attribute part is important.

At _most_, the docs referencing the sample could say CustomExceptionFilterAttribute can't work when used as an attribute due to its constructor parameters or perhaps the class could be renamed. Neither seems particularly important because it'll be obvious to anyone using the samples that [CustomExceptionFilter(...)] doesn't work.

[ggeurts]

I have been writing C# code since version 1.0 and this was pretty confusing to me. Class naming is an important aspect in understanding code with as little mental load as possible. If xxxAttribute classes cannot be used as attributes, they shouldn't be called that way. And because the docs may set naming patterns followed by people reading them, I think the naming patterns used need to be as clear as possible.

[Rick-Anderson]

Lets keep this issued closed for now. The question/answer is very discoverable from the doc page by this issue. Should your request generate enough 👍 responses, we’ll reconsider updating the doc.

[BrianJonesCNM]

I am trying to use the sample as it is and I can not apply the attribute because it is asking me for the DI objects in a contractor which are not available at design time.

So I have not been able to use this code in a ASpNetCore 2.2 web mvc (not api)

CS7036 There is no argument given that corresponds to the required formal parameter 'hostingEnvironment' of 'CustomExceptionFilterAttribute.CustomExceptionFilterAttribute(IHostingEnvironment)'

[peterwenstrom]

@BrianJonesCNM
You can't use the sample as an attribute, and I think that's what @ggeurts is getting at. It's weird to have an example of a class with the Attribute-suffix, but the fact that it relies on DI prohibits it from being used as an attribute.

[Rick-Anderson]

I'll add the following

the class is an Attribute primarily because it subclasses ExceptionFilterAttribute. It is implemented that way to reuse what's in ExceptionFilterAttribute and not because the Attribute part is important.

At _most_, the docs referencing the sample could say CustomExceptionFilterAttribute can't work when used as an attribute due to its constructor parameters or perhaps the class could be renamed. Neither seems particularly important because it'll be obvious to anyone using the samples that [CustomExceptionFilter(...)] doesn't work.

[Rick-Anderson]

@serpent5 here's an easy one if you're interested.

[serpent5]

This appears to be along the same lines as the concern raised in https://github.com/aspnet/AspNetCore.Docs/issues/10292. The outcome there was changing from an attribute to a direct interface implementation. Could the example in question here follow suit and implement IExceptionFilter directly? That would remove any confusion and save the extra explanation being needed.

[Rick-Anderson]

Could the example in question here follow suit and implementIExceptionFilter directly? That would remove any confusion and save the extra explanation being needed.

That's more work for @dougbu to review the change. If @dougbu agrees, OK; otherwise just add explanation.

[dougbu]

I'm on vacation and won't look at this 'til I get back or perhaps later (week of the 4th). @mkArtakMSFT could you find someone to help out here?

[mkArtakMSFT]

I've just approved the PR. The changes look good to me. Thanks for driving clarity in our docs!