96311 Validation Error on Optional Fields When Adding Checklist Item#4543
96311 Validation Error on Optional Fields When Adding Checklist Item#4543AlexStepantsov merged 1 commit intomainfrom
Conversation
WalkthroughEntityPropertyValidator was modified to add an additional condition for string minimum length validation. The check now verifies that MinLength is greater than zero, preventing validation when MinLength is zero or negative. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs (2)
226-232:⚠️ Potential issue | 🟠 MajorOptional fields with
MinLength > 0still fail MinLength validation when value is null/empty.The added
propConfig.MinLength > 0guard fixes theMinLength = 0false-positive, but the PR title describes a broader problem: optional (non-required) fields. For an optional field (Required == false) where the user submits a null/empty value:
- Line 199's required check passes (field is not required).
- Line 226:
stringValueOrEmpty = "","".Length (0) < MinLength (> 0)→ true → validation fails with a MinLength error.Standard validation semantics (e.g., .NET DataAnnotations
[MinLength], FluentValidation) only enforce MinLength when a non-empty value is actually provided. The fix should skip MinLength validation for null/empty values on optional fields.🐛 Proposed fix
- if (propConfig.MinLength.HasValue && propConfig.MinLength > 0 && (value == null || stringValueOrEmpty.Length < propConfig.MinLength)) + if (propConfig.MinLength.HasValue && propConfig.MinLength > 0 && !string.IsNullOrEmpty(stringValueOrEmpty) && stringValueOrEmpty.Length < propConfig.MinLength)This ensures MinLength is only enforced when a non-empty string is actually supplied, letting optional-field null/empty values pass through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs` around lines 226 - 232, In EntityPropertyValidator (the MinLength check block using propConfig.MinLength, stringValueOrEmpty and validationResult), only enforce MinLength when a non-empty value was actually provided: change the guard so the MinLength validation is skipped for null/empty values on optional fields (i.e., require value != null && stringValueOrEmpty.Length > 0 before comparing to propConfig.MinLength) so that optional (Required == false) fields with null/empty values do not trigger a MinLength ValidationResult.
229-230: 🧹 Nitpick | 🔵 TrivialValidation message uses
MinLength - 1phrasing which is non-idiomatic.
"should have value length more than {propConfig.MinLength - 1} symbols"is technically correct but saying "more than N-1" to mean "at least N" is awkward. Prefer the direct form.♻️ Proposed wording
- : $"Property '{friendlyName}' should have value length more than {propConfig.MinLength - 1} symbols")); + : $"Property '{friendlyName}' must have a value of at least {propConfig.MinLength} characters"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs` around lines 229 - 230, The fallback validation message in EntityPropertyValidator.cs uses awkward "more than {propConfig.MinLength - 1}" phrasing; update the ternary fallback (where propConfig.ValidationMessage is used) to a clearer, idiomatic message such as "Property '{friendlyName}' must be at least {propConfig.MinLength} characters long" (referencing propConfig.MinLength and friendlyName) so the intent is direct and readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs`:
- Line 226: In EntityPropertyValidator (around the MinLength check) remove the
redundant value == null clause from the if condition that reads against
propConfig.MinLength and stringValueOrEmpty (stringValueOrEmpty is computed as
value?.ToString() ?? string.Empty), so the condition becomes a plain length
check (propConfig.MinLength.HasValue && propConfig.MinLength > 0 &&
stringValueOrEmpty.Length < propConfig.MinLength); update any related comments
if present.
---
Outside diff comments:
In `@shesha-core/src/Shesha.Framework/Validations/EntityPropertyValidator.cs`:
- Around line 226-232: In EntityPropertyValidator (the MinLength check block
using propConfig.MinLength, stringValueOrEmpty and validationResult), only
enforce MinLength when a non-empty value was actually provided: change the guard
so the MinLength validation is skipped for null/empty values on optional fields
(i.e., require value != null && stringValueOrEmpty.Length > 0 before comparing
to propConfig.MinLength) so that optional (Required == false) fields with
null/empty values do not trigger a MinLength ValidationResult.
- Around line 229-230: The fallback validation message in
EntityPropertyValidator.cs uses awkward "more than {propConfig.MinLength - 1}"
phrasing; update the ternary fallback (where propConfig.ValidationMessage is
used) to a clearer, idiomatic message such as "Property '{friendlyName}' must be
at least {propConfig.MinLength} characters long" (referencing
propConfig.MinLength and friendlyName) so the intent is direct and readable.
1 participant
Summary by CodeRabbit