Fix OptionsValidator source-gen to skip static and const members #88254
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-options Issue DetailsFix #88150
|
src/libraries/Microsoft.Extensions.Options/Microsoft.Extensions.Options.sln
Outdated
Show resolved
Hide resolved
| @@ -462,7 +476,7 @@ private List<ValidatedMember> GetMembersToValidate(ITypeSymbol modelType, bool s | |||
| } | |||
|
|
|||
| // generate a warning if the field/property seems like it should be enumerated | |||
| if (enumerationValidatorTypeName == null && speculate) | |||
| if (enumerationValidatorTypeName == null && speculate && memberType.SpecialType != SpecialType.System_String) | |||
There was a problem hiding this comment.
We don't check string internals anymore
| @@ -59,7 +69,7 @@ | |||
| </trans-unit> | |||
| <trans-unit id="MemberIsInaccessibleTitle"> | |||
| <source>Can't validate private fields or properties.</source> | |||
| <target state="translated">Не удается проверить частные поля или свойства.</target> | |||
| <target state="translated">Не удается проверить приватные поля или свойства.</target> | |||
There was a problem hiding this comment.
This fixes an incorrect translation.
|
|
||
| if (member.IsStatic) | ||
| { | ||
| // generate an error if the member is static and has a validation attribute applied |
There was a problem hiding this comment.
We can change this to warning, or completely ignore these cases. But it's better to notify a user about incorrect usage and that these members won't be validated.
There was a problem hiding this comment.
Would be good if we check what is the runtime behavior when having the validation attribute on the statics.
There was a problem hiding this comment.
using a validation framework like ASP.NET MVC or ASP.NET Core, the RequiredAttribute alone would not automatically trigger validation for static properties. These frameworks primarily focus on validating instance properties of classes that are involved in HTTP request processing.
Considering that, I would say making it as warning will make sense.
@jeffhandley @geeknoid what do you think about that?
There was a problem hiding this comment.
Turns out it was a warning already (bad copy-paste). Added an additional assertion in tests to ensure that
|
Looks you missed adding one resources |
And that's strange, because you can see I added it into |
|
The error is coming from the project |
ghost
locked as resolved and limited conversation to collaborators
Fix #88150.
Also, performance of the generator was improved - now it doesn't check all members of
stringtype.