jwaliszko / ExpressiveAnnotations

Annotation-based conditional validation library.
MIT License
351 stars 123 forks source link

RequiredIf with Enum not working #142

Closed nomorechickennuggets closed 7 years ago

nomorechickennuggets commented 7 years ago

In the below example:

[RequiredIf("WithholdingType == WithholdingType.FederalRateSchedule")]

For some reason we're seeing the expression evaluate to true no matter what the Enum is, and no matter what the operation is (!=, ==). What might cause this behavior? There is no [Required] - only [RequiredIf].

jwaliszko commented 7 years ago

The expression you've shown is not allowed from the client-side perspective. You're supposed to see the following exception:

Naming collisions cannot be accepted by client-side - WithholdingType part at level 0 is ambiguous.

I don't know why you do not see it, mabye you have missed it some log file?

Such an exception is thrown (is should be at least) because your field name is exactly the same as your enumeration type name. Normally at client side, based on the data sent from server, model object is constructed to satisfy JavsScript expression evaluation. Unfortunately an ambiguity appears in your situation (what should be the value of WithholdingType property in such an object) so exception is thrown.

Examples:

A.B.C = 0    {"A":{"B":{"C":0}}}
A.D = true   {"A":{"D":true}}
can be merged into: {"A":{"B":{"C":0},"D":true}}
A.B.C = 0    {"A":{"B":{"C":0}}}
A.B = true   {"A":{"B":true}}
cannot be merged at 1st level - B object would be overwritten

Notice that if client validation is disabled, namely validators are not registered, everything works because server logic has much more context to handle such a situations properly.

If you fix this (e.g. just change the property name), there is one thing you need to be aware of - mainly how your WithholdingType value is presented in HTML. By default, HTML helpers will render input field, value of which is given by the string representation of the enum value (e.g. FederalRateSchedule), while EA requires it to be represented by its numeric equivalent (e.g. 0).

So, instead of writing @Html.HiddenFor(model => model.WithholdingType);, which renders to: <input id="WithholdingType" ... value="FederalRateSchedule">

write @Html.Hidden("WithholdingType", (int)Model.WithholdingType);, which renders to: <input id="WithholdingType" ... value="0">

jwaliszko commented 7 years ago

This comment is probably not related directly with your error, but with the commit https://github.com/jwaliszko/ExpressiveAnnotations/commit/12fb229c9d59fd7a640d7a317b2b73a44ceaa698, I've added possibility to setup of how enum values are internally processed: as integral numerics or string identifiers (see enumsAsNumbers settings flag in the script). Setting should be consistent with the way of how input fields values are stored in HTML, e.g. @Html.EditorFor(m => m.EnumValue) renders to string identifier by default, in contrast to explicit @Html.EditorFor("EnumValue", (int)Model.EnumValue) statement. The flag setup should reflect that behavior to have the internal JS model context deserialized accordingly.

Suppose you've such an expression:

public enum EnumType { First, Second }

[AssertThat('EnumValue == EnumType.First')]
public EnumType EnumValue => EnumType.Second;

Correct approach - choose out of the following options (use one or another and stay consistent):

Incorrect approach - lack of consistency, e.g.

It will be shipped with the next release unless any alternative proposal is given.

jwaliszko commented 7 years ago

Referenced: #10

nomorechickennuggets commented 7 years ago

Thank you very much for this.