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

Route parameters with non-string values fails #48866

Closed
1 task done
Markz878 opened this issue Jun 17, 2023 · 7 comments · Fixed by #49083
Closed
1 task done

Route parameters with non-string values fails #48866

Markz878 opened this issue Jun 17, 2023 · 7 comments · Fixed by #49083
Assignees
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing
Milestone

Comments

@Markz878
Copy link

Markz878 commented Jun 17, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

In .NET 8 preview 5, when creating a SSR Blazor app and using route parameters with e.g. int type, the page fails to load with an error: InvalidCastException: Unable to cast object of type 'System.String' to type 'System.Int32'.

Here is the component I used to test this:

@page "/counter/{currentCount:int?}"
<p role="status">Current count: @CurrentCount</p>
@code {
    [Parameter] public int CurrentCount { get; set; }
}

I tried this on .NET 7 Blazor WASM app and it worked.

Expected Behavior

When going to an url https://localhost:7208/counter/5 the CurrentCount property should be set to 5.

Steps To Reproduce

Create a .NET 8 Blazor SSR app, copy the code to a Counter.razor component and navigate to it's url.

.NET Version

8.0.100-preview.5.23303.2

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Jun 17, 2023
@javiercn
Copy link
Member

@Markz878 thanks for contacting us.

Yes, this seems to be a bug (more of an integration issue).

@surayya-MS @SteveSandersonMS this is very likely a difference between asp.net core routing and Blazor routing.

Blazor will use the constraints to cast the value to the proper type, while ASP.NET Core won't do so.

I suggest two possible action plans:

  • Process the parameters inside Router to convert the string to the appropriate type.
  • Update RouteView to do this casting instead (since it knows the parameter types).
  • Convert route values into cascading parameters and implement [SupplyParameterFromRoute].

@SteveSandersonMS
Copy link
Member

Most likely we'd have to do it in RouteView since Router wouldn't know anything about the types. It doesn't know the original route template and its constraints, nor the declared parameter types on the target component.

It would still technically be breaking because the following component works in .NET 7 but would start throwing:

@page "/counter/{currentCount:int?}"

<p role="status">Current count: @_currentCount</p>
<button class="btn btn-primary" @onclick="IncrementCount">Click me</button>

@code {
    int _currentCount;

    public override Task SetParametersAsync(ParameterView parameters)
    {
        _currentCount = parameters.GetValueOrDefault<int>("currentCount");
        return base.SetParametersAsync(ParameterView.Empty);
    }

    private void IncrementCount() => _currentCount++;
}

If we want real back-compat we would just drop IRoutingStateProvider and go back to the mechanism where <Router/> makes its own decisions independently.

I'm not against requiring page components to use [Parameter] declarations. However given that it's another nontrivial task to do the parameter conversions inside RouteView and still wouldn't be fully back-compatible, it is quite appealing to take the simpler approach.

@javiercn Besides the perf optimization, are there functional requirements to have IRoutingStateProvider?

@javiercn
Copy link
Member

@SteveSandersonMS there are going to be inconsistencies with server-side routing and its going to block all the server-side routing features ASP.NET Core provides.

I would suggest we make the change to align both routers in this case. It's a bit of an oddity that the Blazor router chose to cast the value for the route, even more when it's not saving an allocation, as it gets boxed anyway.

I would suggest we move the code in there to RouteView and process the values there instead. If we care a lot about backcompat, let's add a flag to revert to the old behavior, but I think we shouldn't.

https://github.com/dotnet/aspnetcore/blob/9178b9b117e64ca75821db1927e4857714c5da97/src/Components/Components/src/Routing/RouteConstraint.cs

@SteveSandersonMS
Copy link
Member

there are going to be inconsistencies with server-side routing and its going to block all the server-side routing features ASP.NET Core provides.

Can you give examples? We absolutely can't have inconsistencies in page or parameter selection, because interactive Router components must render the same page with the same parameters as were previously rendered during the initial page load.

If we do think there's a risk of unexpected inconsistencies, that seems like a further reason to prefer Router making its own decisions independently, since we would only need the ASP.NET Core router to select RazorComponentEndpoint, and we wouldn't be relying on it to have picked the same page component or the same parameters. Of course, if the two systems disagree about whether a given URL is a Blazor component then the situation would be irrecoverable either way.

@javiercn
Copy link
Member

@SteveSandersonMS some examples:

#45400
#45401
#45399

@SteveSandersonMS
Copy link
Member

Thanks for pointing those out. It seems like the tradeoff is between:

  • If we have IRoutingStateProvider then people can use server-routing-specific features but only if they never try to make their Router interactive or it will blow up (and as a smaller matter, we have to fix the issue with type conversions)
  • Or, if we don't, then no such problem or inconsistency exists (and as a smaller matter, the type conversion issue disappears), but people can't use server-routing-specific features

Given the amount of uncertainty we're trying to manage as we head quickly towards the .NET 8 release, I tend to err on the side of constraining scope and maximizing correctness. When there isn't strong pressure to add a feature now, and we know we can't really deliver it fully, it's reassuring to scope it out. Would it be reasonable to add an IRoutingStateProvider-style pattern in the future, but only once we've aligned the two routing systems more completely?

@surayya-MS surayya-MS assigned surayya-MS and unassigned surayya-MS Jun 19, 2023
@mkArtakMSFT mkArtakMSFT added feature-routing bug This issue describes a behavior which is not expected - a bug. labels Jun 21, 2023
@mkArtakMSFT mkArtakMSFT added this to the 8.0-preview7 milestone Jun 21, 2023
@mkArtakMSFT
Copy link
Member

We've discussed this offline and decided to go with the following approach.
As a short-term mitigation we will make so that the client router will ignore the server router logic completely for now.

In parallel, we will see if we can bring the missing features to the routing system, so that this scenario works properly. If we manage to do that before rc2 and we are happy with the approach, we will take that and remove the temporary fix. Otherwise, we will come up with the proper fix in .NET 9, leaving the simple workaround in place in .NET 8.

//cc @SteveSandersonMS , @javiercn, @surayya-MS

@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed bug This issue describes a behavior which is not expected - a bug. labels Jun 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants