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

Update webapp template to use top-level statements & minimal hosting API #34016

Merged
merged 12 commits into from
Jul 7, 2021

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Jul 1, 2021

Updated project templates to use top-level statements and minimal hosting APIs.

Here's what Program.cs ends up looking like with individual auth. (Note that most of the using statements will get removed when the implicit global usings SDK PR is merged):

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Identity;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using webapp.Data;

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.
builder.Services.AddDbContext<ApplicationDbContext>(options =>
    options.UseSqlite(
        builder.Configuration.GetConnectionString("DefaultConnection")));
builder.Services.AddDatabaseDeveloperPageExceptionFilter();
builder.Services.AddDefaultIdentity<IdentityUser>(options => options.SignIn.RequireConfirmedAccount = true)
    .AddEntityFrameworkStores<ApplicationDbContext>();
builder.Services.AddRazorPages();

var app = builder.Build();

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseDeveloperExceptionPage();
    app.UseMigrationsEndPoint();
}
else
{
    app.UseExceptionHandler("/Error");
    // The default HSTS value is 30 days. You may want to change this for production scenarios, see https://aka.ms/aspnetcore-hsts.
    app.UseHsts();
}

app.UseHttpsRedirection();
app.UseStaticFiles();

app.UseAuthentication();
app.UseAuthorization();

app.MapRazorPages();

app.Run();

Contributes to #33947 #33948 #33944 #32471 #33813

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 1, 2021
Copy link
Member

@bradygaster bradygaster left a comment

Choose a reason for hiding this comment

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

👍

@pranavkm
Copy link
Contributor

pranavkm commented Jul 2, 2021

@SteveSandersonMS / @javiercn in case you'd like to chime in on the formatting / shape of the code.

@LadyNaggaga
Copy link

LadyNaggaga commented Jul 2, 2021

LGTM 👍🏿

@pranavkm
Copy link
Contributor

pranavkm commented Jul 2, 2021

builder.Services.AddDbContext<ApplicationDbContext>(options =>
    options.UseSqlite(
        builder.Configuration.GetConnectionString("DefaultConnection")));

Is it more legible if the connection string is stored in a variable?

var connectionString = builder.Configuration.GetConnectionString("DefaultConnection");
builder.Services.AddDbContext<ApplicationDbContext>(options => options.UseSqlite(connectionString));

@DamianEdwards
Copy link
Member Author

@pranavkm I'm happy to make that change. @davidfowl has some pretty strong opinions on this and did not like my original proposal that declared variables for services and configuration to avoid the builder.Services and builder.Configuration references everywhere.

@DamianEdwards
Copy link
Member Author

Note that when the global implicit namespaces change lands the namespaces in this Program.cs will be reduced to:

using Microsoft.AspNetCore.Identity; // Needed for the IdentityUser type
using Microsoft.EntityFrameworkCore; // Needed for the UseSqlite extension method
using webapp.Data; // Needed for the ApplicationDbContext type

@pranavkm
Copy link
Contributor

pranavkm commented Jul 2, 2021

@davidfowl has some pretty strong opinions on this

I guess let's keep with this for now and we can always update it once @davidfowl's had a chance to weigh in on the change.

- Using minimal hosting APIs
- Using top-level statements
- using minimal hosting APIs
- using top-level statements
@DamianEdwards
Copy link
Member Author

@JamesNK in case you'd like to chime in on the gRPC project template changes.

- uses minimal hosting APIs
- uses top-level statements
- uses minimal hosting APIs
- uses top-level statements
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

gRPC template 👍

- Uses minimal hosting APIs
- Uses top-level statements
- Fixed template testing scripts to support BlazorWasm template properly
- Fixed nullable issues
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

As long as it works!

@davidfowl
Copy link
Member

There's a super subtle difference to where UseRouting is called implicitly with the WebApplicationBuilder that we should make sure is ok. Routing will run before static files in a couple of these templates. Maybe we want to change that back so it's more explicit in those scenarios?

@DamianEdwards
Copy link
Member Author

