uon-nuget / UoN.ExpressiveAnnotations.NetCore

Annotation-based conditional validation library.
Other
29 stars 12 forks source link

No more than 27 unique attributes of the same type can be applied for a single field or property. #31

Open gdycus opened 3 years ago

gdycus commented 3 years ago

I'm getting the following error "No more than 27 unique attributes of the same type can be applied for a single field or property."

It appears this issue was fixed in v2.6.6 on the original ExpressiveAnnotations project https://github.com/jwaliszko/ExpressiveAnnotations/issues/96

Can you confirm if your project has the changes from v2.6.6? If not, would you be willing to add that change to this project?

Thanks so much for this project!

izakbar commented 11 months ago

It isn't fixed for me. Does look like that fix is in the code though when I looked. Issue I have is when using arrays of structures - the EA WeakID doesn't take array reference into account. Thus if you have 14 array elements and 2 x AssertIf validations the 27 items issue occurs. I do have a possible fix for this - using the context.Attributes["id"] in making the WeakId (if possible) which fixes it for me. Will create PR - see if it helps

MmmBerry commented 11 months ago

From memory, in the end I didn't incorporate this fix because the problem I have with it is when I looked into it, it turned out that the fix in v2.6.6 on the original ExpressiveAnnotations project was rolled back later on, for reasons that are unknown to me. It might not be obvious from the commit messages but I think that change was rolled back. So when this was discussed before, I think I explained that I wasn't comfortable including a change that had been rolled back in the original project, perhaps because it broke something. It wasn't clear though - perhaps it had been rolled back by mistake when implementing a later change. It might or might not not be a problematic change.

It does seem like a few people want to validate more than 26 items but if the proposed change comes with unknown issues, then it doesn't seem right to accept the fix and potentially break things for everyone else. Sadly I don't have time to test this myself, I don't actually support any code that uses this package any more and I'm overwhelmed with other (completely different) work these days. I think the best approach for people who need more than 26 validations would be to fork and implement this fix, either in a private or public version of the package. Maybe somebody has already done so in the last couple of years - perhaps worth exploring forks of this repo.

izakbar commented 11 months ago

The fix I have tried to make doesn't allow more than 27 validations for a single item - it allows collections with validated items not to count as one item and multiply up the validation (so number of validators x number of items > 27 in original code makes things break).

I have gone about it a slightly different way and used the id from the attributes if it exists to generate the fieldid/weakid - else use existing processing for this. I have pulled the context.Attributes["id"] and passed to validators to use in the full/weakid name. thus you can only still have 27 validators per item - but it is per id not per type.fieldname

original code : var fieldId = $"{metadata.ContainerType.FullName}.{metadata.PropertyName}".ToLowerInvariant(); AttributeFullId = $"{attribute.TypeId}.{fieldId}".ToLowerInvariant(); AttributeWeakId = $"{typeof(T).FullName}.{fieldId}".ToLowerInvariant(); FieldName = metadata.PropertyName;

my change : var propertytName = string.IsNullOrEmpty(attributeId) ? metadata.PropertyName : attributeId; var fieldId = $"{metadata.ContainerType.FullName}.{propertytName}".ToLowerInvariant(); AttributeFullId = $"{attribute.TypeId}.{fieldId}".ToLowerInvariant(); AttributeWeakId = $"{typeof(T).FullName}.{fieldId}".ToLowerInvariant(); FieldName = metadata.PropertyName;

When dealing with collections - the PropertyName using original code would not include any index ref so All Properties with same name in collection would add up (collection[0].Property, collection[1].Property and collection[2].Property would be seen as the same and triple up the validators). When looking at the html generator you can see adaptors being allocated : for collection[0].Property - adaptor, adaptora, adaptorb for collection[1].Property adaptorc,adaptord,adaptore for collection[2].Property adaptorf,adaptorg,adaptorh what I think should really be happening is each one should get adaptor, adaptora, adaptorb - which is what my code provides.

It looks like the fix made originally for 2.6.6 was to AllocateSuffix() (that actually did anything) and does look like original maintainer made changes for 2.6.7, 2.7.0 and they look to still be in current version - not sure things are backed out - history doesn't seem to indicate it has been and just slight tweaks.

I have already decided that I might need to use a local modified version of this package on our azure setup - but fancied seeing if it helped anyone else.