Last Comment Bug 353677 - serialization using method="multipart-post" doesn't serialize parts
: serialization using method="multipart-post" doesn't serialize parts
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
Mentors:
http://www.w3.org/TR/xforms/slice11.h...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-21 11:07 PDT by Steve Speicher
Modified: 2007-04-23 16:00 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.95 KB, application/xhtml+xml)
2006-09-21 11:08 PDT, Steve Speicher
no flags Details
Simple XML file to submit (86 bytes, text/xml)
2006-12-07 15:59 PST, Merle Sterling
no flags Details
patch (4.25 KB, patch)
2006-12-07 16:08 PST, Merle Sterling
aaronr: review-
Details | Diff | Review
testcase2 (2.05 KB, application/xhtml+xml)
2006-12-08 00:12 PST, aaronr
no flags Details
patch2 (12.21 KB, patch)
2006-12-10 14:58 PST, aaronr
bugs: review+
Details | Diff | Review
patch3 (12.48 KB, patch)
2006-12-10 16:36 PST, aaronr
surkov.alexander: review+
Details | Diff | Review

Description Steve Speicher 2006-09-21 11:07:28 PDT
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 2006-09-21 11:08:40 PDT
Created attachment 239533 [details]
testcase
Comment 2 Steve Speicher 2006-09-21 11:10:51 PDT
Here's what gets submitted with Moz:
-----------------------------24464570528145
Content-Type: application/xml
Content-ID: <5af1.41bb@mozilla.org>

<?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: <26e9.1eb@mozilla.org>


-----------------------------24464570528145
Content-Type: application/octet-stream
Content-Transfer-Encoding: binary
Content-ID: <bb3.2ea6@mozilla.org>


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

Here's what gets submitted with X-Smiles:
--------xs1q2w3e4r5t6y7u8i50180
Content-Type: application/xml
Content-ID: <content-start35771@xsmiles.org>
<?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:X432584@xsmiles.org</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:X432584@xsmiles.org>
<?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 2006-12-07 13:54:00 PST
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 2006-12-07 15:59:22 PST
Created attachment 247910 [details]
Simple XML file to submit
Comment 5 Merle Sterling 2006-12-07 16:08:09 PST
Created attachment 247911 [details] [diff] [review]
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 2006-12-08 00:10:43 PST
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 2006-12-08 00:12:26 PST
Created attachment 247929 [details]
testcase2

testcase where upload is bound to an attribute node in the instance data.
Comment 8 aaronr 2006-12-10 14:58:28 PST
Created attachment 248189 [details] [diff] [review]
patch2

Fixed up Merle's patch to properly handle when upload binds to attributes.
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2006-12-10 15:41:14 PST
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 2006-12-10 16:36:03 PST
Created attachment 248195 [details] [diff] [review]
patch3

adds attribute comment per smaug's review comment.
Comment 11 alexander :surkov 2006-12-11 00:25:11 PST
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 2006-12-11 09:22:13 PST
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 2006-12-11 11:35:45 PST
(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 2006-12-11 11:36:48 PST
(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 2006-12-12 01:21:04 PST
(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 2006-12-12 01:33:33 PST
(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 2007-01-23 15:16:10 PST
(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 2007-01-23 15:29:56 PST
(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 2007-01-23 15:33:36 PST
Comment on attachment 248195 [details] [diff] [review]
patch3

In lieu of my last two comments, does this pass the muster?
Comment 20 alexander :surkov 2007-01-24 06:00:10 PST
Comment on attachment 248195 [details] [diff] [review]
patch3

r=me, sorry about traversal :)
Comment 21 aaronr 2007-01-24 12:53:28 PST
checked into trunk for msterlin
Comment 22 aaronr 2007-04-23 16:00:04 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

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