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

Add new "api" project template that supports native AOT #46064

Merged
merged 21 commits into from
Jan 13, 2023

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Jan 12, 2023

This adds a new "api" project template that represents an HTTP API project that uses minimal APIs to implement two endpoints: one that returns a list of todos, and another that returns an todo with a given id. The todos are randomly generated once at startup:

var builder = WebApplication.CreateBuilder(args);
builder.Logging.AddConsole();
var app = builder.Build();

var sampleTodos = TodoGenerator.GenerateTodos().ToArray();

var todosApi = app.MapGroup("/todos");
todosApi.MapGet("/", () => sampleTodos);
todosApi.MapGet("/{id}", (int id) =>
    sampleTodos.FirstOrDefault(a => a.Id == id) is { } todo
        ? Results.Ok(todo)
        : Results.NotFound());

app.Run();

Unlike the existing "webapi" project template (which is not going away), this project template is focused squarely at new, cloud-focused API projects, uses minimal APIs only (no MVC option), and has a first-class option to enable support for publishing as native AOT: dotnet new api -aot. When the -aot flag is passed, additional code is emitted in the project to ensure the JSON serializer is configured appropriately to work in native AOT (i.e. a JsonSerializerContext is declared and hooked up to DI). When native AOT is enabled this template currently produces projects that emit build/publish warnings but of course the point is for those to be addressed by the all the currently in-progress work for ASP.NET Core native AOT:

using System.Text.Json.Serialization;
using Company.ApiApplication1;

var builder = WebApplication.CreateBuilder(args);
builder.Logging.AddConsole();

builder.Services.ConfigureHttpJsonOptions(options =>
{
    options.SerializerOptions.AddContext<AppJsonSerializerContext>();
});

var app = builder.Build();

var sampleTodos = TodoGenerator.GenerateTodos().ToArray();

var todosApi = app.MapGroup("/todos");
todosApi.MapGet("/", () => sampleTodos);
todosApi.MapGet("/{id}", (int id) =>
    sampleTodos.FirstOrDefault(a => a.Id == id) is { } todo
        ? Results.Ok(todo)
        : Results.NotFound());

app.Run();

[JsonSerializable(typeof(Todo[]))]
internal partial class AppJsonSerializerContext : JsonSerializerContext
{

}

In line with the goals we have for native AOT and this style of project, there are metrics we intend to hit for projects created with this template regarding publish output file size, startup time, working set, and throughput. At this point that means this template configures the project to use Workstation GC (via a property in the project file), perform full trimming, and suppresses the publishing of IIS-related assets. At a later stage these properties will likely be automatically defaulted to these values in the Web SDK itself based on the value of the <PublishAot> property, at which point this template will be updated. When the new WebApplication.CreateSlimBuilder() API is added, this template will be updated to use it too.

This template is currently configured to be hidden in Visual Studio. We'll change it to show up when we're ready to include the overall ASP.NET Core native AOT story in the .NET 8 preview messaging (currently targeting preview.2).

Addresses #45526

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 12, 2023
@DamianEdwards DamianEdwards marked this pull request as ready for review January 12, 2023 23:00
@DamianEdwards DamianEdwards requested review from a team and eerhardt January 12, 2023 23:00
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the "api" name for this template. I don't think most ASP.NET Core developers will understand the distinction from the "webapi" project template. It's also a bit presumptuous 😆

I know we have our own goals for having a "cloud focused" template which we can add "-aot" to, but I think it's better to continue using "webapi" and live with some unsupported combinations with "-aot". Maybe "-aot" could imply "-minimal".

Also, am I right in assuming -aot will work and --publish-native-aot will work, but --aot won't work? Can we support --aot, because -aot is super weird. I have a similar problem with -minimal working but --minimal not in the webapi template.

That said, we can wait for feedback and adjust.

