353677 - serialization using method="multipart-post" doesn't serialize parts

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060911 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060911 Minefield/3.0a1

Files marked by the <upload> control as xsd:anyURI and with submission method="multipart-post", the referenced resources are to be serialized as multipart/related.  The resources referenced by upload control are not being serialized.

Reproducible: Always

Comment 1

[Steve Speicher]

Attachment: testcase

Comment 2

[Steve Speicher]

Here's what gets submitted with Moz:
-----------------------------24464570528145
Content-Type: application/xml
Content-ID: <[e-mail]>

<?xml version="1.0" encoding="UTF-8"?>
<mail xmlns:xhtml="http://www.w3.org/1999/xhtml" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xforms="http://www.w3.org/2002/xforms">
          <picture>
            <attach xsi:type="xsd:anyURI">file:///C:/temp/crap2.xml</attach>
            <name xsi:type="xsd:anyURI"/>
            <type xsi:type="xsd:string">text/xml</type>
          </picture>
        </mail>
-----------------------------24464570528145
Content-Type: application/octet-stream
Content-Transfer-Encoding: binary
Content-ID: <[e-mail]>


-----------------------------24464570528145
Content-Type: application/octet-stream
Content-Transfer-Encoding: binary
Content-ID: <[e-mail]>


-----------------------------24464570528145--

Here's what gets submitted with X-Smiles:
--------xs1q2w3e4r5t6y7u8i50180
Content-Type: application/xml
Content-ID: <[e-mail]>
<?xml version="1.0" encoding="ISO-8859-1"?>
<mail xmlns:xforms="http://www.w3.org/2002/xforms" xmlns:xhtml="http://www.w3.org/1999/xhtml" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
          <picture>
            <attach xsi:type="xsd:anyURI">cid:[e-mail]</attach>
            <name xsi:type="xsd:anyURI"/>
            <type xsi:type="xsd:string">application/xml</type>
          </picture>
        </mail>
--------xs1q2w3e4r5t6y7u8i50180
Content-Type: application/xml
Content-Transfer-Encoding: binary
Content-ID: <cid:[e-mail]>
<?xml version="1.0" encoding="UTF-8"?>
<db:car xmlns:db="http://www.example.org/double"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://www.example.org/double crap2.xsd ">
  <db:price>-INF</db:price>
</db:car>

--------xs1q2w3e4r5t6y7u8i50180

Comment 3

[Merle Sterling]

TCPMon: listen port = 8081; targetHostname = www.xformstest.org; targetPort=80
xforms:submission action=http://localhost:8081/cgi-bin/echo.sh or showinstance.sh

Comment 4

[Merle Sterling]

Attachment: Simple XML file to submit

Comment 5

[Merle Sterling]

Attachment: patch

nsXFormsUploadElement sets the UploadFileProperty to a pointer to a LocalFile but when the submission document is created in nsFormsSubmissionElement, ImportNode does not copy the property so we think there is no file to upload. The patch will check if the property exists in the original node and will clone the file and set the property on the new node in the submission doc.

Additionally, the file URI that was put in the node by upload element needs to be replaced with the generated content ID in createAttachments: currentNode->setNodeValue(cid) doesn't work. If we get the first child and set its node value to the content ID, the submitted document is correct.

If this patch is to be checked in before January, any review comments will have to be handled by someone else as I will be out for the rest of the year; otherwise I'll address any issues when I return.

Comment 6

[aaronr]

Comment on attachment 247911 [details] [diff] [review]
patch

I've got to r- because this patch won't handle the case where the upload is bound to an attribute node.  This patch assumes the data node is an element node and it shouldn't.  I'll fix up the patch since Merle is on vacation.

Comment 7

[aaronr]

Attachment: testcase2

testcase where upload is bound to an attribute node in the instance data.

Comment 8

[aaronr]

Attachment: patch2

Fixed up Merle's patch to properly handle when upload binds to attributes.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 248189 [details] [diff] [review]
patch2

Possibly too late here to review :) , but looks ok to me, especially if you add some comment why the following isn't needed:
-      } else {
-        nsCOMPtr<nsIAttribute> attr = do_QueryInterface(currentNode);
-        NS_ENSURE_STATE(attr);
-        uploadFileProperty =
-          attr->GetProperty(nsXFormsAtoms::uploadFileProperty);
       }

Comment 10

[aaronr]

Attachment: patch3

adds attribute comment per smaug's review comment.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 248195 [details] [diff] [review]
patch3


