Skip to content

Treating empty string in form post as null for nullable value types #52499

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 12, 2025
Merged

Treating empty string in form post as null for nullable value types #52499

merged 3 commits into from
Feb 12, 2025

Conversation

nvmkpk
Copy link
Contributor

@nvmkpk nvmkpk commented Dec 1, 2023 •
edited
Loading

Adding proper parsing of empty string from form post for a nullable field

The changes treat the empty string as null

Description

Form post sends empty string for an empty input field but NullableConverter does not handle that and just uses corresponding non-nullable converter to parse and fails. This PR updates that to convert the empty string to corresponding type's null value.

Fixes #52195

Customer Impact

Without these changes, we cannot define an optional value type field (like DateOnly) when not using any interactive rendering modes.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

Fixes #52195

@nvmkpk nvmkpk requested a review from a team as a code owner December 1, 2023 04:38
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Dec 1, 2023
@ghost
Copy link

ghost commented Dec 1, 2023

Thanks for your PR, @nvmkpk. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@ghost ghost added this to the 8.0.x milestone Dec 1, 2023
@ghost
Copy link

ghost commented Dec 1, 2023

Hi @nvmkpk. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@nvmkpk
Copy link
Contributor Author

nvmkpk commented Dec 1, 2023

@nvmkpk please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@SteveSandersonMS
Copy link
Member

Thanks @nvmkpk!

@javiercn What's your view on the correctness of this?

@javiercn
Copy link
Member

javiercn commented Dec 1, 2023

@SteveSandersonMS have to think about it.

I would look into what MVC does. We should be consistent here. Also, there's no way to differentiate a null value from an empty string if we treat the empty string as null.

@nvmkpk
Copy link
Contributor Author

nvmkpk commented Dec 2, 2023 •
edited
Loading

Also, there's no way to differentiate a null value from an empty string if we treat the empty string as null.

When looked at the form field in the UI, there is no difference between null and empty string right?

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 13, 2023
@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 52499 in repo dotnet/aspnetcore

@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024
@javiercn
Copy link
Member

Can we re-target this PR to main as a first step?

@javiercn javiercn added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Feb 20, 2024
@nvmkpk nvmkpk changed the base branch from release/8.0 to main February 20, 2024 18:55
@nvmkpk
Copy link
Contributor Author

nvmkpk commented Aug 22, 2024

@javiercn, added e2e tests as well. I would like to see this merged before .net 9 release. I just can't wait another year for this.

@nvmkpk nvmkpk mentioned this pull request Aug 22, 2024
Closed
1 task
@nvmkpk
Copy link
Contributor Author

nvmkpk commented Sep 12, 2024

@javiercn, .net 9 rc 1 is out, does that mean this will not be in .net 9?

@nvmkpk
Copy link
Contributor Author

nvmkpk commented Jan 11, 2025

Any update on when this will be merged?

captainsafia
captainsafia approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

I have no opposition to taking this change in in the context of form binding in minimal APIs.

@javiercn Thoughts on the Blazor side?

src/Components/Endpoints/src/FormMapping/Converters/NullableConverter.cs
}
else
{
result = innerResult;
return true;
return TryConvertValue(ref reader, value!, out result!);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using a null suppression here, perhaps we should add [NotNullWhen] annotations to the FormDataReader.TryGetValue method.

@captainsafia captainsafia requested a review from Copilot January 13, 2025 18:30
Copilot
Copilot AI reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

@captainsafia captainsafia mentioned this pull request Jan 14, 2025
Closed
1 task
@javiercn
Copy link
Member

I have no opposition to taking this change in in the context of form binding in minimal APIs.

@javiercn Thoughts on the Blazor side?

We want to take this, but we want to limit it to primitive types, and not just any IParsable, so Type.GetTypeCode(typeof(T)) != TypeCode.Object + DateOnly, TimeOnly, DateTimeOffset

@javiercn
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 12, 2025
@javiercn javiercn merged commit ff827d2 into dotnet:main Feb 12, 2025
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview2 milestone Feb 12, 2025
@javiercn
Copy link
Member

@nvmkpk thanks for the contribution!

@oroztocil oroztocil mentioned this pull request Feb 19, 2025
Merged
@nvmkpk
Copy link
Contributor Author

nvmkpk commented Feb 21, 2025

Thank you @javiercn, will this be available in preview 2 whenever it comes out?

@nvmkpk nvmkpk deleted the release/8.0 branch February 21, 2025 22:37
@sgarnovsky sgarnovsky mentioned this pull request Apr 2, 2025
Open
1 task
@sgarnovsky
Copy link
Contributor

Thank you @javiercn, will this be available in preview 2 whenever it comes out?

Just confirmed the fix is in place in .Net Preview 2 and it works very well. It improves Blazor static SSR form binding.
Thank you to all who worked on this improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@captainsafia captainsafia captainsafia approved these changes

@javiercn javiercn Awaiting requested review from javiercn

Assignees

@javiercn javiercn

Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member
Projects
Servicing
Status: Done
Milestone
10.0-preview2
Development

Successfully merging this pull request may close these issues.

Unable to define an optional DateOnly field in blazor EditForm using InputDate component
6 participants
@nvmkpk @SteveSandersonMS @javiercn @sgarnovsky @captainsafia @wtgodbe