@@ -57,6 +57,8 @@ function Test-Template($templateName, $templateArgs, $templateNupkg, $isBlazorWa
<Import Project="' + $importPath + '/Directory.Build.targets" />
<PropertyGroup>
<DisablePackageReferenceRestrictions>true</DisablePackageReferenceRestrictions>
<TreatWarningsAsErrors>False</TreatWarningsAsErrors>
<TrimmerSingleWarn>false</TrimmerSingleWarn>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to limit these additions to the tested project to only the -aot scenario?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBC this script isn't part of actual template tests, it's only used to support local dev of templates and these additions are intended to help "disconnect" the templates from the various Directory.Build.props in parent dirs from the repo. So, having them apply for all the helper scripts is useful IMO.

[JsonSerializable(typeof(string))]
[JsonSerializable(typeof(Album))]
[JsonSerializable(typeof(Album[]))]
internal partial class AppJsonSerializerContext : JsonSerializerContext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put this in a separate AppJsonSerializerContext.cs file? Is it easy enough to conditionally include files to the template?

Copy link
Member Author

@DamianEdwards DamianEdwards Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in two minds right now and have made all the logical arguments in my head both ways, but landed here aesthetically. Interested to hear others' thoughts. Conditional includes of files is not a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One part of me thinks: "Put each top level type in its own file", because that's the way I would do it. But the way you've proposed it here makes sense as well. Having this tiny little class in its own file seems going to far.

@DamianEdwards
Copy link
Member Author

I don't like the "api" name for this template. I don't think most ASP.NET Core developers will understand the distinction from the "webapi" project template. It's also a bit presumptuous 😆

I know we have our own goals for having a "cloud focused" template which we can add "-aot" to, but I think it's better to continue using "webapi" and live with some unsupported combinations with "-aot". Maybe "-aot" could imply "-minimal".

We've seen evidence that "web API" is a term that isn't widely common outside of the .NET ecosystem and the existing "webapi" template has its current defaults that are problematic to change. I very much like the simplicity of dotnet new api -aot.

Also, am I right in assuming -aot will work and --publish-native-aot will work, but --aot won't work? Can we support --aot, because -aot is super weird. I have a similar problem with -minimal working but --minimal not in the webapi template.

This is the standard pattern we use for our project template options, i.e. a full -- prefixed option name and in specific cases a - prefix shortened alias.

@davidfowl
Copy link
Member

[JsonSerializable(typeof(string))]
[JsonSerializable(typeof(Album))]
[JsonSerializable(typeof(Album[]))]
internal partial class AppJsonSerializerContext : JsonSerializerContext
{

}

Can we just return NotFound without the JSON string?

@DamianEdwards
Copy link
Member Author

DamianEdwards commented Jan 13, 2023

@davidfowl

Can we just return NotFound without the JSON string?

We certainly can, also removing the attribute for string doesn't break the project, but raises a whole bunch of correctness questions. Happy to just change it to return a 404 with no body.

<ServerGarbageCollection>False</ServerGarbageCollection>
<!--#if (NativeAot) -->
<PublishAot>true</PublishAot>
<TrimMode>full</TrimMode>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - I opened https://github.com/dotnet/aspnetcore/issues/46084 to see if we can remove this from the template and just add it to the SDK.

[JsonSerializable(typeof(string))]
[JsonSerializable(typeof(Album))]
[JsonSerializable(typeof(Album[]))]
internal partial class AppJsonSerializerContext : JsonSerializerContext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One part of me thinks: "Put each top level type in its own file", because that's the way I would do it. But the way you've proposed it here makes sense as well. Having this tiny little class in its own file seems going to far.

@eerhardt
Copy link
Member

This is the standard pattern we use for our project template options, i.e. a full -- prefixed option name and in specific cases a - prefix shortened alias.

If that is already the pattern we use for templates, we should follow it. But this is not a valid POSIX compliant design for command line args.

https://learn.microsoft.com/en-us/dotnet/standard/commandline/syntax#aliases

POSIX short forms typically have a single leading hyphen followed by a single character.

This is so you can group them together: git clean -xdf is setting 3 flags at once.

Happy to just change it to return a 404 with no body.

+1 from me.

var todosApi = app.MapGroup("/todos");
todosApi.MapGet("/", () => sampleTodos);
todosApi.MapGet("/{id}", (int id) =>
sampleTodos.FirstOrDefault(a => a.Id == id) is { } todo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: SingleOrDefault seems more appropriate for resolving an element by a unique identifier.

Copy link
Member Author

@DamianEdwards DamianEdwards Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During a recent review of our API scaffolders we were advised that SingleOrDefault has extra overhead (e.g. enumerating the entire list) that isn't worth incurring in these scenarios so I adopted it here too.

@@ -9,4 +9,4 @@ $ErrorActionPreference = 'Stop'

. $PSScriptRoot\Test-Template.ps1

Test-Template "angular" "angular" "Microsoft.DotNet.Web.Spa.ProjectTemplates.7.0.7.0.0-dev.nupkg" $false
Test-Template "angular" "angular" "Microsoft.DotNet.Web.Spa.ProjectTemplates.8.0.8.0.0-dev.nupkg" $false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not relate to this PR but is this something we should be updated regularly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just each major release.

@DamianEdwards DamianEdwards enabled auto-merge (squash) January 13, 2023 21:51
@DamianEdwards DamianEdwards merged commit 4535ea1 into main Jan 13, 2023
@DamianEdwards DamianEdwards deleted the dedward/api-project-template branch January 13, 2023 23:29
@ghost ghost added this to the 8.0-preview1 milestone Jan 13, 2023
Comment on lines +24 to +25
? Results.Ok(todo)
: Results.NotFound());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captainsafia @davidfowl - How is code like this going to allow our minimal API source generator to infer that we need to add Todo to the JSON Source Generation? Will we look through the delegate and find all places that call Results.X? We won't be able to tell by the Delegate's return type, because here that will just be IResult, and not IResult<Todo>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke to @davidfowl about this recently and IIRC there isn't a firm plan for the SG to do this as yet. For now, the app would be responsible for ensuring returned types are declared on a JSON serializer context that is configured in DI. We talked about analyzers and code-fixers that could make the process of keeping these up to date easier, and of course ideally the minimal API SG would do this automatically as you suggest, but as I understand it that capability will not be there initially.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. There's currently no plan to call the JSON source generator from the minimal API one. All we have are vague ideas. I assume the JSON source generator would need to be refactored to do this, but it would be good to attempt this once we have the baseline functionality checked in.

Maybe @brunolins16 can investigate once we're done with the core JSON work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're interested though, I wrote some ghetto logic for this here:

https://github.com/davidfowl/uController/blob/7c1e8f3db1c6a20738eff792386696ff89d4d7e7/src/uController.SourceGenerator/uControllerGenerator.cs#L366

It was for a different purpose though, identifying the metadata for open api.

@ShreyasJejurkar
Copy link
Contributor

And also do we have separate issue where we keep track of warning that we get from ILlinker when publishing this project!? It will easier to track what warnings we have addressed and what needs to be!

And also not sure we can do this or not. Do we have template tests so that we can see how many warnings we get when publishing this project and asset it to some int value. So that we know we got 3 warnings, we need to address that. Once we addresses a issue, we would just chnage the test to 2 value. That we can be sure, we are getting expected number of warnings and can get notify if we get additional warnings and can see whats changed and also where in aspnetcore internal core or code from runtime as there are constant code changes in aot model scenarios (at runtime repo).

@ghost
Copy link

ghost commented Jan 15, 2023

Hi @ShreyasJejurkar. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

MichalStrehovsky added a commit to dotnet/sdk that referenced this pull request Apr 20, 2023
This is similar to the `api` template in ASP.NET that also allows `-aot` (or `--publish-native-aot`) option (see dotnet/aspnetcore#46064).

We set similar properties:
* `<PublishAot>true</PublishAot>` (this is the obvious one)
* `<InvariantGlobalization>true</InvariantGlobalization>` to get rid of ICU dependency on Linux and make the hello world executable ~20% smaller in general, even outside Linux.
* a commented out `<StripSymbols>true</StripSymbols>`. This is for discoverability. We default this to `false` due to platform conventions, but many users would be fine setting this to `true`. `true` is the .NET convention.

I took the string from ASP.NET. I could have also copied the localized strings but I guess it's better to not step on the toes of the localization team.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants