Component parameters from querystring

[SteveSandersonMS]

Part of #22388

This PR introduces the [SupplyParameterFromQuery] attribute. The name is chosen not to conflict with [FromQuery] - open to suggestions in API review though! Example:

@page "/search"
@code {
    // Handles things like /search?filter=some+stuff&page=3&assignee=Monica&assignee=Chandler
    [Parameter, SupplyParameterFromQuery] public string Filter { get; set; }
    [Parameter, SupplyParameterFromQuery] public int? Page { get; set; }
    [Parameter, SupplyParameterFromQuery(Name = "assignee")] public string[] Assignees { get; set; }
}

SupplyParameterFromQueryAttribute is the only new public API here because I've made the parameter assignment internal to RouteView. In a sense it would be conceptually nice to add some new dictionary QueryValues to RouteData so that you could consume the mapped query data in other ways, however (1) that's different from the notion of routedata in ASP.NET Core, and (2) it would involve at least allocating and populating an extra dictionary on each navigation to a component that has at least one [SupplyParameterFromQuery], so I've kept it internal to RouteView for now.

What's supported

It will supply values from the querystring to parameters of the following types:

1. All those supported for URL constraints (string, bool, DateTime, decimal, double, float, Guid, int, long)
2. Nullable variants of all those (except string - not applicable)
3. Arrays of all those (nullable and not)

In order to support navigation to a URL that has fewer query parameters than the one you're currently on, the system takes care to supply at least a blank value to overwrite every [SupplyParameterFromQuery] parameter. For array-typed parameters, that's done as Array.Empty<T> rather than null to simplify the developer's component logic.

What's not supported

For perf reasons, I've chosen not to support parameter names that require decoding. It doesn't seem clear that anyone would have a reason to do this, given that ultimately it's going to be mapped to a C# property, which typically have simple names.

If this turns out to be inadequate we can trivially change it, but it means that a hostile user sending a huge query containing nothing but encoded keys would make the server go through the process of decoding all of them, even if there's no corresponding parameter (because we wouldn't know until we decoded it). Each such decoding would cost 2 allocations I think.

Why there's so much code here

The main reasons are:

• I refactored the way that RouteConstraint works so that we can share a single implementation for parsing URL values across both regular route parameters and query parameters
• I optimized QueryParameterValueSupplier so that it does the minimum amount of per-navigation work (building a data structure that quickly maps incoming query pairs to corresponding component parameters), and never allocates unnecessarily. It does have to allocate when actually emitting the final rendertree frames, because component parameter values are typed as object, but it shouldn't allocate anything else during processing except when you're accepting an array and the user supplies more than one value. Value-typed parameters - if they don't need decoding - are parsed directly from the original URL ReadOnlyMemory<char> without being represented as string at any time.

[SteveSandersonMS]

I'm guessing it's well-tested

Hell yeah. So well-tested that the diff is too big to show in the PR by default :)

[javiercn]

Spicy question:

Is there a case where we have a parameter that can come from the route and that also has SupplyParameterFromQuery?

While I get that the behavior might seem like an edge case (specially in Blazor) I think we should define a behavior for it that it is potentially consistent with ASP.NET Core/MVC model binding to avoid surprises.

[SteveSandersonMS]

@javiercn Good question. I missed following up on this yesterday so will do now.

Is there a case where we have a parameter that can come from the route and that also has SupplyParameterFromQuery?

Yes, that's possible. The behavior as of this PR is that we'll supply both values for the same parameter. That is, there will be two entries in the ParameterView matching this parameter name, and it's up to the IComponent's SetParametersAsync to do whatever it wants with them both. ParameterView is not a dictionary and has always been able to contain multiple values for any given parameter name.

The default SetParametersAsync logic in ComponentBase will set them in the order they appear in the ParameterView, which happens to be "route parameters first, then query parameters", so the query parameter value will overwrite the route parameter value. People who want to customize this could implement SetParametersAsync manually, though not many will do.

In that sense it is already in a definite order. However TBH I don't see many cases where it's helpful to lose information by overwriting route values with query values. If we're erring on the risk-averse side here, it would be most risk-averse for us to outlaw this whole scenario by throwing if a component declares query params that clash with route params. Then we could always allow it in the future, but don't necessarily have to.

