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

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
11 months ago

People

(Reporter: Steve Speicher, Assigned: Merle Sterling)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
fixed1.8.0.12, fixed1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

1.95 KB, application/xhtml+xml
Details
86 bytes, text/xml
Details
2.05 KB, application/xhtml+xml
Details
12.48 KB, patch
surkov
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 239533 [details]
testcase
(Reporter)

Comment 2

11 years ago
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
(Assignee)

Comment 3

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



(Assignee)

Comment 4

11 years ago
Created attachment 247910 [details]
Simple XML file to submit
(Assignee)

Comment 5

11 years ago
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.
Attachment #247911 - Flags: review?(aaronr)

Updated

11 years ago
Assignee: xforms → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

11 years ago
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.
Attachment #247911 - Flags: review?(aaronr) → review-

Comment 7

11 years ago
Created attachment 247929 [details]
testcase2

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

Comment 8

11 years ago
Created attachment 248189 [details] [diff] [review]
patch2

Fixed up Merle's patch to properly handle when upload binds to attributes.
Attachment #247911 - Attachment is obsolete: true
Attachment #248189 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #248189 - Flags: review?(surkov.alexander)

Comment 9

11 years ago
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);
       }
Attachment #248189 - Flags: review?(Olli.Pettay) → review+

Comment 10

11 years ago
Created attachment 248195 [details] [diff] [review]
patch3

adds attribute comment per smaug's review comment.
Attachment #248189 - Attachment is obsolete: true
Attachment #248195 - Flags: review?(surkov.alexander)
Attachment #248189 - Flags: review?(surkov.alexander)

Comment 11

11 years ago
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

11 years ago
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.
Attachment #248195 - Flags: review?(surkov.alexander)

Comment 13

11 years ago
(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

11 years ago
(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

11 years ago
(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

11 years ago
(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

11 years ago
(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

11 years ago
(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

11 years ago
Comment on attachment 248195 [details] [diff] [review]
patch3

In lieu of my last two comments, does this pass the muster?
Attachment #248195 - Flags: review?(surkov.alexander)

Comment 20

11 years ago
Comment on attachment 248195 [details] [diff] [review]
patch3

r=me, sorry about traversal :)
Attachment #248195 - Flags: review?(surkov.alexander) → review+

Comment 21

11 years ago
checked into trunk for msterlin
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 22

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.