@davidfowl that's a good point but I must say I love the fact we can omit it. Is the difference big enough to warrant being explicit you think? Is the concern about performance, behavior, correctness, ease of diagnosing failure modes, etc.?

@davidfowl
Copy link
Member

I guess to 2 differences are:

  • Routes can win over static files. This might really only matter for implicit defaults (index.htm etc)
  • Performance of static files as routing runs before it.

These can be worked around obviously by adding an explicit call but maybe we should measure.

@javiercn
Copy link
Member

javiercn commented Jul 5, 2021

  • Performance of static files as routing runs before it.

Doesn't routing run before static files today? Matching at least will.

@javiercn
Copy link
Member

javiercn commented Jul 5, 2021

  • Routes can win over static files. This might really only matter for implicit defaults (index.htm etc)

This is also the case today, isn't it? The MapFallback overloads have explicit constraints to avoid matching things that look like files, if you register a slug route explicitly, it will win over the files.

I believe the StaticFiles middleware avoids doing work if it detects there's an endpoint set on the request, so routes should already win?

@davidfowl
Copy link
Member

Doesn't routing run before static files today? Matching at least will.

No it doesn't. For example here is the Razor pages template:

public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
    if (env.IsDevelopment())
    {
        app.UseDeveloperExceptionPage();
    }
    else
    {
        app.UseExceptionHandler("/Error");
        // The default HSTS value is 30 days. You may want to change this for production scenarios, see https://aka.ms/aspnetcore-hsts.
        app.UseHsts();
    }

    app.UseHttpsRedirection();
    app.UseStaticFiles();

    app.UseRouting();

    app.UseAuthorization();

    app.UseEndpoints(endpoints =>
    {
        endpoints.MapRazorPages();
    });
}

Things that run before routing:

  • DeveloperExceptionPage
  • ExceptionHandler
  • Hsts
  • HttpsRedirection
  • StaticFiles

I believe the StaticFiles middleware avoids doing work if it detects there's an endpoint set on the request, so routes should already win?

That's not the problem the "problem" is that it runs route matching first, which is a change from today. It may be irrelevant but we should be aware of the ordering differences here.

@davidfowl
Copy link
Member

This is also the case today, isn't it? The MapFallback overloads have explicit constraints to avoid matching things that look like files, if you register a slug route explicitly, it will win over the files.

I'm not sure what you mean. If you explicitly call MapFallback? Sure but I'm not talking about that scenario.


builder.Services.AddControllersWithViews(options =>
{
var policy = new AuthorizationPolicyBuilder()
Copy link
Member

Choose a reason for hiding this comment

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

@pranavkm @HaoK I thought we did this with middleware now. Why does this template still use the authorize filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that looks strange. Seems like a relic?

@davidfowl
Copy link
Member

Not gonna lie these templates are impossible to review because of those ifs. You really want to look at the resulting content for each of them. Those could be baseline files checked into the repo or something.

@DamianEdwards
Copy link
Member Author

@davidfowl certain baselines are generated in the scripts folder but purposely excluded as they're not part of the build today (i.e. they wouldn't get updated unless one manually runs the various Run-***-Locally.ps1 scripts). I can put the relevant baselines in a gist if it helps for review for now but any larger change to how template baselines are tracked is outside the scope of this work.

- put connection string in its own variable
- add explicit call to UseRouting() to avoid issue with middleware that re-runs the pipeline with a different path (e.g. UseExceptionHandler)
@DamianEdwards
Copy link
Member Author

I've added calls to UseRouting() in all templates that have a call to UseExceptionHandler as not doing that causes issues today (see #34146).

@pranavkm @davidfowl I did the var connectionString ... change too.

Planning to merge once checks finish on this latest commit.

@DamianEdwards DamianEdwards merged commit 7699bab into main Jul 7, 2021
@DamianEdwards DamianEdwards deleted the damianedwards/minimal-hosting-templates-33944 branch July 7, 2021 01:10
@ghost ghost added this to the 6.0-preview7 milestone Jul 7, 2021
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