jwaliszko / ExpressiveAnnotations

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

Naming property Length and using IsNumber isn't valid #167

Closed fehrm closed 7 years ago

fehrm commented 7 years ago

If I name a property Length, and then use IsNumber inside AssertThat on it, like this: https://github.com/fehrm/LengthEA/blob/d9839bdb3e70723c236b9cbadec97680348c4853/LengthEA/Models/TestViewModel.cs#L11-L12

[AssertThat("IsNumber(Length)")]
public string Length { get; set; }

It doesn't validate it correctly in the clientvalidation when validated. If I rename the property to eg. LengthT it works.

I added a sample project to my github, https://github.com/fehrm/LengthEA/ with a very simple testcase for it. It behaves the same if I don't use input type number.

This is using EA 2.9.2, jQuery valid 1.16.0 etc. Here is a link to the packages.config

jwaliszko commented 7 years ago

Hi,

The problem is due to the Length property being overridden by a built-in method of the same name, when model context (within which expression is to be evaluated) is being constructed. Proposed fix here: https://github.com/jwaliszko/ExpressiveAnnotations/commit/f39e43157f56d7283327aef468ce2f4a8a5a046d.

Thanks for raising this defect.

fehrm commented 7 years ago

I agree that it is not an optimal name 😄 I think the proposed change is great though, it will enable the developer to be informed and change the name to a more suitable one. As I did for my project.

Thanks for the prompt reply!

jwaliszko commented 7 years ago

The name is as good as any other, it's just EA which failed to handle it. I've just improved the fix here: https://github.com/jwaliszko/ExpressiveAnnotations/commit/b2bd4f674660d16029f3d83c5d92afc75e492fca.

This one is able to handle your original naming as expected - without any warnings and changes at your side. With this fix, only the essential set of methods, actually used in expression, is registered within model context. Previously all available methods were added to the context - hence bigger chance to have naming conflicts.

Finally, if naming conflict with some method actually occurs, e.g. in expression like Length(Length), exception is thrown as client-side compatibility is broken.