> nsresult
> nsXFormsSubmissionElement::CreateAttachments(nsIModelElementPrivate *aModel,
>                                              nsIDOMNode             *aNode,
>                                              SubmissionAttachmentArray *aAttachments)

Code of CreateAttachments looks very intricated though idea of the method is very simple. I guess you can use here document traversal api to simplify things and to avoid code dublication.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 248195 [details] [diff] [review]
patch3


>+              // the cloning does not copy any properties of the currentNode. If
>+              // the attribute node has an uploadFileProperty we need to copy it
>+              // to the submission document so that local files will be attached
>+              // properly when the submission format is multipart-related.

....

I don't like very big methods. Therefore can you move this code into new method? Also I guess this allow to avoid code dublication.

Comment 13

[aaronr]

(In reply to comment #12)
> (From update of attachment 248195 [details] [diff] [review] [edit])
> 
> >+              // the cloning does not copy any properties of the currentNode. If
> >+              // the attribute node has an uploadFileProperty we need to copy it
> >+              // to the submission document so that local files will be attached
> >+              // properly when the submission format is multipart-related.
> 
> ....
> 
> I don't like very big methods. Therefore can you move this code into new
> method? Also I guess this allow to avoid code dublication.
> 


I can move the code into a different method.  But it won't be any smaller.  I don't see any duplication that can be avoided.  nsIContent and nsIAttribute are distinctly different and they each have their own Set/GetProperty methods on the branches though I believe on trunk the property stuff was moved up a level.  Do you still want the code moved out to another function?

Comment 14

[aaronr]

(In reply to comment #11)
> (From update of attachment 248195 [details] [diff] [review] [edit])
> 
> > nsresult
> > nsXFormsSubmissionElement::CreateAttachments(nsIModelElementPrivate *aModel,
> >                                              nsIDOMNode             *aNode,
> >                                              SubmissionAttachmentArray *aAttachments)
> 
> Code of CreateAttachments looks very intricated though idea of the method is
> very simple. I guess you can use here document traversal api to simplify things
> and to avoid code dublication.
> 

?  Can you give me an example of what you mean?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)

> > Code of CreateAttachments looks very intricated though idea of the method is
> > very simple. I guess you can use here document traversal api to simplify things
> > and to avoid code dublication.
> > 
> 
> ?  Can you give me an example of what you mean?
> 

I meant http://lxr.mozilla.org/mozilla/source/dom/public/idl/traversal/, partially http://lxr.mozilla.org/mozilla/source/dom/public/idl/traversal/nsIDOMDocumentTraversal.idl.  DOM elements as well DOM attributes is be traversed. NodeFilter::accept() will allow only those nodes that have uploadFileProperty. nsIDOMTreeWalker methods will be used to process nodes and to add attachment. Or something else. The result is you have one 'for' statement instead recursive createAttachments() calls and traversing in cycle through attributes.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #13)

>  Do you still want the code moved out to another function?
> 

Yes, I like :). The method CloneUploadProperty() or something else helps to keep code more readable.

> I can move the code into a different method.  But it won't be any smaller.  I
> don't see any duplication that can be avoided.  nsIContent and nsIAttribute are
> distinctly different and they each have their own Set/GetProperty methods on
> the branches though I believe on trunk the property stuff was moved up a level.

So, probably it makes sence to have one more method to get/set property. Right, nsiContent and nsIAttribute is different things but following code is common:

// Clone the local file so the same pointer isn't released twice.
nsIFile *file = NS_STATIC_CAST(nsIFile *, uploadFileProperty);
nsIFile *fileCopy = nsnull;
nsresult rv = file->Clone(&fileCopy);
NS_ENSURE_SUCCESS(rv, rv);
destChildContent->SetProperty(nsXFormsAtoms::uploadFileProperty,
                              fileCopy,
                              ReleaseObject);

So, if you can have method like
CloneUploadPropertyFrom(nsIDOMNode* aSource, nsIDOMNode* aDest) that will handle  both attribute node and element node.

But probably it doesn't make sence since dublication is little. It's up to you.

Comment 17

[aaronr]

