search

uon-nuget / UoN.ExpressiveAnnotations.NetCore

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

Added fix for collection of model with attribute #27

Closed kooliokey closed 2 years ago

kooliokey commented 5 years ago

There is a max of 27 unique attributes allowed on an individual property. This limit is (incorrectly) hit when:

There was a discussion and fix in the original project's repo, which I've linked to below.

Related fix commit in original repo: https://github.com/jwaliszko/ExpressiveAnnotations/commit/ad078cccc63ca1d83464d55fd913a9dd44999907

Discussion of the issue, leading to commit above: https://github.com/jwaliszko/ExpressiveAnnotations/issues/96

beforan commented 5 years ago

@kooliokey thanks for the fix! @MmmBerry can you sanity check it from our side, since you know EA

From me, it'll want a version bump (1.0.1 I expect?) before it makes it into master, otherwise nuget will be upset when we try and push 1.0.0 again...

MmmBerry commented 5 years ago

Thanks for raising this @kooliokey. I've reviewed the code change in this PR, and it seems to make sense. However, on looking into the fix commit in the original repo (jwaliszko@ad078cc) I see that this fix was reverted in the original repo a few commits later ([jwaliszko@4e33aaf), with the comment 'validator cleanups'.

Since I don't understand the potential implications of this change as well as jwaliszko, and I'm not in a position to fully regression test the change, I don't yet feel confident that the change will not introduce new problems. The fix was applied by jwaliszko and then rolled back, and the fix does not appear to be present in the latest version of the original repo, from which I forked.

I'd therefore like a better understanding of why jwaliszko reverted the change, and/or reassurance that it has been thoroughly regression-tested, before accepting the PR. If anyone can shed any light on this, I'll be happy to look at it again.

As an aside, there are also 'changed' files, package.json and package-lock.json, in this PR, which look like they must be due to line-breaks or similar because I can't see any actual change in those files. Also the change to the Contact model, for testing the change, doesn't look like something that should be in the sample, since this issue doesn't seem fundamental to demonstrating general usage of the package, which is what the sample is for. So I think I'd want those changes removed from the PR, and the version bump @beforan mentioned, before including this fix, assuming we agree that it's safe to include the fix.

kooliokey commented 5 years ago

Ahh gotcha - that is an understandable concern. I wonder why they reverted it as well. Perhaps there is another spot they moved it to that isn’t obvious since it is disconnected from when it was originally fixed. I’ll go ahead and hold off on the changes you want until it is decided this should go into the repo.

Thanks for the responses!

MmmBerry commented 5 years ago

@kooliokey do you know whether the latest version of the original project's repo is able to support your scenario of a collection of more than 27 instances of a model? If so, and this issue is fixed in the original project, then there might be some other code that replaced the fix we've been looking at.

Mursicmiso commented 4 years ago

Hi,

Any plans on adding this fix?

Thanks.

irinabatchelor commented 2 years ago

Hi,

Just ran into this issue. I see there is no change to this issue since almost two years ago. May there be a workaround for this or any plans on fixing the issue?

Thank you.