326452 - Required bind only works with post method

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Charles Brunet]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

If the submission method is post, the form isn't sent until all required fields are filled.
If the submission method is get or urlencoded-post, the form is sent even if a required field is empty.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Charles Brunet]

Attachment: testcase

Comment 2

[Allan Beaufour]

Unless I'm sleeping or something this is quite amazing :) Because it seems to be correct. Amazing amazing.

NB. It does also work for put and multipart-post. Not that it makes it so much better though...

Comment 3

[aaronr]

sounds like a good starter bug.  Assigning to Steve.

Comment 4

[Steve Speicher]

Attachment: test case will all submission types, separated

Oddly post,multipart,put work and get,url-post,form-data don't

Comment 5

[Steve Speicher]

After some debugging, here's the issue:

Since post,multipart,put cause nsXFormsSubmissionElement::SerializeDataXML() to be invoked, which indirectly invokes nsXFormsModelElement::HandleInstanceDataNode() (which actually does the validity, relevant and requiredness checking).

HandleInstanceDataNode() is not invoked with the other submission method types.

I'll investigate a way to iterate over the instance nodes for the other method types.

Comment 6

[Steve Speicher]

Attachment: proposed patch to nsXFormsSubmitElement.*

Notes to reviewers:
- Should the arg to CanSubmit() be const?
- Ok to 'borrow' code form CopyChildren(), or is there a better tree traversal approach?
- Should the test be moved to SerializeData* methods? I liked it in in Submit() as it prevents further processing and can be used for other tests (like cross domain?)

Comment 7

[Allan Beaufour]

Comment on attachment 214330 [details] [diff] [review]
proposed patch to nsXFormsSubmitElement.*

Your comments are too long, and maybe use a slightly short variable name. Check:
http://beaufour.dk/jst-review/?patch=214330

(In reply to comment #6)
> Notes to reviewers:
> - Should the arg to CanSubmit() be const?

People (incl. me) are very bad at using const where it should be used. It would be nice if it was, yes.

> - Ok to 'borrow' code form CopyChildren(), or is there a better tree traversal
> approach?

It's ok to keep the style of the code you are changing. I'm not sure we are heading the right path though... more about that.

> - Should the test be moved to SerializeData* methods? I liked it in in Submit()
> as it prevents further processing and can be used for other tests (like cross
> domain?)

It should be moved. You end up doing the same work twice for XML submissions.

>+/*
>+ * This walks the DOM tree, starting at topNode and invokes model->HandleInstanceDataNode()
>+ * that ensures that the instance is valid, required conditions met...if so, then can submit.
>+ */

Good style with a comment! But, please move it to the header file, and use "/**", and comment the argument, just like EndSubmit() is.

>+nsresult
>+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *topNode)

Arguments should start with an "a", so:
nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)