If we do want to throw in that case, I think we could do it by making QueryParameterValueSupplier directly know about the routing system and ask it for the list of routes matching the target component, and then walk them to see if there are any matching parameter names. It's not the most convenient thing to do but we can do it if we think it's the behavior we want.

@javiercn What are your opinions here?

[javiercn]

I don't have a strong opinion here:

• Matching what ASP.NET does is nice because because it builds on existing knowledge, but I don't think/feel it is required, specially if we have to couple this to the router.
  • Could it be done checking the current set of parameters in the ParameterView to make a decission and avoid searching for that parameter if its present on the existing provided values?
• I'm fine if we decide to leave it as is, it would be nice if we have a test to cover it. I agree with you that in most cases it doesn't add value in the context of Blazor.
• Optionally, if we find a cheap way to throw that doesn't involve coupling with routing (or any other parent that adds parameters for that matter), I would be ok with that too.

Those are my thoughts, however I fully trust you to make the right call here, I just thought it was worth raising it up to make sure we are ok with whatever behavior we choose here.

[SteveSandersonMS]

Could it be done checking the current set of parameters in the ParameterView

Yes, but then it would have to be done during every navigation, whereas if we based it on the declared route parameters for the target component, we could do it just once up front when we already cache the QueryParameterValueSupplier.

I don't personally have any philosophical objection to coupling querystring handling to the notion of RouteAttribute. If someone isn't using our router, then they either don't use [Route] in which case we can't anticipate their goals and it's up to them to implement whatever they want, or they do still use [Route] in which case it's very likely they want the same no-clash rule.

I'm also OK with just leaving this behavior alone because people won't have any reason to put themselves in this situation, and even if they do, it's not like it creates some new security issue (both route values and query values are equally untrusted).

[javiercn]

Wouldn't something like this mean that I can type something invalid on the URL and cause the circuit/app to crash?

[javiercn]

Should we instead consider setting a default value instead of throwing? (Modelbinding does this).

[SteveSandersonMS]

You can indeed cause the circuit (not app) to be terminated by supplying nonparseable data. I did consider setting a default value but that makes it harder for the app developer to notice and understand if they are setting wrongly-formatted parameters (e.g., unsupported date formats).

Since an application would never have reason to provide invalid parameter values to itself, this should only throw if someone is deliberately modifying the URL directly, and so I considered ending the circuit to be a reasonable outcome. This doesn't let someone malicious do anything new, as they could always kill their own circuit with an unhandled exception, e.g., by making a malformed JS interop call.

[javiercn]

That's fair, the only thing that I worry about a bit, is that we don't give the developer any hook to avoid terminating the app, and I worry a bit that you can run into this situation by mistake quite often (like for example when binding the query string to a textbox or similar).

If you've considered those things and are ok, then it's fine. Worst case scenario I suspect you can use the Error boundaries component to prevent the circuit from completely crashing. We can wait until we receive feedback to determine if this is a problem or not, and offer some sort of switch/knob/error handler to allow you to deal with errors in whatever way you find best.

[javiercn]

For perf reasons, I've chosen not to support parameter names that require decoding. It doesn't seem clear that anyone would have a reason to do this, given that ultimately it's going to be mapped to a C# property, which typically have simple names.

What about all the folks that program using non-english identifiers? This would block globalization for many apps that use non-English characters, and that can be problematic since developers might want for those things to appear correctly on the url.

[javiercn]

• I optimized QueryParameterValueSupplier so that it does the minimum amount of per-navigation work (building a data structure that quickly maps incoming query pairs to corresponding component parameters), and never allocates unnecessarily. It does have to allocate when actually emitting the final rendertree frames, because component parameter values are typed as object, but it shouldn't allocate anything else during processing except when you're accepting an array and the user supplies more than one value. Value-typed parameters - if they don't need decoding - are parsed directly from the original URL ReadOnlyMemory<char> without being represented as string at any time.