(In reply to comment #15)
> (In reply to comment #14)
> 
> > > Code of CreateAttachments looks very intricated though idea of the method is
> > > very simple. I guess you can use here document traversal api to simplify things
> > > and to avoid code dublication.
> > > 
> > 
> > ?  Can you give me an example of what you mean?
> > 
> 
> I meant http://lxr.mozilla.org/mozilla/source/dom/public/idl/traversal/,
> partially
> http://lxr.mozilla.org/mozilla/source/dom/public/idl/traversal/nsIDOMDocumentTraversal.idl.
>  DOM elements as well DOM attributes is be traversed. NodeFilter::accept() will
> allow only those nodes that have uploadFileProperty. nsIDOMTreeWalker methods
> will be used to process nodes and to add attachment. Or something else. The
> result is you have one 'for' statement instead recursive createAttachments()
> calls and traversing in cycle through attributes.
> 

Looking at the spec (http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/DOM2-Traversal-Range.txt), it doesn't look like using a nsIDOMTreeWalker isn't going to buy us much.  It says:

SHOW_ATTRIBUTE
     Show Attr nodes. This is meaningful only when creating
     an iterator or tree-walker with an attribute node as its
     root; in this case, it means that the attribute node
     will appear in the first position of the iteration or
     traversal. Since attributes are never children of other
     nodes, they do not appear when traversing over the
     document tree.

So the dom tree walker won't encounter attributes unless we set the root of the tree walker to be an attribute.  So we couldn't filter out any element nodes just because they didn't have the upload property since either way we'd still have to check each element for attributes by hand like we are currently doing.

Comment 18

[aaronr]

(In reply to comment #16)
> (In reply to comment #13)
> 
> >  Do you still want the code moved out to another function?
> > 
> 
> Yes, I like :). The method CloneUploadProperty() or something else helps to
> keep code more readable.
> 
> > I can move the code into a different method.  But it won't be any smaller.  I
> > don't see any duplication that can be avoided.  nsIContent and nsIAttribute are
> > distinctly different and they each have their own Set/GetProperty methods on
> > the branches though I believe on trunk the property stuff was moved up a level.
> 
> So, probably it makes sence to have one more method to get/set property. Right,
> nsiContent and nsIAttribute is different things but following code is common:
> 
> // Clone the local file so the same pointer isn't released twice.
> nsIFile *file = NS_STATIC_CAST(nsIFile *, uploadFileProperty);
> nsIFile *fileCopy = nsnull;
> nsresult rv = file->Clone(&fileCopy);
> NS_ENSURE_SUCCESS(rv, rv);
> destChildContent->SetProperty(nsXFormsAtoms::uploadFileProperty,
>                               fileCopy,
>                               ReleaseObject);
> 
> So, if you can have method like
> CloneUploadPropertyFrom(nsIDOMNode* aSource, nsIDOMNode* aDest) that will
> handle  both attribute node and element node.
> 
> But probably it doesn't make sence since dublication is little. It's up to you.
> 

I'd vote no.  Not much in way of savings.  I'll keep it in mind in case we set the property somewhere else in the future.  3 times would probably be worth a utility function.

Comment 19

[aaronr]

Comment on attachment 248195 [details] [diff] [review]
patch3

In lieu of my last two comments, does this pass the muster?

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 248195 [details] [diff] [review]
patch3

r=me, sorry about traversal :)

Comment 21

[aaronr]

checked into trunk for msterlin

Comment 22

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

Comment 23

[Sheldon Mapes]

(In reply to aaronr from comment #17)

(In reply to comment #15)

(In reply to comment #14)

Code of CreateAttachments looks very intricated though idea of the method is
very simple. I guess you can use here document traversal api to simplify things
and to avoid code dublication.

? Can you give me an example of what you mean?

I meant http://lxr.mozilla.org/mozilla/source/dom/public/idl/traversal/,
partially
http://lxr.mozilla.org/mozilla/source/dom/public/idl/traversal/nsIDOMDocumentTraversal.idl.
DOM elements as well DOM attributes is be traversed. NodeFilter::accept() will
allow only those nodes that have uploadFileProperty. nsIDOMTreeWalker methods
will be used to process nodes and to add attachment. Or something else. The
result is you have one 'for' statement instead recursive createAttachments()
calls and traversing in cycle through attributes.

https://hotmyfreecams.com/
Looking at the spec
(http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/DOM2-
Traversal-Range.txt), it doesn't look like using a nsIDOMTreeWalker isn't
going to buy us much. It says:

SHOW_ATTRIBUTE
Show Attr nodes. This is meaningful only when creating
an iterator or tree-walker with an attribute node as its
root; in this case, it means that the attribute node
will appear in the first position of the iteration or
traversal. Since attributes are never children of other
nodes, they do not appear when traversing over the
document tree.
https://bestpornsites.mobi
So the dom tree walker won't encounter attributes unless we set the root of
the tree walker to be an attribute. So we couldn't filter out any element
nodes just because they didn't have the upload property since either way
we'd still have to check each element for attributes by hand like we are
currently doing.

ty