Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpContext and JSON #17160

Closed
rynowak opened this issue Nov 15, 2019 · 48 comments · Fixed by #21731
Closed

HttpContext and JSON #17160

rynowak opened this issue Nov 15, 2019 · 48 comments · Fixed by #21731
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@rynowak
Copy link
Member

rynowak commented Nov 15, 2019

HttpContext and JSON

This is a proposal to add extension methods for using HttpContext (and related types) with System.Text.Json. Adding this functionality will make ASP.NET Core developers productive with smaller amounts of code. This is aligned with our goal of making route-to-code a useful programming pattern.

endpoints.MapPost("/weather", async context =>
{
    var weather = await context.Request.ReadFromJsonAsync<WeatherForecast>();
    await UpdateDatabase(weather);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

Goals

  • Make this the easiest imperative way to read/write JSON

  • Make this fastest way to read/write JSON

  • Do HTTP things correct by default (don't cut corners)

  • Replace the guts of MVC's JSON formatter with this (validates flexibility)

  • Do all of this without requiring service registration/configuration

  • NON-GOAL: Create an abstraction for JSON serializers/settings

API Additions

These are the proposed APIs, see the following (annotated) sections for discussion.

Some updates based on recent discussions with @JamesNK and @Tratcher:

  • These would be added to the Microsoft.AspNetCore.Http.Extensions assembly.
  • JsonOptions would be inherited (used as the base) by MVC and SignalR - but they are copies. So modifying JsonOptions would influence MVC and SignalR - but not the reverse. This is worth a discussion.
  • We got rid of APIs that allow specifying an Encoding for writing. Can bring these back if needed.
  • Overloads that specify a contentType should specify the value they want to see in the Content-Type header. We will not append a charset. We will not parse the value you give us and change the encoding based on charset. It's your job to get it right.
namespace Microsoft.AspNetCore.Http
{
    public static class HttpRequestJsonExtensions
    {
        public static bool HasJsonContentType(this HttpRequest request);
    }
}

namespace Microsoft.AspNetCore.Http.Json
{
    public static class HttpContextJsonExtensions
    {
        public static ValueTask<TValue> ReadFromJsonAsync<TValue>(
            this HttpRequest request, 
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask<TValue> ReadFromJsonAsync<TValue>(
            this HttpRequest request, 
            JsonSerializerOptions? options, 
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask<object> ReadFromJsonAsync(
            this HttpRequest request, 
            Type type, 
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask<object> ReadFromJsonAsync(
            this HttpRequest request, 
            Type type, 
            JsonSerializerOptions? options, 
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask WriteAsJsonAsync<TValue>(
            this HttpResponse response, 
            TValue value, 
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask WriteAsJsonAsync<TValue>(
            this HttpResponse response, 
            TValue value, 
            JsonSerializerOptions? options,
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask WriteAsJsonAsync<TValue>(
            this HttpResponse response,
            TValue value,
            JsonSerializerOptions? options,
            string? contentType,
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask WriteAsJsonAsync(
            this HttpResponse response,
            Type type,
            object? value,
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask WriteAsJsonAsync(
            this HttpResponse response,
            Type type,
            object? value,
            JsonSerializerOptions? options,
            CancellationToken cancellationToken = default) { throw null; }

        public static ValueTask WriteAsJsonAsync(
            this HttpResponse response,
            Type type,
            object? value,
            JsonSerializerOptions? options,
            string? contentType,
            CancellationToken cancellationToken = default) { throw null; }
    }

    public class JsonOptions
    {
        public JsonSerializerOptions SerializerOptions { get; }
    }
}

Design Notes

0: General Shape

There are three primary APIs here:

  • Detect if the request body is JSON
  • Read JSON from the body
    • Using a static type (TValue)
    • Using a dynamic type (Type type)
  • Write JSON to the body
    • Using a static type (TValue value)
    • Using a dynamic type (Type type, object value)

In general I've tried to keep naming consistent with the functionality for reading the form (HasFormContentType, ReadFormAsync()).

1: HasJsonContentType

I tried to make the design of this similar to HasFormContentType which already exists on the HttpRequest. I considered as well making this an extension method, but there's no good reason to deviate from the pattern we established with form.

There's nothing about this property that makes it specific to a serializer, it's a straightforward comparison of the content type.

2: Namespace

One of the challenges in this area is that if we park the best method names to mean System.Text.Json, then this will be confusing to someone using another serializer. It would be really annoying to use JIL and see ReadJsonAsync() show up everywhere, but mean System.Text.Json. For this reason I put these extensions in a different namespace.

You could imagine that another serializer would want to provide similar functionality. I want to make sure the existing serializer ecosystem can coexist with this addition.


In my mind there are a few ways to address this challenge:

  • Create a serializer abstraction
  • Put System.Text.Json in the method names
  • Make this an extra package
  • Use a namespace to isolate this functionality

Creating a serializer abstraction has a bunch of problems:

  • The abstraction would have to go in the BCL where they (likely) don't want it
  • We'd likely need to require service registration (or coupling in hosting) to use these extensions
  • We'd have to remove JsonSerializerOptions from the APIs

I reject this option, I think it defeats enough of our stated goals.


Considering other options - the namespace seems like the best, most flexible choice. Putting System.Text.Json in the names would be flexible as well, but would be ugly. So I'm pitching the namespace.

I think any of the non-abstraction options would be reasonable for us to choose while still meeting our goals.

3: ReadJsonAsync and Overload Set

I took the approach of defining lots of overloads here with just the cancellation token as an optional parameter.

The reason why is because of what happens when you using cancellation tokens with other optional parameters. Imagine that I made a single overload with optional parameters for both options and cancellation token.

public static ValueTask<TValue> ReadJsonAsync<TValue>(
    this HttpRequest request, 
    JsonSerializerOptions options = default, 
    CancellationToken cancellationToken = default) { throw null; }

await httpContext.Request.ReadJsonAsync<WeatherForecast>(myCancellationToken); // compile error

await httpContext.Request.ReadJsonAsync<WeatherForecast>(null, myCancellationToken); // works
await httpContext.Request.ReadJsonAsync<WeatherForecast>(cancellationToken: myCancellationToken); // works

We can avoid this ugliness by defining more overloads.

4: Media Type and Encoding

There are a lot of overloads of WriteJsonAsync - I'm interested in ways to cut this down.

We need to expose the media type as an option because it's becoming more common to use suffix content types like application/cloud-events+json. This fills a gap in what traditional media types do - a media type like application/json describes a data-representation, but says nothing about the kind of data being represented. More modern designs like CloudEvents will use suffix content types to describe both.

We need to expose the encoding because we MVC supports it (it's a goal to have MVC call this API), and because we still have the requirement to support things like GB2312.

I don't think either of these things are contraversial, we should debate whether we're happy with the design being proposed here as the best way to express this flexibility.

5: Managing JsonSerializerOptions

We need a strategy to deal with the fact that the default settings of JsonSerializerOptions are bad for the web. We want the serializer to output camelCase JSON by default, and be more tolerant of casing differences on input. Also because we know that we're always outputting text with a character encoding, we can be more relaxed about the set of characters we escape compared to the defaults.

I reject the idea that we'd give the user the default JsonSerializerOptions through these APIs and make it their job to manage, because that conflicts with the goal of this being the easiest way to do JSON - we want these APIs to have good defaults for the web.

There's a couple of options for how we could implement this:

  • Have a static instance
  • Have a feature on the http context
  • Use options

Each of these have a downside. The static is a static, the downside is that its static. Using a feature either allocates a bunch or has wierd coupling (kestrel coupled to a serializer). Using options has some runtime overhead for the service lookup. Of these options seems like the best choice. We could also use the options approach to share the options instance between MVC's JsonOptions and this one for compatibility.

Behavious

HasJsonContentType

This method will return true when a request has a JSON content type, that is:

  • application/json
  • text/json
  • application/*+json

Null or empty content type is not considered a match.

ReadJsonAsync

The overloads of ReadJsonAsync will throw an exception if the request does not have a JSON content type (similar to ReadFormAsync).

Depending on the value of CharSet the method may need to create a stream to wrap the request body and transcode - System.Text.Json only speaks UTF8. We're in discussions with CoreFx about moving our transcoding stream implementations into the BCL. We will assume UTF8 if no CharSet was provided, the serializer/reader will validate the correctness of the bytes.

We'll call the appropriate overload of JsonSerializer.DeserializeAsync to do the heavy lifting.


There's a couple of usability concerns here related to error handling. These APIs optimize for the happy path:

  • Throws exceptions for non-JSON content type
  • Throws exceptions for invalid JSON content

Someone who wants to handle both of these errors and turn them into a 400 would need to write an if for the content type, and a try/catch for the possible exceptions from deserialization.

It would be possible to make a TryXyz set of APIs as well - they would end up handling the exception for you. Since these are extension methods, they can't really log (without service locator).

With a union:

public static ValueTask<(bool success, TValue value)> TryReadJsonAsync<TValue>(this HttpRequest request);

endpoints.MapPost("/weather", async context =>
{
    var result = await context.Request.TryReadJsonAsync<WeatherForecast>();
    if (!result.success)
    {
        context.Response.StatusCode = StatusCodes.Status400BadRequest;
        return;
    }

    await UpdateDatabase(result.value);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

A possibly better version:

public static ValueTask<TValue> ReadJsonOrDefaultAsync<TValue>(this HttpRequest request);

endpoints.MapPost("/weather", async context =>
{
    var weather = await context.Request.ReadJsonOrDefaultAsync<WeatherForecast>();
    if (weather is null)
    {
        context.Response.StatusCode = StatusCodes.Status400BadRequest;
        return;
    }

    await UpdateDatabase(weather);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

WriteJsonAsync

Prior to doing the serialization to the response body, the WriteJsonAsync method will write a content type (with CharSet) to the Content-Type header. If no content type is specified then application/json will be used. If no encoding is specified, then UTF8 will be used.

Serialization will call JsonSerializer.SerializeAsync - and provide a wrapper stream if an encoding other than UTF8 is in use.

Code Samples

Reading JSON from the request.

endpoints.MapPost("/weather", async context =>
{
    var weather = await context.Request.ReadJsonAsync<WeatherForecast>();
    await UpdateDatabase(weather);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

Writing JSON to the response.

endpoints.MapGet("/weather/{city}", async context =>
{
    var city = (string)context.Request.RouteValues["city"];
    var weather = GetFromDatabase(city);

    await context.Response.WriteJsonAsync(weather);
});

Writing JSON to the response with explicit content type.

endpoints.MapGet("/weather/{city}", async context =>
{
    var city = (string)context.Request.RouteValues["city"];
    var weather = GetFromDatabase(city);

    await context.Response.WriteJsonAsync(weather, options: null, mediaType: "application/weather+json");
});

I'm not completely in love with this one. We might want to think about making more parameters optional.


Explicitly handling bad content-type

endpoints.MapPost("/weather", async context =>
{
    if (!context.Request.HasJsonContentType)
    {
        context.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType;
        return;
    }

    var weather = await context.Request.ReadJsonAsync<WeatherForecast>();
    await UpdateDatabase(weather);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

Letting routing handle bad content-type (possible feature)

endpoints.MapPost("/weather", async context =>
{
    var weather = await context.Request.ReadJsonAsync<WeatherForecast>();
    await UpdateDatabase(weather);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
})
.WithRequiredContentType("application/json");
@rynowak rynowak added enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 15, 2019
@rynowak rynowak self-assigned this Nov 15, 2019
@davidfowl davidfowl added this to the 5.0.0-preview1 milestone Nov 16, 2019
@davidfowl davidfowl added area-servers and removed feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform labels Nov 16, 2019
@Tratcher
Copy link
Member

Tratcher commented Nov 16, 2019

1: HasJsonContentType

I tried to make the design of this similar to HasFormContentType which already exists on the HttpRequest. I considered as well making this an extension method, but there's no good reason to deviate from the pattern we established with form.

There's nothing about this property that makes it specific to a serializer, it's a straightforward comparison of the content type.

Not a fan of adding any more stuff directly on HttpRequest. HasFormContentType mainly makes sense because Form is also there, and in hindsight neither should have been.

When all of the other new JSON APIs are extensions, HasJsonContentType might as well be too.

@rynowak
Copy link
Member Author

rynowak commented Nov 16, 2019

1: HasJsonContentType

I tried to make the design of this similar to HasFormContentType which already exists on the HttpRequest. I considered as well making this an extension method, but there's no good reason to deviate from the pattern we established with form.
There's nothing about this property that makes it specific to a serializer, it's a straightforward comparison of the content type.

Not a fan of adding any more stuff directly on HttpRequest. HasFormContentType mainly makes sense because Form is also there, and in hindsight neither should have been.

When all of the other new JSON APIs are extensions, HasJsonContentType might as well be too.

I think either one of these could be reasonable, I went back and forth on which one to propose. It accomplishes the same goal either way, just slightly inconsistent in terms of syntax.

@davidfowl
Copy link
Member

While I agree it would be ideal to not add more virtuals like this to the HttpContext, extension methods for things like this aren't great. Would we still do this if we add the Try methods?

@xsoheilalizadeh
Copy link

Why the HttpResponse isn't readable and I can't see any ReadJsonAsync extension method for that in your API proposal.

@davidfowl
Copy link
Member

Why the HttpResponse isn't readable and I can't see any ReadJsonAsync extension method for that in your API proposal.

The HttpResponse is write only. You can read the body if you replace the output stream with a readable stream (like a memory stream or something more efficient...)

@xsoheilalizadeh
Copy link

You can read the body if you replace the output stream with a readable stream (like a memory stream or something more efficient...)

I did it in this way and not sure it's efficient. Could you show me similar internal usage of reading HttpResponse in ASP.NET Core?

@jstclair
Copy link

In the second-to-last example, isn’t that mixing up Content-Type and Accept?

@Yves57
Copy link
Contributor

Yves57 commented Nov 16, 2019

Request content type check

if (!context.Request.HasJsonContentType)
{
    context.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType;
    return;
}

seems a little bit painful to write to each endpoint.
But .WithRequiredContentType("application/json") breaks the imperative code idea for me (and there is no possibility anymore to set a custom status code).

Extension methods target

But I was asking myself if imperative involved low level. For example attach the extension methods to HttpRequest and HttpResponse is the most logical in a technical point of view, but if the extension methods were attached to HttpContext, the code would be maybe clearer to read? In the same interrogation imperative / low level, are there other methods like WriteStatusCode in the pipe (not mandatory for a low level functionality, but useful in a imperative code)?

endpoints.MapPost("/weather", async context =>
{
    if (await context.TryReadJsonAsync<WeatherForecast>(out var weather))
    {
       await UpdateDatabase(weather);

       await context.WriteJsonAsync(weather);
   }
   else
   {
      context.WriteStatusCode(StatusCodes.Status415UnsupportedMediaType);
      // await context.WriteStatusCodeAsync(StatusCodes.Status415UnsupportedMediaType);
   }
});

@Rinsen
Copy link

Rinsen commented Nov 16, 2019

This looks really good and useful!

Will it be possible to use together with dependency injection in an easy way in a library for example? Also without resolving the whole huge business implementation that I might have in a single huge ctor for all mappings.
Will it also be possible to use standard scoped services here in any way?

The only examples I found did this in the Configure() method.

@poke
Copy link
Contributor

poke commented Nov 16, 2019

it's a goal to have MVC call this API

Wouldn't break that the ability though to use different serializers like Json.NET with MVC? That would be a huge breaking change for anyone that relies on the ability to switch to a serializer with a larger feature set.

Wouldn't it make more sense to use the existing abstractions that exist, to allow users to have the choice?

@nil4
Copy link
Contributor

nil4 commented Nov 16, 2019

Rather than coupling these methods to the HTTP request and response objects, couldn't they be targeting the PipeReader/PipeWriter or other abstraction for the request/response bodies? After all, that's what is actually being read/written. These may be more generally useful beyond HttpContext.

@epignosisx
Copy link
Contributor

How would this play with model validation? Will there be a way of getting a hold of the model validation system through DI?

@JamesNK
Copy link
Member

JamesNK commented Nov 16, 2019

Important scenario: Reading and writing JSON DOM. Think of JObject and JArray from Json.NET, but with the new JSON DOM:

endpoints.MapPost("/weather", async context =>
{
    var weather = await context.Request.ReadJsonAsync<JsonObject>();
    await UpdateDatabase(weather["id"], weather["state"]);

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

Hopefully System.Text.Json.JsonSerializer will Just Work when given one of these JSON DOM objects, but it is worth double checking with the JSON crew.

@JamesNK
Copy link
Member

JamesNK commented Nov 16, 2019

At risk of starting a bikeshedding sesh, the methods on JsonSerializer were changed from Read/Write to Serialize/Deserialize after a usability study.

ReadJsonAsync -> DeserializeJsonAsync?

Serialize/Deserialize would be more consistent with JsonSerializer.

@davidfowl
Copy link
Member

Rather than coupling these methods to the HTTP request and response objects, couldn't they be targeting the PipeReader/PipeWriter or other abstraction for the request/response bodies? After all, that's what is actually being read/written. These may be more generally useful beyond HttpContext.

They should be on the request and response. There's already and API to read from a Stream but not a PipeReader and PipeWriter). If those get added, they should be in corefx, not in ASP.NET Core.

The other reason I prefer them on the HttpRequest and HttpResponse is that it allows us to change the implementation (from Stream to PipeReader) and not break consuming code.

@davidfowl
Copy link
Member

At risk of starting a bikeshedding sesh, the methods on JsonSerializer were changed from Read/Write to Serialize/Deserialize after a usability study.
ReadJsonAsync -> DeserializeJsonAsync?
Serialize/Deserialize would be more consistent with JsonSerializer.

We could do another study 😄 , I still prefer Read/Write since those are the verbs we use today https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.formatters.inputformatter?view=aspnetcore-3.0

@davidfowl
Copy link
Member

@poke Wouldn't it make more sense to use the existing abstractions that exist, to allow users to have the choice?

Which abstractions? Anything that exists today live in the wrong layer in the stack. That's another challenge. We don't want to push those things (which have the wrong namespace) lower into the core abstractions.

That said, @rynowak what would it look like if we added an abstraction (even if it lives in corefx)?

@davidfowl
Copy link
Member

@Yves57 But I was asking myself if imperative involved low level. For example attach the extension methods to HttpRequest and HttpResponse is the most logical in a technical point of view, but if the extension methods were attached to HttpContext, the code would be maybe clearer to read? In the same interrogation imperative / low level, are there other methods like WriteStatusCode in the pipe (not mandatory for a low level functionality, but useful in a imperative code)?

I think it should be on the request and response, not HttpContext (we don't have much on the HttpContext today). That code sample does make me lean towards the Try* API more though.

@davidfowl
Copy link
Member

@Rinsen

Will it be possible to use together with dependency injection in an easy way in a library for example? Also without resolving the whole huge business implementation that I might have in a single huge ctor for all mappings.
Will it also be possible to use standard scoped services here in any way?

Are you asking specifically about injecting services into those routing delegates?

@JamesNK
Copy link
Member

JamesNK commented Nov 17, 2019

I still prefer Read/Write since those are the verbs we use today

No one uses inputters/outputters directly. I don't think we should pay them any attention.

More interesting prior art is ReadAsAsync<T> from System.Net.Http.Formatting - https://docs.microsoft.com/en-us/previous-versions/aspnet/hh835763(v%3Dvs.118)

@davidfowl
Copy link
Member

And ReadFormAsync as @rynowak mentioned.

@JamesNK
Copy link
Member

JamesNK commented Nov 17, 2019

        public static ValueTask WriteJsonAsync(
            this HttpResponse response,
            Type type,
            object? value,
            CancellationToken cancellationToken = default) { throw null; }

What does the type argument do here? Why an explicit type vs value?.GetType()?

@Tratcher
Copy link
Member

You can read the body if you replace the output stream with a readable stream (like a memory stream or something more efficient...)

I did it in this way and not sure it's efficient. Could you show me similar internal usage of reading HttpResponse in ASP.NET Core?

Your usage is adequate but will have issues with larger responses. The main optimization we make in our own components is to do what we call write-through processing. The bytes are processed as they're written rather than copied back at the end.
https://github.com/aspnet/AspNetCore/blob/426a70c4506cd000cbf827d8bfa1c4a7ad8aa45d/src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs#L108-L123
https://github.com/aspnet/AspNetCore/blob/426a70c4506cd000cbf827d8bfa1c4a7ad8aa45d/src/Middleware/ResponseCaching/src/Streams/ResponseCachingStream.cs#L133-L157

@pimbrouwers
Copy link

@pimbrouwers Why not just wrap your request delegate's in a closure and poor-man/woman inject the services yourself?

Because that doesn't work when you need to access services with a scoped lifetime.

It does if you bring into context the IServiceCollection. I'm also not certain that DI should be the default approach for every project, so it may not even be a concern in all cases.

@Rinsen
Copy link

Rinsen commented Nov 18, 2019

@pimbrouwers Why not just wrap your request delegate's in a closure and poor-man/woman inject the services yourself?

Because that doesn't work when you need to access services with a scoped lifetime.

It does if you bring into context the IServiceCollection. I'm also not certain that DI should be the default approach for every project, so it may not even be a concern in all cases.

If you want to use DI it is really not a god option to not use DI. And injecting a IServiceCollection or some other factory method is not a compelling solution at all. Then I rather go with som other pattern like pure MVC or something else.

@davidfowl
Copy link
Member

@pimbrouwers It does if you bring into context the IServiceCollection. I'm also not certain that DI should be the default approach for every project, so it may not even be a concern in all cases.

Not sure what you mean, you don't have to use DI if you don't want, but you can't really use closures (or I'm misunderstanding what you mean).

Consider the following:

endpoints.MapPost("/weather/{id}", async context =>
{
    var id = context.Request.RouteValues["id"];
    var weather = await context.Request.ReadJsonAsync<JsonObject>();

    using var db = new WeatherContext();

    var w = db.Weathers.Find(int.Parse(id));
    w.State = weather.State;
    await db.SaveChangesAsync();

    context.Response.StatusCode = StatusCodes.Status202Accepted;
});

Now you want to inject the WeatherContext so this can be tested. Here are some options:

Task PostWeather(HttpContext context, WeatherContext db) 
{
    var id = context.Request.RouteValues["id"];
    var weather = await context.Request.ReadJsonAsync<JsonObject>();

    var w = db.Weathers.Find(int.Parse(id));
    w.State = weather.State;
    await db.SaveChangesAsync();

    context.Response.StatusCode = StatusCodes.Status202Accepted;
}

// 1.
endpoints.MapPost("/weather/{id}", async context =>
{
    using var db = new WeatherContext();
    await PostWeather(context, db);
});

// 2.
endpoints.MapPost("/weather/{id}", async context =>
{
    // Scoped service
    var db = context.RequestServices.GetRequiredService<WeatherContext>();
    await PostWeather(context, db);
});

// 3. We could add overloads that make this easier

endpoints.MapPost("/weather/{id}", async (HttpContext context, WeatherContext db) =>
{
    // Scoped service
    await PostWeather(context, db);
});

@pimbrouwers
Copy link

pimbrouwers commented Nov 19, 2019

@davidfowl -- Sorry for the confusion. I should've been more specific, clear and accurate. I only meant to imply that developer's don't always HAVE to jump right into the built-in IoC container. And that dependencies could be provided by others means, in (my opinion) sometimes a more explicit way. Either using closures, or a plain old object. For example:

using System;
using System.Data;
using System.Data.SQLite;
using System.Net;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace WebApp {
    public class Program {
        public static async Task<int> Main(string[] args) {
            try {
                var db = new Db("Data Source=:memory:;Version=3;New=True;");
                var server = new Server(db);

                var host = new WebHostBuilder()
                    .UseKestrel()
                    .UseUrls("http://localhost:5000/;https://localhost:5001")
                    .ConfigureServices(_configureServices)
                    .Configure(_configure(server))
                    .Build();

                await host.RunAsync();

                return 0;
            } catch {
                return -1;
            }
        }

        static void _configureServices(IServiceCollection services) {
            services.AddRouting();
        }

        static Action<WebHostBuilderContext, IApplicationBuilder> _configure(Server server) {
            return (context, app) => {
                app.UseRouting();
                app.UseEndpoints(server.Routes);
                app.Run(server.handleNotFound);
            };
        }
    }

    internal class Server {
        readonly Db _db;

        internal Server(Db db) {
            _db = db;
        }

        internal void Routes(IEndpointRouteBuilder r) {
            r.MapGet("/post/get", handlePostGet);
            r.MapGet("/hello/{name}", handleHelloName);
            r.MapGet("/", handleHello);
        }

        internal Task handleNotFound(HttpContext ctx) {
            ctx.Response.StatusCode = (int)HttpStatusCode.NotFound;
            return ctx.Response.WriteAsync("Post not found.");
        }

        Task handleHello(HttpContext ctx) =>
            ctx.Response.WriteAsync("Hello World!");

        Task handleHelloName(HttpContext ctx) =>
            ctx.Response.WriteAsync("hello " + ctx.Request.RouteValues["name"]);

        Task handlePostGet(HttpContext ctx) {
            using var conn = _db.CreateConnection();
            using var cmd = conn.CreateCommand();
            cmd.CommandText = "SELECT 1 as post_id, 'hello world' as title, 'some text' as body";

            using var rd = cmd.ExecuteReader();
            if (rd.Read()) {
                return ctx.Response.WriteAsync(rd.GetString(rd.GetOrdinal("body")));
            } else {
                return handleNotFound(ctx);
            }
        }
    }

    internal class Db {
        readonly string _connStr;

        internal Db(string connStr) {
            _connStr = connStr ?? throw new ArgumentNullException(nameof(connStr));
        }

        internal IDbConnection CreateConnection() =>
            new SQLiteConnection(_connStr).OpenAndReturn();
    }
}

This example is trivial, uses poor naming (I did this for simplicity) and missing a bunch of boilerplate code (null dep checks, error checking etc.), but demonstrates the point I'm trying to make. This approach I think is testable and clear. I also like that Server can be constructed prior to kestrel's activation, which becomes useful if you want to use precompiled templates (ex: Stubble/Scriban/Nustache etc.) and wish to panic/exit if something goes amuck. You could also assert the db in this manner if desired.

I could be missing something glaring, I definitely don't have a CS background. For whatever that's worth.

As someone who came from outside .NET, I wish the approach in teaching the platform started from this perspective. As opposed to "here is full-blown MVC and EF". I believe that we'd like have greater adoption, and probably better developer's.

Just my two uneducated cents.

p.s. @davidfowl loving the lack of braces on those usings?

@jstclair
Copy link

@jstclair

In the second-to-last example, isn’t that mixing up Content-Type and Accept?

I don't think so.

If the client sends a request with Content-Type: application/xml to a handler that only knows how to process JSON, then 415 is the right response.

One enhancement that could be asked for here, is an easy way to know the client want to receive JSON (Accept header). That can result in a 406 - however, we generally just don't return 406 today. By the time you're fully checking both input and output content types, this starts to feel a lot like what MVC does.

Sorry if it feels like I'm bike-shedding here - but in the example, it's a GET and the parameter is via the route, so I'm unclear why you'd care what the Content-Type is? It's fine to explicitly avoid the content-negotiation dance, but the proposed API changes seem to assume that checking Content-Type is the same as checking Accept

@Rinsen
Copy link

Rinsen commented Nov 19, 2019

@davidfowl -- Sorry for the confusion. I should've been more specific, clear and accurate. I only meant to imply that developer's don't always HAVE to jump right into the built-in IoC container. And that dependencies could be provided by others means, in (my opinion) sometimes a more explicit way. Either using closures, or a plain old object. For example:
.....

I feel now that I was a bit to hard in my response, I strongly agree with you that it's great to have both options! Thad adds a lot of value to the stack!

@davidfowl
Copy link
Member

davidfowl commented Nov 19, 2019

@pimbrouwers In your example, the Db class is a singleton. That's why I said closures don't work for all scenarios. If you want things created per request and you want to be able to pass them explicitly, you're kinda stuck with the IoC container or code is that creating objects per request (like a factory or using new)

@rynowak
Copy link
Member Author

rynowak commented Nov 19, 2019

@jstclair

In the second-to-last example, isn’t that mixing up Content-Type and Accept?

I don't think so.
If the client sends a request with Content-Type: application/xml to a handler that only knows how to process JSON, then 415 is the right response.
One enhancement that could be asked for here, is an easy way to know the client want to receive JSON (Accept header). That can result in a 406 - however, we generally just don't return 406 today. By the time you're fully checking both input and output content types, this starts to feel a lot like what MVC does.

Sorry if it feels like I'm bike-shedding here - but in the example, it's a GET and the parameter is via the route, so I'm unclear why you'd care what the Content-Type is? It's fine to explicitly avoid the content-negotiation dance, but the proposed API changes seem to assume that checking Content-Type is the same as checking Accept

Ah! Thats the mistake. I'll fix the example, thanks!

@alefranz
Copy link
Contributor

5: Managing JsonSerializerOptions
...
Using options has some runtime overhead for the service lookup. Of these options seems like the best choice. We could also use the options approach to share the options instance between MVC's JsonOptions and this one for compatibility.

That feels the best option. The default options as you said are sub-optimal (my standard is: camelCase properties, camelCase enums, omit null properties) so there is a need for an easy way to change the options, in a single place for everything (uncommon to have different options per endpoint).

Writing JSON to the response with explicit content type.
...
I'm not completely in love with this one. We might want to think about making more parameters optional.

I think it would be nice to have a way to specify globally a media type, like the options, as it is not uncommon to use same custom media type and serialization options for all endpoints.
This would define the default content-type, not sure if it would be needed for the Accept value as well.

Letting routing handle bad content-type (possible feature)
...
.WithRequiredContentType("application/json");

It's not clear to me if this would only handle the content-type or also the Accept value.

Anyway, if this is going to happen, and there is going to be a default media type (as per above comment), then there should probably be a method to enforce the default like .WithRequiredContentType() or .WithRequiredDefaultContentType()


I really like this as I think it would be really useful to write simple endpoints.
However, to be a viable solution out of quick prototyping, there should be some syntactic sugar to have content negotiation and error handling, otherwise creating a Controller would still be more convenient then to write inline handler.

What about having some standard exceptions that can be thrown by this methods and handled by default, so you can either:

  • use those methods and handle the exceptions yourself
  • use the Try methods and handle the error scenarios
  • (NEW) don't handle the error scenario and it would just work

e.g.

endpoints.MapPost("/weather", async context =>
{
    var weather = await context.Request.ReadJsonAsync<WeatherForecast>();
    // Throws exception then translated to 415 if body is not json
    // Throws exception then translated to 400 if it fails to parse

    await UpdateDatabase(weather);

    await context.Response.WriteJsonAsync(weather);
    // Throws exception then translated to 406 if the Accept is not json
    // Not ideal not being able to fail fast, but you can not know upfront is there is going to be a body and it is going to be json.
    // Easy to see the drawbacks in this specific example as you get an error even if you had updated the DB
});

@analogrelay analogrelay removed this from the 5.0.0-preview1 milestone Mar 11, 2020
@analogrelay analogrelay added this to the Next sprint planning milestone Mar 30, 2020
@rynowak rynowak assigned JamesNK and unassigned rynowak May 1, 2020
@rynowak rynowak added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 1, 2020
@rynowak
Copy link
Member Author

rynowak commented May 1, 2020

@JamesNK - updated with our notes

@Tratcher
Copy link
Member

Tratcher commented May 2, 2020

You know the other place people have been begging us to add these json extensions? Session. Our docs even have a basic version of them you can paste into your app.

@rynowak rynowak added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 4, 2020
@rynowak
Copy link
Member Author

rynowak commented May 4, 2020

API approved - we've made some small edits:

ReadJsonAsync -> ReadFromJsonAsync
WriteJsonAsync -> WriteAsJsonAsync
mediaType -> contentType

Also, it looks like IdentityServer has already done the work to make their extensions scoped: IdentityServer/IdentityServer4#3699

@BrennanConroy
Copy link
Member

@JamesNK Can you get to this in preview6?

@JamesNK JamesNK closed this as completed Jun 2, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.