Refactor content negotiation code into a service #6998
Conversation
|
YES! 😍 |
|
|
||
| namespace Microsoft.AspNetCore.Mvc.Infrastructure | ||
| { | ||
| public abstract class OutputFormatterSelector |
There was a problem hiding this comment.
What's the benefit of an abstract class instead of an interface?
There was a problem hiding this comment.
It has a better set of tradeoffs for something like this (IMO).
With an abstract class we can easily add new functionality or overloads without breaking anything. When something is implementation-ey, the requirements frequently change. We'd do something like add a new API, obsolete the old one, and then remove it in the next major release. You can't do that with an interface, you have to make a new interface.
Interfaces are better when the contract is minimal, the requirements seldom change, or when we expect lots of implementations.
I'm open to feedback if you think this is the wrong direction
There was a problem hiding this comment.
I don't have a clear preference in this case. An abstract class with only a single abstract method seems a bit strange, but I understand that the class is better than the interface for future changeability.
Interfaces are better when the contract is minimal, the requirements seldom change
I kind of expect a content negotiation interface to be minimal and rarely changing. Ideally, we could have something like this:
public interface IContentNegotiator
{
IOutputFormatter SelectFormatter(IEnumerable<IOutputFormatter> formatters, MediaTypeCollection mediaTypes);
}
But then the selection logic would not take into account if the result object can be serialized by the selected output formatter and I guess that's important too. :-)
BTW I got to this PR, because I wanted to do content negotiation in an exception handling middleware. Currently the easiest way of doing it is calling the SelectFormatter by reflection. This PR would allow me to do that without any reflection hack, so as far as I'm concerned, this is perfect.
| private readonly bool _respectBrowserAcceptHeader; | ||
| private readonly bool _returnHttpNotAcceptable; | ||
|
|
||
| public DefaultOutputFormatterSelector(ILoggerFactory loggerFactory, IOptions<MvcOptions> options) |
There was a problem hiding this comment.
LoggerFactory after Options (to keep it consistent with most of our other API)
| formatters = _formatters; | ||
|
|
||
| // Complain about MvcOptions.OutputFormatters only if the result has an empty Formatters. | ||
| Debug.Assert(formatters != null, "MvcOptions.OutputFormatters cannot be null."); |
There was a problem hiding this comment.
this won't be null (since you initialize this)
| } | ||
| } | ||
|
|
||
| result.Sort((left, right) => left.Quality > right.Quality ? -1 : (left.Quality == right.Quality ? 0 : 1)); |
There was a problem hiding this comment.
Unrelated: should we cache this delegate?
There was a problem hiding this comment.
Sure - it will be cached already since it doesn't capture, but making it explicitly cached makes it harder to break
| /// <param name="writerFactory">The <see cref="IHttpResponseStreamWriterFactory"/>.</param> | ||
| /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param> | ||
| public ObjectResultExecutor( | ||
| IOptions<MvcOptions> options, | ||
| OutputFormatterSelector formatterSelector, |
There was a problem hiding this comment.
What's our level of concern for breaking change here? Particularly since this is in the .Infrastructure namespace?
There was a problem hiding this comment.
This type was pubternal in 2.0 so there's no breaking change
|
|
||
| namespace Microsoft.AspNetCore.Mvc.Infrastructure | ||
| { | ||
| public abstract class OutputFormatterSelector |
There was a problem hiding this comment.
Need some docs for this + the method
This is a refactor in anticipation of using this logic in some other places
rynowak
force-pushed
the
rynowak/formatter-service
branch
from
2895f44 to
1e82aab
Compare
rynowak
force-pushed
the
rynowak/formatter-service
branch
from
97177d7 to
1c63743
Compare
This is a refactor in anticipation of using this logic in some other
places