Proposed Change to CollectionSize rule

[Greg Moser]

I wanted to open up a dialog with everyone on here about a potential change to the CollectionSize validation method.  Right now, it defaults to a min & max collection size of 1, but I am proposing that it be switched to 0.

For example if I only wanted a given productType have a maximum of 10 products associated with it, I would setup my rule like this:

<property name="products">

<rule type="collectionSize">

<param name="max" value="10" />

</rule>

</property>

However if my productType doesn't have any products assigned yet I would expect 0 to pass validation, but it doesn't because the range automatically starts at 1.

More importantly I plan to use this rule to validate my ORM entities before I allow them to be deleted.  So the rule actually looks like this:

<property name="products">

<rule type="collectionSize" context="delete">

<param name="max" value="0" />

</rule>

</property>

The idea is that would check to make sure there aren't any products assigned to this productType before actually trying to call entityDelete() on this object.  However, right now the result says "sorry but the value isn't between 1 & 0".

Hope this is making sense, very interested to hear everyone's thoughts on this.

-Greg

[Greg Moser]

To further illustrate my point, I have done a rewrite of this server rule and come to realize that an additional default message would need to be added to the resource bundle as well (which makes sense when you look at the code).

Here is the logic that I am proposing: https://github.com/gregmoser/ValidateThis/blob/develop/ValidateThis/server/ServerRuleValidator_CollectionSize.cfc

Here is the current logic: https://github.com/ValidateThis/ValidateThis/blob/develop/ValidateThis/server/ServerRuleValidator_CollectionSize.cfc

-Greg

[Bob Silverberg]

With the new version, doesn't that remove the need for any default
value at all? I don't think the defaults set are ever used, which
seems to make sense.

One thing I noticed: Your version does not seem to allow for a length
check on a simple string. I believe the original did that, although I
have no idea why anyone would want to use this validation type for
that type of check, so perhaps it's best to leave it out in this
version.

Bob

> --
> You received this message because you are subscribed to the Google Groups
> "ValidateThis-dev" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/validatethis-dev/-/5u_iI_iSa50J.
> To post to this group, send email to validate...@googlegroups.com.
> To unsubscribe from this group, send email to
> validatethis-d...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/validatethis-dev?hl=en.
>

--
Bob Silverberg
www.silverwareconsulting.com

[John Whish]

This sounds like a good idea to me!

- sent by a little green robot powered device

[Greg Moser]

It does actually remove the need for defaults, so now they are just there to be var'd at the top.

As for the length check on a simple string that you talked about being in the orignial version, i don't see anything like that.  However both my version and the original version do a listLen() check for simple values, is that what you are talking about?

-Greg

[Bob Silverberg]

It's this bit in here (found at
https://github.com/ValidateThis/ValidateThis/blob/develop/ValidateThis/server/ServerRuleValidator_CollectionSize.cfc#L81):

if (isSimpleValue(theVal)) {
if (listLen(theVal) gt 1) {
theSize = listLen(theVal);
} else if (listLen(theVal) lte 1) {
if (isRangeCheck) {
theSize = len(theVal);
} else {
theSize = 1;
}
}

If the simple string is not a list with more than one element, then
the length of the string itself is used as theSize. The extra logic
around isRangeCheck is something that doesn't make sense to me, and I
think we need to do away with, but the fact remains that there is some
logic in here to check the length of a string that isn't a list.

Having said that, I really do not see any reason to keep this in here,
as there are other validators to do that, and I don't think the length
of a string can really be considered to be a check of CollectionSize.

Adam, if you are reading this please feel free to chime in with the
logic behind this original version.

Thanks,
Bob

> --
> You received this message because you are subscribed to the Google Groups
> "ValidateThis-dev" group.
> To view this discussion on the web visit

> https://groups.google.com/d/msg/validatethis-dev/-/rErP85bfLvsJ.

[Greg Moser]

You are right, there is some code for length... however now that I see that little bit of code, it really desn't make sense to have it in there in my opinion.

Imagine if you had a list of id's that looked like this: 8,9,10,11
When running that value, you would get a resulting theSize value of 4
However if first 3 items were removed from the list and it was now just: 11
Well then the resulting theSize value would be 2 which is the same as passing 10,11. That I think is a bug in itself.

As always if there is a valid reason for this behavior, I'm all ears.

-Greg

[Bob Silverberg]

I'm not sure I'd agree that it's a bug, but yeah, let's get rid of it.
I think your version looks good. You were going to submit some tests
along with the component in your pull request, right?

Cheers,
Bob

> --
> You received this message because you are subscribed to the Google Groups "ValidateThis-dev" group.

> To view this discussion on the web visit https://groups.google.com/d/msg/validatethis-dev/-/wSVvBVk_DDgJ.

[Greg Moser]

Sounds great, I will put together some tests and submit a pull request tomorrow.

[Greg Moser]

I ran into another issue with the updates that I was making to the ServerRuleValidator_CollectionSize.cfc

At the top of the collection size validator (and all validators) there is a call to shouldValidate()  which in turns calls the propertyHasValue() method in the core/Validation.cfc

The problem with this propertyHasValue() method is that for array's and structs's it requires that there are more than 1 item to consider it a value.  Here is the logic:

<cfreturn (isSimpleValue(theVal) and len(theVal) gt 0) or (isStruct(theVal) and structCount(theVal) gt 0) or (isArray(theVal) and arrayLen(theVal) gt 0)/>

However I argue that an empty array or struct is still a value.... it may not have any items in it's collection (hence why you would use a collection size validator) but it is still a value.

Thoughts?

[Bob Silverberg]

I think perhaps the issue here is that the CollectionSize SRV
shouldn't be using shouldTest(). That is in place to allow for rules
to be optional if a property is empty, but perhaps this rule should
always be checked, whether a property is empty or not?

What do you think?

Bob

> --
> You received this message because you are subscribed to the Google Groups
> "ValidateThis-dev" group.
> To view this discussion on the web visit

> https://groups.google.com/d/msg/validatethis-dev/-/BbA5aTexb4gJ.

[Greg Moser]

I think that the quick solution is to do exactly what you talked about, which is to remove shouldTest() from the top of the SRV, and replace it with a check to make sure that the value is simple, array, or struct.  However if it isn't I would expect a failure and not a success.

That being said for the long term it brings up two questions about VT in general.

1) The shouldTest() method is somewhat ambiguous and not used consistently.  Mostly at the top of SRV's there is a shouldTest() along with some other simple conditionals.  If the whole point of shouldTest() is just to check if the property has a value, or that the property be required, then I think that those methods should be called at the top of the SRV instead to alleviate ambiguity.

