Commons Validator
  1. Commons Validator
  2. VALIDATOR-163

[validator] validator-rules.xml JavaScript fails when field not present in jsp

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0 Release
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      Sometimes, esp. when writing a wizard-like interface, not all the form fields
      will be present in a single jsp - each jsp asks for a subset of the required
      data from the user. It's useful to validate each piece of data. If you use the
      validator.xml as supplied, it will fail for rules where the particular field
      doesn't exist in the jsp.

      I propose adding a check in each of the validators to check if the form field
      is undefined (i.e. doesn't exist) before attempting to use its properties.

      Each validator must have the following line or similar (in the case
      of "required") under the for (x in oRequired) { line:

      for (x in oRequired) {
      if (form[oRequired[x][0]] != undefined) {

      Hope this is of use!

      Andrew

        Activity

        Hide
        Andrew Berridge added a comment -

        Created an attachment (id=3820)
        Updated validator-rules.xml

        Show
        Andrew Berridge added a comment - Created an attachment (id=3820) Updated validator-rules.xml
        Hide
        Robert Leland added a comment -

        Are you using the nightly build ?
        You may find that it has the behavior you desire.
        Also when submitting a patch generate is using
        'diff -u' and state which version of the CVS
        file it is a diff against.

        Show
        Robert Leland added a comment - Are you using the nightly build ? You may find that it has the behavior you desire. Also when submitting a patch generate is using 'diff -u' and state which version of the CVS file it is a diff against.
        Hide
        Ted Husted added a comment -

        Apparently fixed.

        Show
        Ted Husted added a comment - Apparently fixed.
        Hide
        Rob Hannah added a comment -

        As of 1.1 release, this error exists (I don't know if it since the 2002/11/12
        build). The <html:javascript.../> tag generates code for non-present fields.
        Even when I comment out estimatedAnnualPremium, the source includes an attribute
        for it in method required(). And there is no check for undefined in any of the
        javascript:

        <!-nested:text property="estimatedAnnualPremium" size="12"/->
        ...
        this.ac = new Array("estimatedAnnualPremium", "Estimated Annual Premium is
        required.", new Function ("varName", "this.max='1000000';
        this.mask=/^\\d+(\\.
        d

        {2}

        )?$/; this.min='0'; return this[varName];"));

        Show
        Rob Hannah added a comment - As of 1.1 release, this error exists (I don't know if it since the 2002/11/12 build). The <html:javascript.../> tag generates code for non-present fields. Even when I comment out estimatedAnnualPremium, the source includes an attribute for it in method required(). And there is no check for undefined in any of the javascript: <!- nested:text property="estimatedAnnualPremium" size="12"/ -> ... this.ac = new Array("estimatedAnnualPremium", "Estimated Annual Premium is required.", new Function ("varName", "this.max='1000000'; this.mask=/^\\d+(\\. d {2} )?$/; this.min='0'; return this [varName] ;"));
        Hide
        Rob Hannah added a comment -

        Created an attachment (id=8263)
        validator-rules.xml nightly build 20030917 w/ null tests included

        Show
        Rob Hannah added a comment - Created an attachment (id=8263) validator-rules.xml nightly build 20030917 w/ null tests included
        Hide
        Robert Leland added a comment -

        Rob,

        We might also want to modify the <html:javascript> tag
        to turn on 'ignoring null fields', so the old behaviour
        is preserved.

        So <html:javascript ignoreNullField="true" />

        then in the javascript functions.
        use if (field == null && ignoreNullField == "true")

        { continue; }

        What do you think ?

        Show
        Robert Leland added a comment - Rob, We might also want to modify the <html:javascript> tag to turn on 'ignoring null fields', so the old behaviour is preserved. So <html:javascript ignoreNullField="true" /> then in the javascript functions. use if (field == null && ignoreNullField == "true") { continue; } What do you think ?
        Hide
        CyberZombie added a comment -

        I was thinking of pushing it deeper – down into validation.xml as a <field>
        optional attribute. That way, the server side can also be clued in to the
        possibility of a field not being present.

        Show
        CyberZombie added a comment - I was thinking of pushing it deeper – down into validation.xml as a <field> optional attribute. That way, the server side can also be clued in to the possibility of a field not being present.
        Hide
        Robert Leland added a comment -

        Why can't the 'page' attribute in the validator.xml,
        be used for multi-page validations ? This seems to work cleanly for
        the struts-validator example.

        I am going to close this tricket as Invalid, but if
        there is a use case that this doesn't work for then
        please reopen this ticket, and attach the use case.

        Show
        Robert Leland added a comment - Why can't the 'page' attribute in the validator.xml, be used for multi-page validations ? This seems to work cleanly for the struts-validator example. I am going to close this tricket as Invalid, but if there is a use case that this doesn't work for then please reopen this ticket, and attach the use case.
        Hide
        CyberZombie added a comment -

        The big reason is that my form is not used in a multi-page layout. It is a
        single form that represents one of 4-5 objects (all with the same base class).
        I have <nested:equals> tags to sub in subclass-specific data with the majority
        used by all. The page attribute would never work in this case. Even requiredIf
        and when are lacking and/or add large amounts of difficult-to-maintain business
        logic tied to the UI (and not in the business layer). I have already added a
        few custom tags (w/ associated Java and CDATA to support client/server
        validation) for processing a collection of checkboxes (generalized so that it
        allows for 'none', 'any', 'all', and - in the case of 2 - 'xor'). I've even
        added a custom tag to force some CDATA into the generated HTML that does no
        validation – only initializes the page based on what it thinks is there.

        Much of this (save the booleanCompare) would not be necessary if Struts cleanly
        supported a 'variable' page content as opposed (rather in addition) to the
        multi-page wizard.

        Show
        CyberZombie added a comment - The big reason is that my form is not used in a multi-page layout. It is a single form that represents one of 4-5 objects (all with the same base class). I have <nested:equals> tags to sub in subclass-specific data with the majority used by all. The page attribute would never work in this case. Even requiredIf and when are lacking and/or add large amounts of difficult-to-maintain business logic tied to the UI (and not in the business layer). I have already added a few custom tags (w/ associated Java and CDATA to support client/server validation) for processing a collection of checkboxes (generalized so that it allows for 'none', 'any', 'all', and - in the case of 2 - 'xor'). I've even added a custom tag to force some CDATA into the generated HTML that does no validation – only initializes the page based on what it thinks is there. Much of this (save the booleanCompare) would not be necessary if Struts cleanly supported a 'variable' page content as opposed (rather in addition) to the multi-page wizard.
        Hide
        CyberZombie added a comment -

        See prior comments for usage. Not really a use case...but I'm too tired at the
        moment to flush out a good one.

        Show
        CyberZombie added a comment - See prior comments for usage. Not really a use case...but I'm too tired at the moment to flush out a good one.
        Hide
        Joe Germuska added a comment -

        Since all of the javascript has moved to Commons Validator, this is no longer a Struts issue. I know the
        Javascript has changed a lot, so it may be a complete non-issue, but I'm not qualified to judge.

        Show
        Joe Germuska added a comment - Since all of the javascript has moved to Commons Validator, this is no longer a Struts issue. I know the Javascript has changed a lot, so it may be a complete non-issue, but I'm not qualified to judge.
        Hide
        Paul Benedict added a comment -

        This is not an enhancement, but a bug. I am running into this problem today, and
        the best patch available is from comment #1, which is placing this code after
        retrieving the field:
        if (field == null)

        { continue; }

        To answer comment #7: When fields are not required, there should not be a
        penalty if the field is absent. All server-side validation works under this
        premesis. For example, when the "maxlength" validation is specified for a form
        property, but the field is absent in the submission, the validation is not
        applied. Thus, the implication is that it is acceptable for non-required fields
        to be absent.

        So to "push it deeper – down into validation.xml as a <field> optional
        attribute" is not needed. By definition, fields without the "required"
        validation are optional.

        However, today's JavaScript validation is deficient and a penalty occurs for
        non-required fields. The JavaScript blindly tries to access properties for
        fields that may not exist. This penalty needs to be removed to match expectations.

        I am willing to work on this if no one else is.

        Show
        Paul Benedict added a comment - This is not an enhancement, but a bug. I am running into this problem today, and the best patch available is from comment #1, which is placing this code after retrieving the field: if (field == null) { continue; } To answer comment #7: When fields are not required, there should not be a penalty if the field is absent. All server-side validation works under this premesis. For example, when the "maxlength" validation is specified for a form property, but the field is absent in the submission, the validation is not applied. Thus, the implication is that it is acceptable for non-required fields to be absent. So to "push it deeper – down into validation.xml as a <field> optional attribute" is not needed. By definition, fields without the "required" validation are optional. However, today's JavaScript validation is deficient and a penalty occurs for non-required fields. The JavaScript blindly tries to access properties for fields that may not exist. This penalty needs to be removed to match expectations. I am willing to work on this if no one else is.
        Hide
        Niall Pemberton added a comment -

        I'm currently doing some work on an extension to Commons Validator under Bug
        32343.

        If the proposal is accepted then I believe this issue will be resolved. Theres
        a working Example webapp here, if you are interested in taking a look:

        http://www.niallp.pwp.blueyonder.co.uk/validatorjs.html

        Niall

        Show
        Niall Pemberton added a comment - I'm currently doing some work on an extension to Commons Validator under Bug 32343. If the proposal is accepted then I believe this issue will be resolved. Theres a working Example webapp here, if you are interested in taking a look: http://www.niallp.pwp.blueyonder.co.uk/validatorjs.html Niall
        Hide
        Paul Benedict added a comment -

        Niall, your proposal looks very good and interesting, but also pretty hefty in
        its changes. I am just looking for a small patch compared to the extant that you
        are proposing. Do you think I should still continue?

        Show
        Paul Benedict added a comment - Niall, your proposal looks very good and interesting, but also pretty hefty in its changes. I am just looking for a small patch compared to the extant that you are proposing. Do you think I should still continue?
        Hide
        Paul Benedict added a comment -

        Created an attachment (id=13713)
        JS Validation skips fields that are not present.

        The attachment contains an update to the javascript methods. There are three
        main changes:

        1) Absent fields are skipped over. For most methods, this line has been added:
        if (!isFieldPresent(field))

        { continue; }

        ...Except for the validateRequired method, which by its nature demands that
        absent required fields generate an error response.

        2) Focus is not set to fields that are absent, hidden, or disabled:
        if(isFieldFocusable(focusField))

        { focusField.focus(); }

        3) Two new methods in validateUtilities: isFieldPresent and isFieldFocusable

        These changes are intended to patch this problem in the 1.1.x branch of
        Validator. I know Niall (see comment #13) is working on some hefty changes that
        solve the problem too, but that may end up in 1.2? I don't know. I am just
        looking for a simple fix. Thanks!

        Show
        Paul Benedict added a comment - Created an attachment (id=13713) JS Validation skips fields that are not present. The attachment contains an update to the javascript methods. There are three main changes: 1) Absent fields are skipped over. For most methods, this line has been added: if (!isFieldPresent(field)) { continue; } ...Except for the validateRequired method, which by its nature demands that absent required fields generate an error response. 2) Focus is not set to fields that are absent, hidden, or disabled: if(isFieldFocusable(focusField)) { focusField.focus(); } 3) Two new methods in validateUtilities: isFieldPresent and isFieldFocusable These changes are intended to patch this problem in the 1.1.x branch of Validator. I know Niall (see comment #13) is working on some hefty changes that solve the problem too, but that may end up in 1.2? I don't know. I am just looking for a simple fix. Thanks!
        Hide
        Don Brown added a comment -

        Niall, any update on this patch?

        Show
        Don Brown added a comment - Niall, any update on this patch?
        Hide
        Don Brown added a comment -
            • COM-2071 has been marked as a duplicate of this bug. ***
        Show
        Don Brown added a comment - COM-2071 has been marked as a duplicate of this bug. ***
        Hide
        Don Brown added a comment -

        Paul, I'm willing to apply your quick patch. Could you please make it against
        the commons-validator trunk? Thanks.

        Show
        Don Brown added a comment - Paul, I'm willing to apply your quick patch. Could you please make it against the commons-validator trunk? Thanks.
        Hide
        Rachel Vanderboom added a comment -

        (In reply to comment #15)
        > ...Except for the validateRequired method, which by its nature demands that
        > absent required fields generate an error response.

        Paul,
        I disagree with this premise. You may have a field that is required only for
        certain users and is hidden or absent to other users. To me it seems, the
        absence of any field should ignore any validation. If the field is absolutely
        required and is not on the form, then that is the fault of the coder. The user
        will not be able to fix the required error if the field does not exist, so why
        give them a validation error for a field that does not exist?

        btw, thank you for fixing the other validation routines to ignore absent fields.

        Show
        Rachel Vanderboom added a comment - (In reply to comment #15) > ...Except for the validateRequired method, which by its nature demands that > absent required fields generate an error response. Paul, I disagree with this premise. You may have a field that is required only for certain users and is hidden or absent to other users. To me it seems, the absence of any field should ignore any validation. If the field is absolutely required and is not on the form, then that is the fault of the coder. The user will not be able to fix the required error if the field does not exist, so why give them a validation error for a field that does not exist? btw, thank you for fixing the other validation routines to ignore absent fields.
        Hide
        Don Brown added a comment -

        Marking as an enhancement as it works as advertised.

        Show
        Don Brown added a comment - Marking as an enhancement as it works as advertised.
        Hide
        Niall Pemberton added a comment -

        (In reply to comment #19)
        Rachel, sorry I don't agree - however you can overcome this by plugging in your
        own custom/modified version of the required validation.

        (In reply to comment #15)
        Paul, thanks for the patch - I had just fixed the "focus" issue for COM-665 -
        unfortunately not seeing this first . Anyway, applied your patch for this
        issue - thanks:

        http://svn.apache.org/viewcvs?rev=367249&view=rev

        This will be available to test in the next nightly build:

        http://cvs.apache.org/builds/jakarta-commons/nightly/commons-validator/

        Closing as FIXED

        Show
        Niall Pemberton added a comment - (In reply to comment #19) Rachel, sorry I don't agree - however you can overcome this by plugging in your own custom/modified version of the required validation. (In reply to comment #15) Paul, thanks for the patch - I had just fixed the "focus" issue for COM-665 - unfortunately not seeing this first . Anyway, applied your patch for this issue - thanks: http://svn.apache.org/viewcvs?rev=367249&view=rev This will be available to test in the next nightly build: http://cvs.apache.org/builds/jakarta-commons/nightly/commons-validator/ Closing as FIXED
        Hide
        Niall Pemberton added a comment -

        Re-openned and the set to "Resolved Fixed" again to correct "resolution" which was lost in Bugzilla --> JIRA conversion

        Show
        Niall Pemberton added a comment - Re-openned and the set to "Resolved Fixed" again to correct "resolution" which was lost in Bugzilla --> JIRA conversion

          People

          • Assignee:
            Niall Pemberton
            Reporter:
            Andrew Berridge
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development