[Blazor] Take cascading parameter attribute type into account when supplying cascading values

[MackinnonBuck]

[Blazor] Take cascading parameter attribute type into account when supplying cascading values

This PR allows cascading value suppliers to choose not to supply a value to a cascading parameter property depending on the type of the attribute used to annotate the property.

Description

This PR allows [SupplyParameterFromForm], [SupplyParameterFromQuery] and [CascadingParameter] to work indpendently. This prevents, for example, a query parameter being interpreted as a form request entry.

WIP:

• Update [SupplyParameterFromQuery] to work in non-page components
• Support multiple cascading value attributes on a single component parameter
• Add tests

Fixes #46981
Fixes #48770

[javiercn]

We need to think about what happens when you have [SupplyParameterFromQuery] and [SupplyParameterFromFrom] attribute. There are some cases that might be "incompatible", but there are other cases where we might want both attributes to apply. For example, if we do [SupplyParameterFromPrerenderedState]

[MackinnonBuck]

Hm, true. I wonder if it would be fine to modify this code slightly to add a ReflectedCascadingParameterInfo for each attribute on the parameter property. Maybe we could also add something like an Order property to ICascadingParameterAttribute so that, for example, if [SupplyParameterFromQuery] and [SupplyParameterFromForm] are both present, we prioritize supplying a value from a form when possible, but fall back to supplying a parameter from the query if form data is not available.

Is this something you think should be addressed by this PR?

[SteveSandersonMS]

Great points to raise. It would definitely be consistent with other parts of the framework if we had an Order property.

I don't think you should feel required to implement that in this PR but if there's broad agreement about the direction, it would be good to file an issue summarizing the design then we can hopefully get on and implement that soon.

[SteveSandersonMS]

BTW we likely end up with a fairly clear, straightforwards design if:

• CascadingParameterAttribute is unsealed
• ... and gains a new Order property
• ... and we change it to AllowMultiple = true

Then we can cheaply have form/query subclasses in a different assembly, and the core doesn't need to know or care about anything but the base class.

[javiercn]

CascadingParameterAttribute I'd rather not open up the ability for third-parties to create their own definitions of Cascading values by extending/overriding CascadingParameterAttribute.

[SteveSandersonMS]

I tried to address that in #48554 (comment). Can you give more details about what you're concerned about?

[MackinnonBuck]

Then we can cheaply have form/query subclasses in a different assembly, and the core doesn't need to know or care about anything but the base class.

I like this, but wouldn't CascadingModelBinder and CascadingQueryValueProvider still need to know what attribute type was used so that, in the case of only one attribute being present, they don't supply values for each other (or for CascadingValue) by accident? Or would we be okay with, for example, a parameter annotated with [CascadingValue] receiving values from the query or a form request?

[SteveSandersonMS]

I would expect CascadingModelBinder and CascadingQueryValueProvider to be in layers where they are able to know about the corresponding CascadingParameterAttribute subclass and hence can do the same type check that you already have.

[SteveSandersonMS]

I would rather change CascadingParameterAttribute to be nonsealed and thereby not require a marker interface. The forms and query ones can then just inherit it.

This does mean that developers can make their own subclasses but I don't see any reason to object to that. Unless we make ICascadingValueSupplier public, subclassing CascadingParameterAttribute would not allow people to do anything different from just using CascadingParameterAttribute as they already do today.

[javiercn]

This does mean that developers can make their own subclasses but I don't see any reason to object to that.

There's no benefit to it either, is it?

The consequences of doing it that way is that we have created an "infinite set" of attributes that can mean "I am a cascading value", and that's something we'll have to account for in the future.

For example, if we do source generation based on the attribute, now we need to pay the cost of performing semantic analysis over all the parameters, as we need to inspect each attribute and their base class for determining its a target, instead of limiting the scope to the concrete set of attributes we care about.

[SteveSandersonMS]

The benefit is just that this relationship corresponds naturally to inheritance, as we want to share code across these types anyway, and they literally are variations on a core thing. Is there a problem with using Roslyn's semantic model to tell us if a type inherits from another? Seems like that's exactly what it's designed for, and is no different from hundreds of other places in the codebase that allow subclassing.

It's not that I'm hugely fixated on using a subclass here, but I am getting concerned that our codebase is getting increasingly niche.

[javiercn]

It's not that I'm hugely fixated on using a subclass here, but I am getting concerned that our codebase is getting increasingly niche.

Can you clarify what you mean by this?

