Skip to content

Forms PRG and error handling fixes #49472

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

Merged
merged 6 commits into from
Jul 17, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Jul 17, 2023

Fixes #47903 and #49143

Mostly it's just tweaks to the flow in RazorComponentEndpointInvoker, plus a ton of E2E cases.

Forms + streaming rendering

While doing this I realised the way we dispatch form events isn't as integrated into streaming SSR as I thought. What I thought would happen is:

  • Initial synchronous render
  • Streaming begins
    • Any series of further async renders until quiescence
    • Dispatch submit event
    • Any series of (possibly async) renders until quiescence

... but what actually happens is:

  • Initial synchronous render
  • Pause until quiescence, not doing any streaming during this time
  • Dispatch submit event
  • Streaming begins
    • Any series of (possibly async) renders until quiescence

So, it's not streaming until the submit event is dispatched. I tried fixing this but it involves more changes than I was ready to do just hours before the CC deadline. One particularly tricky bit is that the handling for returning 400 errors in the case of invalid/missing form names relies on the response not yet having started. If we are willing to do real streaming before we even get to the submit event dispatch, we also have to deal with the case where we then see it's an invalid submission but the response has already started.

We already handle "error after response already started" when it's an exception - we send a message over the SSR channel about it. Extending this to handle cases that aren't unhandled exceptions makes the flow more complex.

If we choose to do this in the future, the main aspects of the work are something like:

  • Move the call to DispatchEventAsync inside BeginRenderingRootComponent.
    • BeginRenderingRootComponent should continue waiting only until the non-streaming tasks have completed and then return an HtmlRootComponent, but now it should represent the entire rest of the process including event dispatch. That way the upstream code doesn't have to differentiate all these cases.
    • To achieve that, we probably need some new protected API on StaticHtmlRenderer that waits for a supplied task (for which we'll pass the original quiescence task) and then dispatches an event and return an HtmlRootComponent with a QuiescenceTask representing quiescence after that event
    • Then, EndpointHtmlRenderer would return that instead of the original HtmlRootComponent
    • The tricky bit with "invalid submission" remains. The best I can think of is throwing some kind of new "InvalidFormSubmitException" that we then catch at the same level we'd catch NavigationException and convert that into a 400 (if the response hasn't started) or a new streaming SSR message (if it has)

That's not going to happen for preview 7 but we can consider whether it's important for RC1.

Update Great point from @javiercn below about why the user experience is better if we don't change this: #49472 (comment)

@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner July 17, 2023 18:29
@ghost ghost added area-blazor Includes: Blazor, Razor Components labels Jul 17, 2023
MappingScopeName = mappingScopeName;
RestrictToFormName = restrictToFormName;
AcceptMappingScopeName = acceptMappingScopeName;
AcceptFormName = acceptFormName;
Copy link
Member Author

Choose a reason for hiding this comment

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

These unrelated renames are follow-up on code-review feedback from an earlier PR. Just trying to clear up!

@SteveSandersonMS SteveSandersonMS enabled auto-merge (squash) July 17, 2023 18:39
@javiercn
Copy link
Member

So, it's not streaming until the submit event is dispatched. I tried fixing this but it involves more changes than I was ready to do just hours before the CC deadline. One particularly tricky bit is that the handling for returning 400 errors in the case of invalid/missing form names relies on the response not yet having started. If we are willing to do real streaming before we even get to the submit event dispatch, we also have to deal with the case where we then see it's an invalid submission but the response has already started.

I don't think we should do streaming rendering before we dispatch the event. It will result in a very inconsistent experience if you try to alter the response in any way as part of the POST and there is not a ton of value for it. (You are not getting to the page for the first time nor getting new data to consume, you are waiting the results from an action you triggered).

@SteveSandersonMS
Copy link
Member Author

It will result in a very inconsistent experience if you try to alter the response in any way as part of the POST and there is not a ton of value for it. (You are not getting to the page for the first time nor getting new data to consume, you are waiting the results from an action you triggered).

Great point. TBH I don't have a certain sense of there being important scenarios for streaming before dispatching the event, now you've made me think of it more clearly.

Happy to leave it as-is then.

@SteveSandersonMS SteveSandersonMS merged commit c69cb31 into main Jul 17, 2023
@SteveSandersonMS SteveSandersonMS deleted the stevesa/forms-nav-and-error-fixes branch July 17, 2023 19:45
@ghost ghost added this to the 8.0-preview7 milestone Jul 17, 2023
@darena-pjindal
Copy link

Hello - Was the issue reported by Daniel in #49143 fixed in the latest release? I am still having that issue. Is there a workaround for this? The form code below crashes.
@page "/Test"
@Inject NavigationManager NavigationManager

Index

Hello, world!

Welcome to your new app.

Submit and navigate

@code {
void Navigate()
{
Console.WriteLine("Navigate");
NavigationManager.NavigateTo("/counter");
}
}

@ghost
Copy link

ghost commented Feb 4, 2024

Hi @darena-pjindal. 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.

@SteveSandersonMS
Copy link
Member Author

@darena-pjindal Are you sure that code crashes? It works fine for me. Is it possible that it doesn't really crash, but you've got the VS debugger connected and it's telling you about first-chance exceptions instead?

@darena-pjindal
Copy link

