Bug 823282 - web-admin/user-portal: replace usage of NonEmptyValidation with a parameter to validateEntity() method (or similar)
web-admin/user-portal: replace usage of NonEmptyValidation with a parameter t...
Status: CLOSED WONTFIX
Product: oVirt
Classification: Community
Component: ovirt-engine-webadmin (Show other bugs)
unspecified
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Einav Cohen
ux
: CodeChange
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-20 11:00 EDT by Einav Cohen
Modified: 2016-01-28 16:55 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-23 04:20:36 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Einav Cohen 2012-05-20 11:00:10 EDT
When using RegexValidation for validation a specific field, the NonEmptyValidation validation shouldn't be used - the non-empty limitation should be already encapsulated within the regular-expression that is used within the RegexValidation.

Due to a bug, empty strings were always considered valid when using RegexValidation on a certain field; as a workaround, NonEmptyValidation was used as an additional validation for the same field.

Now that this bug is fixed (see http://gerrit.ovirt.org/#change,4571):
- Need to remove the unnecessary NonEmptyValidation usages (practically in every place in the code in which RegexValidation is being used)
- Error messages for RegexValidation should be rephrased accordingly.
I.e., a message like:
 "Name must contain alphanumeric characters only." 
should become:
 "Name must be non-empty and contain alphanumeric characters only."
[the rephrased messages will actually represent better the regular expression, since accoring to the regular-expression itself, empty strings are *not* legal, and only due to a bug they were legal]
Comment 1 Einav Cohen 2012-05-22 02:26:10 EDT
Note that usages of NonEmptyValidation inherited classes should be checked as well:
BaseI18NValidation
ByteSizeValidation
IpAddressValidation
MacAddressValidation
NoSpacesValidation

Note that generally, doesn't matter if empty string is either legal or illegal -  it should be encapsulated within the regular expression itself.
So in case there has been NonEmptyValidation in use in addition to the RegexValidation, need to make sure that the regular-expression by itself blocks non-empty strings. If not - it should be changed accordingly (assuming it is easy enough to do).
Comment 2 Tomas Jelinek 2012-05-23 04:54:02 EDT
in gerrit: http://gerrit.ovirt.org/#change,4690
Comment 3 Einav Cohen 2012-05-26 15:16:26 EDT
correction:

(In reply to comment #0)
> - Need to remove the unnecessary NotEmptyValidation usages (practically in
> every place in the code in which RegexValidation is being used)

no need to do that; using NotEmptyValidation results in a dedicated validation error message in case of an empty string ("This field can't be empty"), which is nicer.
However, In order to make sure that this message will appear for empty values, it should be passed first in the IValidation array passed to the validateEntity() method. That way it will be processed first.
One way to avoid making sure that the NotEmptyValidation is processed first is to pass a boolean parameter to "validateEntity()" - either "emptyValueLegal" or "processEmptyValue"
- when passing to validateEntity() an empty value and "emptyValueLegal == false" (or "processEmptyValue == false"), validateEntity() should return "false" immediately and return the "this field can't be empty" message.
- when passing to validateEntity() an empty string and "emptyValueLegal == true" (or "processEmptyValue == true"), validateEntity() should return "true" immediately (or simply process the empty value regularly (according to the IValidation array)).

> - Error messages for RegexValidation should be rephrased accordingly.
> I.e., a message like:
>  "Name must contain alphanumeric characters only." 
> should become:
>  "Name must be non-empty and contain alphanumeric characters only."

Should still be done, in order to keep the integrity / full match of regular expression to validation error message.
Comment 4 Tomas Jelinek 2012-05-28 02:47:07 EDT
> One way to avoid making sure that the NotEmptyValidation is processed first is > to pass a boolean parameter to "validateEntity()" - either "emptyValueLegal" or > "processEmptyValue"
That would mean, that we will change from declarative style like this:
someField.ValidateEntity(
   new IValidation[] {
   new NotEmptyValidation(),
   new RegexValidation(nameExpr, nameMsg)
});
... 

to procedural like this:
...
someField.ValidateEntity(false,
  new IValidation[] {
  new RegexValidation(nameExpr, nameMsg)
});
....

I would say, that the first possibility is more readable than the second (e.g. do you know what does the "false" means without opening the ValidateEntity method?) 

Also the second possibility would mix up two things:
- EntityModel.ValidateEntity would still be an "engine" which orchestrates the validations
- EntityModel.ValidateEntity would become sometimes also a validation (depending on a boolean switch).

I would say, that the instances of IValidation should do one-purpose validations (like LengthValidation, IntegerValidation, NotEmptyValidation, LinuxMountPointValidation...)
and should be passed in the correct order to the EntityModel.ValidateEntity which will orchestrate them. It separates the concerns clearly and is (considering the validations are well named) readable.
Comment 5 Itamar Heim 2012-12-23 04:20:36 EST
Closing old bugs. If this issue is still relevant/important in current version, please re-open the bug.

Note You need to log in before you can comment on or make changes to this bug.