BUG: ODataInputFormatter and ODataOutputFormatter Does Not Override Expected Behavior

[commonsensesoftware]

NOTE: This issue has been updated from the original submission to reflect that both the input and output formatters are affected by the bug

The ODataInputFormatter and ODataOutputFormatter extend the built-in InputFormatter and OutputFormatter respectively, which each implement the IApiResponseTypeMetadataProvider interface used by the API Explorer. The ODataInputFormatter and ODataOutputFormatter break the expected design by allowing a configuration that does not have any supported media types. The OData implementation should be overriding the default behavior so that the base implementation from InputFormatter and OutputFormatter no longer produces exceptions.

This issue was almost certainly never previously detected because OData does not provide any API Explorer extensions.

Assemblies affected

Microsoft.AspNetCore.OData 7.0.1+

Reproduce steps

Call ODataInputFormatter.GetSupportedContentTypes or ODataOutputFormatter.GetSupportedContentTypes when ODataInputFormatter.SupportedMediaTypes or ODataOutputFormatter.SupportedMediaTypes is empty.

Expected result

No exception. The result should be null or an empty list.

Actual result

InvalidOperationException is thrown.

Additional detail

This happens because the ODataInputFormatterFactory creates an ODataInputFormatter and ODataOutputFormatterFactory creates an ODataOutputFormatter which each have no supported media types. This currently only appears to be for the configured formatter to handle raw values. This is the intended design as the media type is not known upfront and can only be detected from the incoming request.

The issue is that the default behavior from the inherited InputFormatter and OutputFormatter base classes require that there be at least one supported media type. Since OData is changing the expected behavior, it should override the GetSupportedContentTypes method in its own formatters.

The suggested implementation for each formatter is:

/// <inheritdoc />
public override IReadOnlyList<string> GetSupportedContentTypes(string contentType, Type objectType)
{
    if (SupportedMediaTypes.Count == 0)
    {
        // note: this is parity with the base implementation when there are no matches
        return default;
    }

    return base.GetSupportedContentTypes(contentType, objectType);
}

Impact

Without this change, OData formatters break certain APIs such as the API Explorer. Callers of the GetSupportedContentTypes expect a null or empty result, but not an exception.

Workarounds

• Remove ODataInputFormatter from MvcOptions.InputFormatters and ODataOutputFormatter from MvcOptions.OutputFormatters when they do not have any supported media types
  • This will result in the loss of media type negotiation for raw values
• Replace ODataInputFormatter in MvcOptions.InputFormatters and ODataOutputFormatter in MvcOptions.OutputFormatters when they have not supported media types with a subclass that copies all the existing values and overrides GetSupportedContentTypes correctly

[julealgon]

Nice discovery there.

I'd just like to suggest returning an empty read-only collection instead of null, as handling collections as null is usually a bad practice and can cause NullRefrrenceException on consumers.

In this particular case there is no special semantic difference between null and empty (I understand there may be in other contexts though), so it's better to just return empty IMHO.

[commonsensesoftware]

@julealgon, totally agree with you. I prefer avoiding null as well; however, if you take a peek at the implementation in OutputFormatter you'll see that it also returns null. I know ASP.NET Core team often does this to avoid allocations. Apparently, they feel the juice is worth the squeeze. Ultimately, I'd be satisfied with a null or empty list. A caller, however, always has to consider null with this API as it's likely to return null from one of the base implementations.

#no-nulls #billion-dollar-mistake

[grahamehorner]

is there any update with regards this issue? is this on the roadmap/scheduled to be fixed; we have ran into this issue when deploying non-odata and odata controllers in the same host and would like to understand if/when this would be fixed.

[grahamehorner]

@commonsensesoftware I'm not seeing the ODataInputFormatter or ODataOutputFormatter within the MvcOptions ? any additional pointers ?

[cecilphillip]

@xuzhg I think this is really important and seems like a simple fix

[Zzarek]

Is this issue dead?

[gathogojr]

Fixed by #2494