Hi @SteveSandersonMS -
I get the following exception and am not able to debug it. It does work if I run without debugging. I need to be able to debug. Is there a setting that I have to choose for VS to ignore specific types of exceptions?

image

@ghost
Copy link

ghost commented Feb 6, 2024

Hi @darena-pjindal. 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.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Feb 6, 2024

Yes, see the checkbox in the screenshot labelled "break when this exception type is user-unhandled"

@darena-pjindal
Copy link

Ah ok. That made it work. Thank you so much.

@ghost
Copy link

ghost commented Feb 6, 2024

Hi @darena-pjindal. 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.

@julioct
Copy link

julioct commented Mar 5, 2024

Hi @SteveSandersonMS Long time! Hey, I'm also getting the NavigationException in VS Code, as part of a tutorial I'm preparing for folks new to Blazor:

image

I don't think it is a good experience for folks new to the platform, and they should not need to mess around with code editor settings to work around such a simple scenario.

Is there anything that can be done to properly handle the exception in the runtime itself?

@siroman
Copy link

siroman commented Mar 5, 2024

are you sure you are not mixing renderModes? i.e. you are trying to NavigateTo from a SSR mode page to an InteractiveServer one?

@julioct
Copy link

julioct commented Mar 5, 2024

are you sure you are not mixing renderModes? i.e. you are trying to NavigateTo from a SSR mode page to an InteractiveServer one?

I'm sure. There's no interactivity enabled for this project. Pure SSR.

@julioct
Copy link

julioct commented Mar 5, 2024

Got it, happens here:

    public void AddGameDetails()
    {
        DB.GameDetails.Add(GameDetails);
        NavigationManager.NavigateTo("/gamedetails");
    }

But does not happen here:

    public async Task AddGameDetails()
    {
        DB.GameDetails.Add(GameDetails);
        NavigationManager.NavigateTo("/gamedetails");
        await Task.CompletedTask;
    }

Created that with the Blazor scaffolder in VS.

In the async version I see this in the debugger logs:

Exception thrown: 'Microsoft.AspNetCore.Components.NavigationException' in Microsoft.AspNetCore.Components.Endpoints.dll

...but the debugger does not break into the exception and the app happily continues to the next page.

@SteveSandersonMS
Copy link
Member Author

Hey @julioct!

Is there anything that can be done to properly handle the exception in the runtime itself?

I'm afraid the exception already is properly handled in the runtime itself. That's why the app "happily continues to the next page".

The issue is just that the debugger is being very proactive about telling you about an internal implementation detail of the framework (this exception). People don't need to know about this, so it's unfortunate it shows up on the log there.

Longer term we might change how the navigation is signalled to avoid this sort of weirdness. Nobody really likes using an exception here, but we did it to recreate the same semantics that happen on other platforms. We may decide that allowing the app code to continue executing even after the redirection is acceptable, and then would not need to use the exception.

@julioct
Copy link

julioct commented Mar 6, 2024

Sounds good, thank you Steve!

@zarna-flywheel
Copy link

Interestingly, if I wrap

 NavigationManager.NavigateTo("/account");
 await Task.CompletedTask;

in a try-catch block, the redirection does not happen (as it hits the catch block).

Any insights on how I can get this to work in a try-catch block correctly?

@SteveSandersonMS
Copy link
Member Author

Any insights on how I can get this to work in a try-catch block correctly?

Don't catch System.Exception (instead, catch the subclass that you're planning to deal with). Or if you must do that, then check if the exception you caught is NavigationException, and if it is, use throw;' to rethrow it.

@mohiyo
Copy link

mohiyo commented May 13, 2024

@SteveSandersonMS I have a Login Form inside a Blazor Server App which is working fine with built-in functionality.
But now I am trying to solve a problem where I will be allowing Users to log in automatically when they reach from certain URL and with certain parameters, Without submitting a form

I tried this in my Login Component

protected override async Task OnInitializedAsync() {
  try {
    if (HttpMethods.IsGet(HttpContext.Request.Method))
      await HttpContext.SignOutAsync();

    if (parameter && condition) {
      await DirectLogin();
    }
  } catch (Exception ex) {
    _logger.LogError(message: ex.Message);
  }
}

async Task DirectLogin() {
  try {
  //Perform some logic here and then SignIn

    await HttpContext.SignInAsync(principal, new AuthenticationProperties {
      ExpiresUtc = DateTimeOffset.UtcNow.AddMinutes(5)
    });

    _navigation.NavigateTo($"/direct/page/{parameter}", true);
    //Here I get this exception "Microsoft.AspNetCore.Components.NavigationException"

  } catch (Exception ex) {
    errorMessage = $ "Error: {ex.Message}";
    _logger.LogError(message: ex.Message);
  }
}

This login Component is in a Static Server Render mode whereas after the successful SignIn, the User will be redirected to an Interactive Server Component.

But Navigation is not working and I am getting this exception

Exception of type 'Microsoft.AspNetCore.Components.NavigationException' was thrown

@javiercn
Copy link
Member

@mohiyo please don't comment on closed PRs.

File a separate issue instead. What's likely happening is that you are breaking on first chance exceptions. NavigationException is thrown by the framework to trigger an SSR navigation, but it's caught internally. If you have a try..catch you should avoid capturing NavigationException.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blazor SSR form post handling: integrate errors with ErrorBoundary
8 participants