>+{
>+  nsCOMPtr<nsIDOMNode> currentNode(topNode), node;
>+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();

You need to make sure that you actually got a model here. It could be null
NS_ENSURE_STATE(model);

>+
>+  while (currentNode)
>+  {

(I can see that this file has it's own "style", compared to the rest of XForms code.... hmmm, but:)
please move the "{" up on the prev. line.

>+     PRUint16 handleNodeResult;
>+     model->HandleInstanceDataNode(currentNode, &handleNodeResult);
>+
>+    /*
>+     *  SUBMIT_SERIALIZE_NODE   - node is to be serialized
>+     *  SUBMIT_SKIP_NODE        - node is not to be serialized
>+     *  SUBMIT_ABORT_SUBMISSION - abort submission (invalid node or empty required node)
>+     */
>+    if (handleNodeResult == nsIModelElementPrivate::SUBMIT_SKIP_NODE) {
>+       // skip node and subtree
>+       currentNode->GetNextSibling(getter_AddRefs(node));
>+       currentNode.swap(node);
>+       continue;
>+    } else if (handleNodeResult == nsIModelElementPrivate::SUBMIT_ABORT_SUBMISSION) {
>+      // abort
>+      return NS_ERROR_ILLEGAL_VALUE;

Since you are aborting, please use NS_ERROR_ABORT.

>+    }

If you want to be really nice :), you can do a:
NS_ASSERTION(handleNodeResult == SUBMIT_SERIALIZE_NODE)
here, just in case.

>+    // recurse
>+    nsCOMPtr<nsIDOMNode> startNode;

I know this is copy'n'paste, but please call it "firstChild", since that is what it is.

>Index: nsXFormsSubmissionElement.h
>===================================================================
>+  nsresult CanSubmit(nsIDOMNode *source);

Here you call the argument something else than in the cpp file.

Something is wrong in the submission handling. I think we need a common "CreateInstancePurgedForNonRelevantNodes" function, that is called for all serialization possibilities and that will also check for required nodes. We need that to do a full schema-check before submission. BUT, that is a different bug.

Comment 8

[Steve Speicher]

Attachment: patch take 2

Can't find a good solution to const args (at least in this case), as I get into all sorts of nsCOMPtr contructor type conflicts (they want non-const *).  So, not fixing that.

Regarding the comment:
  we need a common "CreateInstancePurgedForNonRelevantNodes" function
I assume that wasn't directed at this patch but towards the forward referenced full schema check bug.

Comment 9

[Allan Beaufour]

Comment on attachment 214460 [details] [diff] [review]
patch take 2

>Index: nsXFormsSubmissionElement.cpp
>===================================================================
>-  // XXX call nsISchemaValidator::validate on each node
>-
>-
>+  // XXX This is handled by the SerializeData() method
>+  

No need for XXX here, if there's no bug.

>+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
>+    } else if (handleNodeResult ==
>+        nsIModelElementPrivate::SUBMIT_ABORT_SUBMISSION) {
>+      // abort
>+      return NS_ERROR_ILLEGAL_VALUE;

Ahem. I remember saying something about this ;-)

>Index: nsXFormsSubmissionElement.h
>===================================================================
>   /**
>+   * This walks the DOM tree, starting at aTopNode and invokes
>+   * model->HandleInstanceDataNode()that ensures that the instance is valid,
>+   * required conditions met...if so, then can submit.
>+   *
>+   * @param aTopNode Root instance node of tree to be evaluated
>+   * @return NS_ERROR_ABORT if instance is not valid and empty required nodes,
>+   * otherwise return NS_OK.
>+   *
>+   */
>+  nsresult CanSubmit(nsIDOMNode *aTopNode);

Please indent the comment properly, that is, something like this:
>+   * @param        aTopNode Root instance node of tree to be evaluated
>+   * @return       NS_ERROR_ABORT if instance is not valid and empty required
>+   *                nodes, otherwise return NS_OK.

With the above changes, r=me

Comment 10

[Steve Speicher]

Attachment: updated with Allan's review comments

Comment 11

[Doron Rosenberg (IBM)]

Comment on attachment 214896 [details] [diff] [review]
updated with Allan's review comments

Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2 spaces only.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> (From update of attachment 214896 [details] [diff] [review] [edit])
> Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode
> *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2
> spaces only.

Not only that, it also has Windows line-endings. Steve: Please convert file to Unix line-endings before submitting.

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 214896 [details] [diff] [review] [edit] [edit])
> > Whoever checks this in, nsXFormsSubmissionElement::CanSubmit(nsIDOMNode
> > *aTopNode) has some 4 space indentation (or tab chars perhaps) that should be 2
> > spaces only.
> 
> Not only that, it also has Windows line-endings. Steve: Please convert file to
> Unix line-endings before submitting.

I also changes the return value in CopyChildren() and not in CanSubmit()...

Comment 14

[Allan Beaufour]

Comment on attachment 214896 [details] [diff] [review]
updated with Allan's review comments

Please fix:
* return value in correct function
* indentation
* unix line endings

Comment 15

[Steve Speicher]

Attachment: fixed: return value, indents, eol

Comment 16

[Allan Beaufour]

Comment on attachment 215005 [details] [diff] [review]
fixed: return value, indents, eol

*sigh* still, Windows line-endings...

Whoever checks this in, be SURE to change this.

Comment 17

[Allan Beaufour]

(In reply to comment #16)
> (From update of attachment 215005 [details] [diff] [review] [edit])
> *sigh* still, Windows line-endings...
> 
> Whoever checks this in, be SURE to change this. 

jst-review v2 now warns about this too...

Comment 18

[aaronr]

fixed line endings.  Checked into trunk for sspeiche.

Comment 19

[Allan Beaufour]

Comment on attachment 215005 [details] [diff] [review]
fixed: return value, indents, eol

>+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
>+{
>+  nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node;
>+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>+  NS_ENSURE_STATE(model);

Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on each call. Very expensive. We need to change this.

I'm suspecting something else is wrong too...

Comment 20

[Allan Beaufour]

(In reply to comment #19)
> (From update of attachment 215005 [details] [diff] [review] [edit])
> >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
> >+{
> >+  nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node;
> >+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
> >+  NS_ENSURE_STATE(model);
> 
> Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on
> each call. Very expensive. We need to change this.

CopyChildren does the same... :(

> I'm suspecting something else is wrong too...

Still trying.

Comment 21

[aaronr]

(In reply to comment #19)
> (From update of attachment 215005 [details] [diff] [review] [edit])
> >+nsXFormsSubmissionElement::CanSubmit(nsIDOMNode *aTopNode)
> >+{
> >+  nsCOMPtr<nsIDOMNode> currentNode(aTopNode), node;
> >+  nsCOMPtr<nsIModelElementPrivate> model = GetModel();
> >+  NS_ENSURE_STATE(model);
> 
> Ouch. I just saw this. The recursive CanSubmit() function calls GetModel() on
> each call. Very expensive. We need to change this.
> 
> I'm suspecting something else is wrong too...
> 

Not THAT expensive.  Gets the parent node and QI's it.  But you are right.  No reason we can't pass model through as a parameter since it is recursive.

Comment 22

[Allan Beaufour]

(In reply to comment #21)
> Not THAT expensive.  Gets the parent node and QI's it.  But you are right.  No
> reason we can't pass model through as a parameter since it is recursive.

Well, it's unnecessary and being done for every tree level in an instance document... that's expensive in my world.