2) When it comes to the propertyHasValue() method, I argue that an empty array or an empty struct is still a value, where null or undefined is not a value.

At the end of the day these are really just judgment calls you are going to have to make.  I'm only looking at this problem from an isolated perspective and I know that you have a lot more of a broad view and opinion on the matter.

-Greg

[Greg Moser]

FWIW, my development partner Sumit Verma disagrees with me completely on the second issue.  Here is his quote:

Well, the method says propertyHasValue. So, the check is done based on the type of property. If the property is simple (string) then "blank" mean it has no value, if the property is array, then 0 array len means no value. If the property is struct, then, no struct key means no value. Null means the property itself won't exists. This is also because CF has no concept of "Null Value". 

-Greg

[Sumit Verma]

Just to clarify (not the fact that I do disagree with Greg ;)), it depends on what is the purpose of method. If the purpose is to just check if the property is not null then you can just do isNull(property), no need to check simple/struct/array. But that will only work on CF9+... So, it's also an issue of CF not having a concept of NULL prior to CF9.

[Bob Silverberg]

Great conversation, guys. Thank you for bringing these points up.

I think I agree with both of you on the empty array/struct thing. It
really depends on the context and why you are asking the question.

I also agree that the current implementation of the logic can be
improved, but I'm not in favour of defactoring (if I can coin a term
which I've never heard before), i.e., taking code that has been
encapsulated in a method and breaking it out and repeating it all over
the place. That isn't the solution, imo.

The point of that code is to allow an SRV to have optionality - to
allow the rule to be ignored if the property has not been populated
and is not required. So maybe we need to think of a different way of
indicating that in the SRVs and also reconsider the actual
implementation. I envision some way of saying "hey, this SRV should
be optional if the property hasn't been populated" that we would put
right in the SRV, and then have it implemented in the base SRV. This
is basically what is being done now, expect that now it is done via a
call to shouldTest() in an if statement, so it's a bit difficult to
decipher. Having just written all that, I'm not sure it's actually so
terrible at all, but yes, it could be a bit easier to follow.

Because shouldTest() is in the base SRV, it is overridable, so if the
logic for a particular SRV needs to be different than the standard one
can always put a shouldTest() method in the SRV itself.

Regarding whether an empty array and empty struct should be considered
as having a value, as I said I think it comes down to context. Does
anyone want to volunteer to look through the existing SRVs and see
which ones can validly operate on an array or struct and then we can
evaluate it on a case by case basis?

Thanks,
Bob

> --
> You received this message because you are subscribed to the Google Groups
> "ValidateThis-dev" group.
> To view this discussion on the web visit

> https://groups.google.com/d/msg/validatethis-dev/-/kssl-CrPUOEJ.

[Greg Moser]

This all makes sense and the more that I think about it, you are right... shouldTest() is just a helper method for those SRVs that choose to use it.  If they were all bound to that constraint, then it wouldn't be implemented in the the SRVs but in the wrapper method that calls them.

That being said I made this tweak to the CollectionSize_SRV and pushed it to my develop branch on github.  I'll love to issue a pull request and contribute it back to the project but still need some help with getting the unit tests figured out (sorry I'm still an MXUnit newbie).

Thanks,

Greg