Skip to content

Support more types of redirection during prerendering. Fixes #11591 #12418

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 22, 2019
Merged

Support more types of redirection during prerendering. Fixes #11591 #12418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68a42d6
E2E test to show current behavior
SteveSandersonMS Jul 21, 2019
064fe0b
Actually support base-relative, root-relative, and absolute redirecti…
SteveSandersonMS Jul 21, 2019
0bf9e25
Fix MVC functional test
SteveSandersonMS Jul 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Components/Server/src/Circuits/RemoteUriHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,10 @@ protected override void NavigateToCore(string uri, bool forceLoad)

if (_jsRuntime == null)
{
throw new NavigationException(uri);
var absoluteUriString = ToAbsoluteUri(uri).ToString();
throw new NavigationException(absoluteUriString);
Copy link
Member

Choose a reason for hiding this comment

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

Since I'm digging around in this code, does it make sense to change the contract of this method (I can do it in my changes).

Basically it's surprising when something says you're going to get a URI, might be relative, might not be. Not handling one of those cases is the source of this bug.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Jul 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm open to either choice here.

Having the base class auto-resolve the URL does discard some info that might have been useful (e.g., maybe the implementer wants to vary behavior for relative vs absolute URLs for some reason), but after consideration, I can't think of any definite cases where it would really be disadvantageous.

However, if we're biasing decisions towards future back-compatibility concerns, then it would be safest to say that implementers are responsible for handling all the cases, since we can remove cases later but can't add new ones.

Overall, it won't matter much because the only people subclassing UriHelperBase are basically us, so I'm fine with whatever you choose.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Steve, I was actually going to make that same comment in that PR until I saw that the implementations simply forward to the underlying location object in the browser.

It would be good to handle it as it would make the situation better for prerendering though.

}

_jsRuntime.InvokeAsync<object>(Interop.NavigateTo, uri, forceLoad);
}

Expand Down
22 changes: 22 additions & 0 deletions src/Components/test/E2ETest/ServerExecutionTests/PrerenderingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Net;
using System.Net.Http;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure;
using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures;
using Microsoft.AspNetCore.E2ETesting;
Expand Down Expand Up @@ -75,6 +79,24 @@ public void CanReadUrlHashOnlyOnceConnected()
() => Browser.FindElement(By.TagName("strong")).Text);
}

[Theory]
[InlineData("base/relative", "prerendered/base/relative")]
[InlineData("/root/relative", "/root/relative")]
[InlineData("http://absolute/url", "http://absolute/url")]
public async Task CanRedirectDuringPrerendering(string destinationParam, string expectedRedirectionLocation)
{
var requestUri = new Uri(
_serverFixture.RootUri,
"prerendered/prerendered-redirection?destination=" + destinationParam);

var httpClient = new HttpClient(new HttpClientHandler { AllowAutoRedirect = false });
var response = await httpClient.GetAsync(requestUri);

var expectedUri = new Uri(_serverFixture.RootUri, expectedRedirectionLocation);
Assert.Equal(HttpStatusCode.Redirect, response.StatusCode);
Assert.Equal(expectedUri, response.Headers.Location);
}

private void BeginInteractivity()
{
Browser.FindElement(By.Id("load-boot-script")).Click();
Expand Down
16 changes: 16 additions & 0 deletions src/Components/test/testassets/BasicTestApp/PrerenderedRedirection.razor
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
@page "/prerendered-redirection"
@inject IUriHelper UriHelper

@{
throw new InvalidOperationException("The rendering logic should never be executed");
}

@code {
protected override Task OnInitializedAsync()
{
var uri = UriHelper.GetAbsoluteUri();
var destination = uri.Substring(uri.IndexOf("?destination=") + 13);
UriHelper.NavigateTo(destination);
return Task.CompletedTask;
}
}
5 changes: 2 additions & 3 deletions src/Mvc/test/Mvc.FunctionalTests/ComponentRenderingFunctionalTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public async Task Redirects_Navigation_Component()

// Assert
await response.AssertStatusCodeAsync(HttpStatusCode.Redirect);
Assert.Equal("/navigation-redirect", response.Headers.Location.ToString());
Assert.Equal("http://localhost/navigation-redirect", response.Headers.Location.ToString());
}

[Fact]
Expand All @@ -99,10 +99,9 @@ public async Task Redirects_Navigation_ComponentInteractive()

var response = await client.GetAsync("http://localhost/components/Navigation/false");

// Assert
// Assert
await response.AssertStatusCodeAsync(HttpStatusCode.Redirect);
Assert.Equal("/navigation-redirect", response.Headers.Location.ToString());
Assert.Equal("http://localhost/navigation-redirect", response.Headers.Location.ToString());
}

[Fact]
Expand Down