[SteveSandersonMS]

The specific example here is making a nonobvious design choice that introduces extra concepts (marker interfaces), with a rationale based on a future possibility of optimizing source generation.

[javiercn]

That it's just a practical sample.

The rationale here is opening up a potential extensibility point without a clear benefit to it. Marker interfaces are a well-known concept that is used across our codebase.

The principle in contrast here is minimizing API surface and extensibility points unless there are clear scenarios that users can exercise.

[SteveSandersonMS]

I'd love to reduce the number of implicit, system-created layers in the component hierarchy. Each layer is a nontrivial cost as we've been observing with @DamianEdwards's recent benchmarking (e.g., when we changed to have an App component and a Router).

Not suggesting this should change in this PR, but the approach I have in mind is:

• We define a new mechanism within the cascading parameter lookup flow where, if we're scanning up the hierarchy and don't find any match for a cascading parameter, we also call some new API on the Renderer in case it wants to supply a value.
• We define a new protected virtual API on Renderer so that subclasses can plug in their own set of ICascadingValueSupplier instances for this
• We update EndpointHtmlRenderer and others to have implementations for form/query/etc as applicable based on the renderer type.

This will also let us remove the CascadingModelBinder layer, and means we can add as many new built-in cascaded parameter types as we want without incurring the cost of an extra layer in the component hierarchy for each one.

I'm happy to do this sort of thing as a follow-up.

[javiercn]

I'd love to reduce the number of implicit, system-created layers in the component hierarchy. Each layer is a nontrivial cost as we've been observing with @DamianEdwards's recent benchmarking (e.g., when we changed to have an App component and a Router).

This will also let us remove the CascadingModelBinder layer, and means we can add as many new built-in cascaded parameter types as we want without incurring the cost of an extra layer in the component hierarchy for each one.

I would say that we shouldn't jump to conclusions with regards to perf here without more data. The data we have so far compares two very different pipelines, and we would need data with and without the component to make a proper judgement call. Otherwise, it's premature optimization.

CascadingModelBinder exists because the app must have the ability to influence the context in which form dispatching (and in the future, passing prerendered state from the server to the client), happens.

[SteveSandersonMS]

I would say that we shouldn't jump to conclusions with regards to perf here without more data. The data we have so far compares two very different pipelines, and we would need data with and without the component to make a proper judgement call. Otherwise, it's premature optimization.

Collecting data is great. No objection to that. At the same time it does come at a time cost, and there isn't innately a guarantee that any particular microbenchmark we set up is actually measuring the right thing. In this particular case my sense is that we'd be removing one set of complexities (special hidden built-in components) and adding a different set (a new renderer feature) so there wouldn't necessarily be a net increase in complexity here, while there would definitely be less going on at runtime.

the app must have the ability to influence the context in which form dispatching

Just to be clear, I'm only talking about at the root level. Ideally there would be a built-in default behavior that does the most natural form binding, so we can reduce the number of out-of-box concepts we make people understand. People would only need to add them manually where they want to introduce a scope boundary. Is that what you're already thinking? If not, can we find a way to design the system that way?

[MackinnonBuck]

I'm still working on tests for this, but I've updated the implementation to not rely on separating ICascadingValueSupplier and ICascadingValueSupplierFactory.

[javiercn]

I would have expected this to be done directly within CascadingModelBinder and not as a separate component. CascadingModelBinder in my view, is responsible for all the binding operations related to [SupplyParameterFromXXX]

[MackinnonBuck]

It looks like the big remaining Q's here are:

1. How do we set up the layering to keep web-specific concepts out of Microsoft.AspNetCore.Components?
2. How should we handle cases where multiple cascading value attributes are present on a single property?

And there are some other sub-questions too, like "Should we use a marker interface for cascading parameter attributes or unseal CascadingParameterAttribute?"

I'm going to propose a set of design decisions in this comment that hopefully provides an answer to all those questions, and we can iterate from there as necessary.

Layering:

@SteveSandersonMS proposed in #48554 (comment) an approach where we add a new API on Renderer that allows us to define ICascadingValueSupplier instances that supply values if no other cascading value suppliers in the component hierarchy were able to. I think this is a good approach because it solves multiple problems:

1. It eliminates the "hidden layers" in RouteView.
2. It allows different renderer implementations to configure their own ICascadingValueSuppliers, which means we can keep form model binding code in Microsoft.AspNetCore.Components.Endpoints, for example.