For future reference (I'm not suggesting we change anything), I'm not sure we made the right trade-off here with regards to perf/simplicity/maintenance. While I agree is best if we avoid allocations, I think that we essentially only read parameters once per navigation, so the perf impact of the binding shouldn't be significant (even if we were to allocate all values into a dictionary and then do the bindings, provided we discarded the original dictionary after the binding was done).

If we are concerned about the scalability/performance characteristics of this scenario when someone spams a large query string with a gazillion values repeatedly via navigations, I would question how is that different from a server receiving those as HTTP Requests, where we are actually making those allocations.

The question is, if its ok for us to do it in a traditional server scenario, what is it different here that requires a different approach?

[SteveSandersonMS]

One difference vs full HTTP request/responses is that navigation is much cheaper to trigger when it’s just a single websocket message vs a complete HTTP request. Someone spamming the server with navigations can do so a lot faster this way and need not spend time reading responses to escape backpressure. Also, all improvements are improvements and maybe we’ll end up similarly improving ASP.NET Core. If not, no loss!

[javiercn]

One difference vs full HTTP request/responses is that navigation is much cheaper to trigger when it’s just a single websocket message vs a complete HTTP request.

That only increments the work by a constant factor, we are not doing more work on the server because you can do this through a websocket. In addition to that, there's not much difference between sending an HTTP request vs sending a message over a websocket, I doubt the HTTP overhead is significant once the connection has been established.

Someone spamming the server with navigations can do so a lot faster this way and need not spend time reading responses to escape backpressure.

An HttpClient or for that matter a TCP doesn't have to wait for the server to answer before sending a new request.

Also, all improvements are improvements and maybe we’ll end up similarly improving ASP.NET Core. If not, no loss!

There is loss, loss in clarity/maintainability. As I mentioned, I'm not suggesting we change anything (what's done is done). I want to make sure we consider these things in the future and we don't jump to more complex solutions without clear data that justifies they are needed.

In this case, my reasoning is that this happens at most once per page navigation and navigations don't happen at a high framerate to be problematic for the user experience. If we think there is a true security concern here, there might be better options here, like offloading all the parsing/binding work to the browser and requiring it to provide the list of parsed values that we need.

[SteveSandersonMS]

OK, I'm sure you're right that we could have got away with a more basic solution. I saw an opportunity to make things really streamlined and took it. This was partly about taking advantage of the ways .NET has evolved and improved since ASP.NET Core's querystring APIs were first defined, and partly in response to the things learned from optimizing Blazor WebAssembly for 5.0 (e.g., hashing arbitrarily long strings being super expensive). But I'm sure you're right that I could have taken a more simplistic route and then waited for evidence that customers were feeling perf pain. In the end this code has been through a bunch of revisions and now TBH it feels pretty straightforward to me now. Especially given the very extensive test coverage, I'm confident that most C# devs could update this and not break it.

So, anyway, thanks for the feedback - I'm not disputing it!

[SteveSandersonMS]

What about all the folks that program using non-english identifiers?

Good point. I've changed it to decode the keys now.

Previously I was thinking that since URL values have been able to include arbitrary unicode characters since about 2010 (except the explicitly unsafe ones like &, =, etc.) that people would just use those arbitrary unicode characters without the percent encoding. However having just checked, I see that current browsers still send percent-encoded characters even if you directly type in the unicode chars in a URL, probably for back-compat with proxies and the like. So yes, you're right that we need to decode in all cases.

[pranavkm]

API review feedback: the hope is to consolidate these attributes across the framework in 7.0. For now this is the best solution we have at hand.

[robertmclaws]

So I know this is already merged, but the word "Query" is ambiguous with data queries, and might be confusing to developers. I tend to prefer names that are wholly unambiguous. SupplyParameterFromQuery makes it sound like you could automatically map it to a LINQ query too.

I believe however that "QueryString" is unambiguous in relation to web programming.

If it is intended to be used like this: [Parameter, SupplyParameterFromQuery] then you might consider something like: [Parameter, ValueFromQueryString] or just [Parameter, QueryString]. The latter would open doors to use the same pattern to pull the parameter value from other sources (like HttpHeaders or Environment).

That would allow a simple find-and-replace in 7.0 to transition to [QueryStringParameter]. [HttpHeaderParameter], or [EnvironmentParameter], which as a developer would be fantastic to code against.

I hope any of this is helpful. Have a great weekend!

[ghost]

Hi @robertmclaws. 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.

[robertmclaws]

@SteveSandersonMS @pranavkm Since the bot said no one would see my above post, I thought I'd tag you for consideration at some point. You're awesome! 👊

[ghost]

Hi @robertmclaws. 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.