Skip to content

The new FormInputRenderMode option adds unnecessary noise to query strings #47593

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

Closed
1 task done
dougwaldron opened this issue Apr 6, 2023 · 5 comments
Closed
1 task done
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. Status: No Recent Activity
Milestone

Comments

@dougwaldron
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

After upgrading my app to use ASP.NET 7, my search forms now produce query strings that contain additional (unnecessary?) parameters.

The Problem

The search forms on my site use the GET method so that searches can be bookmarked & shared. (Google does the same thing.) So a typical search result URL might look like:

https://localhost/Public?SearchDate=2023-04-06

But because of the work done in #43182, the HTML now contains additional hidden input fields to inform the parser about "which inputs need culture-invariant parsing", and now the same search form produces this URL:

https://localhost/Public?SearchDate=2023-04-06&__Invariant=Spec.SearchDate

... which isn't terrible, but consider that my search forms might contain a dozen fields, several of which get similarly doubled.

Code used for this example...
<input asp-for="Spec.SearchDate" name="@nameof(Model.Spec.SearchDate)" />

The input element includes a name attribute because I use a DTO instead of putting all the search fields directly in the page model:

// Search form DTO
public class SearchDto
{
    [DataType(DataType.Date)]
    public DateTime? SearchDate { get; init; }

    // ...
}

// Page model
public class IndexModel : PageModel
{
    public SearchDto Spec { get; set; } = default!;

    // ...
}

Concerns

The new URL is unnecessarily cluttered. This is an aesthetic issue, but aesthetics is important. It adds value when the user (and programmer!) can understand the URL. Also, creating a new search by editing the URL is more cumbersome and intimidating, and possibly more error prone.

I'm also concerned about how this will affect existing bookmarks. My guess is that some old bookmarks may have been affected by the bug that was fixed, so it's hard to say how they will work under the new behavior.

Finally -- and this is a secondary issue -- but why can't the new parameter at least use the shorter name SearchDate I specified instead of Spec.SearchDate?

Based on my limited testing, removing the __Invariant parameters from the search string didn't affect how the values were parsed. It seems to me that since FormInputRenderMode is an all or nothing setting, you could just determine how to parse all dates and numbers based on that setting, rather than get an extra parameter for each date & number field.

Expected Behavior

Cool URIs don't change 😉

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

7.0.202

Anything else?

No response

@ghost ghost added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Apr 6, 2023
@MackinnonBuck
Copy link
Member

MackinnonBuck commented Apr 11, 2023

You can turn this feature off by setting HtmlHelperOptions.FormInputRenderMode to FormInputRenderMode.AlwaysUseCurrentCulture. Something like:

var builder = services.AddMvc()
    .AddViewOptions(options =>
    {
        options.HtmlHelperOptions.FormInputRenderMode = FormInputRenderMode.AlwaysUseCurrentCulture,
    });

you could just determine how to parse all dates and numbers based on that setting, rather than get an extra parameter for each date & number field.

The challenge is that without a mechanism similar to what we implemented, the server does not have insight into how each field got formatted in the form request. One could have a model containing a double property, but the format of its corresponding entry in the form request will vary depending on the type of input that generated the value.

For example, the user might type the double value in a text input like "2,5" (depending on the culture, of course), and it will be submitted as "2,5" in the form request. But the same value typed into a number input would result in the value "2.5" being submitted. Regardless of which format the server choses to parse in, it will sometimes be wrong.

@dougwaldron
Copy link
Contributor Author

Thanks a lot for looking into this! I feel like I must still be missing something, though.

I added a decimal property to a page for testing. I changed language settings for both my browser and OS (independently) to a language that uses a comma for decimals (fr-FR) and one that uses a period (en-US). I tried both settings for FormInputRenderMode. I added form inputs both with and without type="number". And I also tried editing the URL directly to change the number format and to add/remove the __Invariant=... parts of the query string.

In no circumstance was a number with a comma interpreted correctly as a decimal, but in all circumstances, a number with a period was interpreted correctly. So I fail to see how this new behavior is helping here. I'd love to see the steps required that would enable the use of a number formatted with a comma!

As far as dates go, the default behavior is already to format the date as invariant unless the developer overrides it as type="text" in which case it's now the developer's job to worry about parsing.

@MackinnonBuck
Copy link
Member

I changed language settings for both my browser and OS (independently) to a language that uses a comma for decimals (fr-FR) and one that uses a period (en-US)...

Do you have localization middleware configured? See https://learn.microsoft.com/aspnet/core/fundamentals/localization/select-language-culture?view=aspnetcore-7.0#configure-localization-middleware

@MackinnonBuck MackinnonBuck added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Apr 13, 2023
@MackinnonBuck
Copy link
Member

We're going to backlog this for now because addressing it would likely require making a breaking change and adding a new feature to the framework to make this behavior opt-in. We'll come back to this if we get enough feedback from the community that this is important to address. Thanks!

@MackinnonBuck MackinnonBuck added this to the Backlog milestone Apr 17, 2023
@ghost
Copy link

ghost commented Apr 21, 2023

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Apr 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 24, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. Status: No Recent Activity
Projects
None yet
Development

No branches or pull requests

2 participants