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

[Blazor] Allow properties marked with both [Parameter] and [SupplyParameterFromQuery] to receive values directly #48954

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jun 22, 2023

Allow properties marked with both [Parameter] and [SupplyParameterFromQuery] to receive values directly

Fixes an issue where if a component parameter was marked with both [Parameter] and [SupplyParameterFromQuery], a misleading exception message would be thrown when the parameter's value was set directly.

Description

Since #48554 was merged, marking a component parameter with both [Parameter] and [SupplyParameterFromQuery] would cause the following exception to be thrown if the parameter was set directly:

Object of type 'MyComponent' has a property matching the name 'MyParameter', but it does not have [ParameterAttribute] applied.

This error is misleading, because the parameter does have ParameterAttribute applied. The existing parameter setting logic had assumed that if a component's property was marked with a cascading parameter attribute, it must not have also had the [Parameter] attribute applied, hence the misleading error message. Since [SupplyParameterFromQuery] recently became a cascading parameter attribute, it inherited that behavior.

Solution

This PR fixes the issue by allowing [Parameter] and [SupplyParameterFromQuery] to both exist on a single property without causing an exception to be thrown. If both direct and cascading parameter values are supplied, the direct parameter value is preferred.

Fixes #48937

@MackinnonBuck MackinnonBuck added the area-blazor Includes: Blazor, Razor Components label Jun 22, 2023
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner June 22, 2023 00:05
Comment on lines 66 to 73
else if (parameter.Cascading && writer.AcceptsDirectParameters && writer.AcceptsCascadingParameters)
{
// It's possible that the writer accepts both cascading and direct parameters. In that case,
// we want to prefer the direct parameter, if it exists.
if (parameters.HasDirectParameter(parameterName))
{
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is way more general behavior than what I expected.

In my view, we should just see where we are throwing the exception and special case it for supply from query parameters. We are doing this for back compat, we are not trying to create a new language/idiom.

Copy link
Member

@SteveSandersonMS SteveSandersonMS Jun 22, 2023

Choose a reason for hiding this comment

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

I agree. If we are able to limit this to just backcompat, I think we'll end up in a better place where we don't have the nonobviousness of needing priority rules to pick between cascading and direct parameters. I could be missing something, but perhaps the only change we need is:

  • When a direct parameter arrives and matches a [Parameter] property, we continue to check if there's also [SomeCascadingParameter] on the target property. If there is:
    • New behavior: If it's [SupplyParameterFromQuery], we allow it and prefer the direct value for back-compat
    • Otherwise we throw like we did before

That might be the only case we have to deal with. It would mean that:

  • [Parameter] [SupplyParameterFromQuery] would accept both direct and cascaded values, preferring direct. We don't actually document this because it's legacy back-compat and we would only tell people to use [SupplyParameterFromQuery] on its own.
  • [Parameter] [SomeOtherCascadedParameter] would never be useful, because it would throw if a direct parameter was received
  • [AnyCascadedParameter] on its own, including [SupplyParameterFromQuery], could only receive cascaded values for the same reason (if a direct value was received at runtime, it would throw)

Does that sound right?

Copy link
Member Author

@MackinnonBuck MackinnonBuck Jun 26, 2023

Choose a reason for hiding this comment

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

That analysis seems right to me. My initial reasoning for not limiting the fix to just [SupplyParameterFromQuery] was that it felt weird to have something so special-cased in the core parameter setting logic. But I do acknowledge that it's safer to not introduce fundamental new behavior in that area.

When a direct parameter arrives and matches a [Parameter] property, we continue to check if there's also [SomeCascadingParameter] on the target property. If there is...

Yep, this logic seems right to me and is equivalent to what I've implemented (AFAICT), minus the part about checking the existence of [SupplyParameterFromQuery] specifically. In my commit addressing this feedback, the core parameter setting code (ComponentProperites.SetProperties()) is still attribute-agnostic, but I made a one-line change to WritersForType so that properties can only receive direct parameters if they don't have a CascadingParameterAttribute, unless that attribute is a SupplyParameterFromQueryAttribute.

@MackinnonBuck MackinnonBuck changed the title [Blazor] Allow parameters to be both cascading and non-cascading [Blazor] Allow properties marked with both [Parameter] and [SupplyParameterFromQuery] to receive values directly Jun 26, 2023
@MackinnonBuck MackinnonBuck merged commit ae7c7ae into main Jun 26, 2023
@MackinnonBuck MackinnonBuck deleted the mbuck/fix-cascading-parameters branch June 26, 2023 20:24
@ghost ghost added this to the 8.0-preview7 milestone Jun 26, 2023
MackinnonBuck added a commit that referenced this pull request Jun 26, 2023
…ParameterFromQuery]` to receive values directly (#48954)
wtgodbe pushed a commit that referenced this pull request Jun 26, 2023
…ParameterFromQuery]` to receive values directly (#48954) (#49038)
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
3 participants