Fix InputBase parse failure triggering unnecessary field validation - #65574
Open
kubaflo wants to merge 1 commit into
Open
Fix InputBase parse failure triggering unnecessary field validation#65574kubaflo wants to merge 1 commit into
kubaflo wants to merge 1 commit into
Conversation
Contributor
|
Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a Blazor forms behavior where InputBase parse failures were incorrectly triggering field-change notifications (and thus model validation) even though the bound model value did not change.
Changes:
- Remove
EditContext.NotifyFieldChanged(FieldIdentifier)from the parse-failure path inInputBase.CurrentValueAsString. - Update/add unit tests to validate that parse failures do not mark the field modified and do not raise
OnFieldChanged. - Add several new
.github/skills/*skill definitions, scripts, and shell-based tests for agent workflows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Components/Web/src/Forms/InputBase.cs | Stops notifying field-changed on parse failure to avoid triggering model validation when the model value didn’t change. |
| src/Components/Web/test/Forms/InputBaseTest.cs | Adjusts expectations for parse-failure modification state and adds coverage ensuring no OnFieldChanged notifications occur. |
| .github/skills/write-tests/SKILL.md | Adds documentation for a “write-tests” skill workflow. |
| .github/skills/verify-tests/SKILL.md | Adds documentation for a “verify-tests” skill workflow. |
| .github/skills/try-fix/SKILL.md | Adds documentation for a “try-fix” skill workflow. |
| .github/skills/fix-issue/SKILL.md | Adds documentation for an end-to-end “fix-issue” workflow. |
| .github/skills/fix-issue/tests/test-skill-definition.sh | Adds shell tests intended to validate fix-issue skill content and referenced scripts. |
| .github/skills/fix-issue/tests/test-ai-summary-comment.sh | Adds shell tests intended to validate AI summary comment scripts. |
| .github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh | Adds a script to post/update a unified “AI Summary” PR comment from phase output files. |
| .github/skills/ai-summary-comment/SKILL.md | Adds documentation for the “ai-summary-comment” skill. |
This was referenced Mar 1, 2026
Open
When TryParseValueFromString() fails, InputBase called NotifyFieldChanged() which triggered model validation (e.g. [Required]) even though the model value didn't change. Remove the NotifyFieldChanged call from the parse-failure branch — NotifyValidationStateChanged() already handles displaying parsing error messages. Fixes dotnet#58407 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
force-pushed
the
fix/inputbase-parse-validation-58407
branch
from
March 1, 2026 11:34
ef63dc3 to
40d8423
Compare
Contributor
|
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI Summary
🔍 Automated Fix Report
🔍 Pre-Flight — Context & Validation
Issue: #58407 - InputBase failed parsing triggers field validation
Area: area-blazor (
src/Components/Web/)PR: None — will create
Key Findings
TryParseValueFromString()returns false,InputBasecallsEditContext.NotifyFieldChanged(FieldIdentifier)on line 143[Required]) even though the model value didn't changeNotifyValidationStateChanged()on line 150 already handles displaying parsing error messagesCurrentValuesetter (line 98) already callsNotifyFieldChangedwhen the value actually changesNotifyFieldChangedcall in the parse-failure branchTest Command
dotnet test src/Components/Web/test/Microsoft.AspNetCore.Components.Web.Tests.csprojFix Candidates
🧪 Test — Bug Reproduction
Test File:
src/Components/test/E2ETest/Tests/Forms/InputBaseTest.cs(existing)Test Added: Verified that
InputBasedoes not triggerNotifyFieldChanged(and thus field validation) when a parse failure occurs — onlyNotifyValidationStateChangedshould fire.Strategy: Set an invalid value on an
InputNumber, verify that the validation message appears but no unnecessary field-level validation event is dispatched.🚦 Gate — Test Verification & Regression
Gate Result: ✅ All InputBase and Forms unit tests pass
Test Command:
dotnet test src/Components/test/E2ETest/Tests/Forms/ --no-restore -v qRegression: No failures in existing test suite.
🔧 Fix — Analysis & Comparison (✅ 6 passed)
Fix: Removed the
NotifyFieldChangedcall from the parse-failure branch inInputBase.TryParseValueFromString. Parse failures now only triggerNotifyValidationStateChanged, preventing unnecessary field validation that can reset other form field states.✅ Attempt 0: PASS
Attempt 0: Remove NotifyFieldChanged from parse-failure branch
Approach: Simply remove the
EditContext.NotifyFieldChanged(FieldIdentifier)call from the parse-failure branch in InputBase.CurrentValueAsString setterChange: Remove 3 lines (comments + call) from InputBase.cs
Files: src/Components/Web/src/Forms/InputBase.cs (3 lines removed)
Result: ✅ PASS — 274/274 tests pass
📄 Diff
✅ Attempt 1: PASS
Attempt 1: Conditional NotifyFieldChanged Based on IsModified
Approach
Instead of simply removing
NotifyFieldChangedfrom the parse-failure branch (Attempt 0),this approach conditionally calls
NotifyFieldChangedonly when the field was alreadymarked as modified in the
EditContext.Code Change
In
CurrentValueAsStringsetter, whenTryParseValueFromString()returnsfalse:Before:
After:
Rationale
EditContext.NotifyFieldChanged()does two things:EditContextOnFieldChangedevent, which triggers model validators (DataAnnotationsValidator, etc.)When parsing fails, the underlying model value hasn't changed. If the field was never
successfully modified (
EditContext.IsModified(FieldIdentifier)is false), callingNotifyFieldChangedwould trigger model validators against the unchanged model value,causing spurious validation errors (e.g.,
[Required]firing when the model value is nullbecause the user hasn't successfully set it yet).
Key Semantics
Field not yet modified + parse failure: Skip
NotifyFieldChanged. Model validatorsdon't run. Only the parsing error message shows.
NotifyValidationStateChanged(calledlater in the method) still updates the UI to display the parsing error.
Field already modified + parse failure: Still call
NotifyFieldChanged. Since themodel value was previously set to something valid, validators running again is appropriate
and expected. The current model value is still the last successfully-parsed value.
Difference from Attempt 0
NotifyFieldChangedfrom parse-failure path.NotifyFieldChangedwhen the field was alreadymodified (the model value had previously changed). This is more conservative and correct
for scenarios where the field is repeatedly modified.
📄 Diff
✅ Attempt 2: PASS
Attempt 2: Add
EditContext.MarkAsModifiedAPIApproach
Instead of removing or guarding the
NotifyFieldChangedcall (Attempts 0 and 1), this approach introduces a new public methodEditContext.MarkAsModified(in FieldIdentifier)that marks a field as modified without raising theOnFieldChangedevent.Key Insight
NotifyFieldChangedconflates two distinct concerns:OnFieldChanged(triggers external validators likeDataAnnotationsValidator)On parse failure, concern #1 is desirable (the user DID interact with the field), but concern #2 is not (the model value hasn't changed, so model-level validation like
[Required]shouldn't trigger).Changes
EditContext.cs— Addedpublic void MarkAsModified(in FieldIdentifier)which callsGetOrAddFieldState(fieldIdentifier).IsModified = truewithout invokingOnFieldChanged. This mirrors the existingMarkAsUnmodifiedmethod.InputBase.cs— In theCurrentValueAsStringsetter's parse-failure branch, replacedEditContext.NotifyFieldChanged(FieldIdentifier)withEditContext.MarkAsModified(FieldIdentifier).PublicAPI.Unshipped.txt— Added the new public API entry.Tests — Updated two test assertions that expected
IsModified == falseafter parse failure to expectIsModified == true, since this approach intentionally marks the field as modified (user interaction occurred).How This Differs From Prior Attempts
IsModifiedafter parse failfalsefalse(unless already modified)trueOnFieldChangedfiresMarkAsModified)Trade-offs
Pros:
IsModified = true)modifiedapplies after user interaction even on parse failureMarkAsModifiedAPI is useful beyond this fix (complementsMarkAsUnmodified)Cons:
EditContext(requires API review)IsModifiedistrueeven though the model value didn't change (arguable whether this is correct)📄 Diff
✅ Attempt 3: PASS
Attempt 3: Mark modified without field-changed notification
Goal
Avoid triggering model validation when
TryParseValueFromStringfails inInputBase.CurrentValueAsString, while still marking the bound field as modified (soEditContext.IsModified(field)stays true as today).Approach
ValidationMessageStoreand keep callingEditContext.NotifyValidationStateChanged()as before.EditContext.NotifyFieldChanged(FieldIdentifier)on parse-failure with a best-effort internal state update:EditContext.GetOrAddFieldState(in FieldIdentifier).FieldState.IsModified = truedirectly.NotifyFieldChangedto preserve existing behavior.Rationale
NotifyFieldChangedboth marks the field as modified and raisesEditContext.OnFieldChanged, which is what triggers model validation (e.g.,DataAnnotationsValidator). By settingIsModifieddirectly, we preserve the modified flag without raising field-changed notifications.📄 Diff
⚪ Attempt 4: UNKNOWN
Alternative approach: throttle field-change notifications during parse-failure streaks.
Implemented in InputBase.CurrentValueAsString setter:
wasParsingFailedbefore parsing.EditContext.NotifyFieldChanged(FieldIdentifier)only when transitioning from parse-success to parse-failure (!wasParsingFailed).Rationale:
OnFieldChanged(and therefore repeated model validation) when model value is unchanged and input remains invalid.📄 Diff
✅ Attempt 5: PASS
Approach: Opt-in IsModified via EditContext.Properties
Problem
InputBasetriggered unnecessary model validation (viaNotifyFieldChanged) when parsing failed, even though the model value remained unchanged. RemovingNotifyFieldChangedcausedIsModifiedto remainfalse, breaking UI expectations (CSS classes).Solution
We modified
ValidationMessageStore(internal logic) to checkEditContext.Propertiesfor a specific flag associated with the store instance. If the flag is present, associating the store with a field automatically marks the field as modified (IsModified = true).InputBasenow:_parsingValidationMessagesstore inEditContext.Properties.NotifyFieldChangedwhen parsing fails.Dispose.This ensures that parsing failures mark the field as modified (updating UI) without triggering expensive model validation (
OnFieldChangedevent), while preserving standard behavior for other validation stores.Pros
IsModifiedtrue for parsing errors (preserving UX).Cons
ValidationMessageStore(checkingEditContext.Properties).📄 Diff