This wouldn't prevent customers from still using <CascadingModelBinder> lower in the component hierarchy if they wish.

Here's what I propose the specific layering looks like:

• Query-related model binding functionality stays in Microsoft.AspNetCore.Components, because existing related APIs (like SupplyParameterFromQuery) live there. Also, query model binding features are applicable both to M.A.C.Endpoints and M.A.C.Web, and we probably wouldn't want to introduce a new package just to share this feature between them.
• CascadingModelBinder stays in M.A.Components. This makes sense if we want a single component to provide all available model-binding functionality rather than have separate components for each model binding source (query strings, form requests).
• Form-related model binding functionality lives in Microsoft.AspNetCore.Components.Endpoints.

We could register one or more services under a new interface (maybe ICascadingModelBindingProvder?), where the specific implementations depend on the scenario. CascadingModelBinder can then get a collection of these services from DI and use them to provide model binding functionality. That way, CascadingModelBinder can live in M.A.Components but have its features get augmented depending on the scenario.

Should we use a marker interface for cascading parameter attributes or unseal CascadingParameterAttribute?

I could go either way here, but I'm leaning toward using the marker interface. In addition to the points made in earlier discussions, subclassing CascadingParameterAttribute actually makes things a bit more awkward for CascadingValue<T>. We only want CascadingValue<T> to supply values for properties annotated with CascadingValueAttribute, not any of its subtypes. But if [SupplyParameterFromQuery] and [SupplyParameterFromForm] are CascadingValueAttributes, we can't just say attribute is CascadingParameterAttribute. We have to do something like attribute.GetType() == typeof(CascadingParameterAttribute), which I think shows that CascadingParameterAttribute can't always be substituted for one of its subtypes.

Handling multiple cascading parameter attributes on a property:

As mentioned in #48554 (comment), we could add an "Order" property to ICascadingParameterAttribute:

• Cascading value suppliers will be tried in order of increasing Order value. This allows parameters to have multiple cascading value suppliers and exhibit consistent value supplier resolution behavior.
• We could consider making this property public in the future, but I think it's fine for it to stay internal for now because it's not clear how often customers would want to configure this.
• Alternative considered: Use the lexical order of attributes on the property to determine priority. I think this is not as ideal because:
  • It doesn't appear to be a guarantee that the lexical attribute order matches the returned attribute order in GetCustomAttributes()
  • This behavior is non-obvious, so it could lead to apparent inconsistencies in the behavior of supplying cascading values

@SteveSandersonMS @javiercn What are your thoughts on this?

[SteveSandersonMS]

@MackinnonBuck That looks like a great analysis and I'm pretty much convinced by the tradeoffs you're proposing.

I do wonder if we can get away without needing to support multiple [SupplyParameterFrom*] on a single property for .NET 8. We're at a stage where we may need to scope out the sort of scenarios that would have motivated doing that anyway. As long as we're able to add this in the future (for example, as long as the marker interface is internal we can add Order to it later, or if it can't be internal then we could instead use a new abstract base class as the "marker interface").

[MackinnonBuck]

Okay, I think this PR is ready to have another look. I believe I've addressed everything except the following:

• Supporting multiple [SupplyParameterFrom*] attributes on a single property (although this should be easily addable later).
• Removing the root CascadingModelBinder in favor of a new Renderer API:
  • This is an optimization that can come later, I think. I've already restructured things to not require adding an additional level of components on top of the existing CascadingModelBinder.

cc @SteveSandersonMS @javiercn

[SteveSandersonMS]

Later on, rather than this being a general DI service with a public API, we might change this into a Renderer API so it's not publicly visible or extensible (except for people implementing a renderer). It potentially leads to a whole bunch of simplifications and reductions in API surface. This is something we discussed before, but I think you're right not to add it to this PR.

I'm totally happy with this PR proceeding as-is, since right now this works and we have other things to focus on for preview 6. But just wanted to raise this so it doesn't seem surprising if I try to move around a bunch of this machinery and possible constrain how much we have to support in preview 7. If you have any concerns about that please let me know!

[SteveSandersonMS]

Nice one, @MackinnonBuck!

There is a lot of implementation change/extension here, probably beyond what I can be confident that I've understood all the implications of. Everything I've read looks reasonable but it is tough to fully visualize if this would lead to any meaningful perf differences or other behavior changes.

I'll be digging into this area in more detail in preview 7 so by that stage this area will have had close attention from @javiercn, me, and you. Hopefully by that point we can all be pretty confident in it :)