Closed Bug 353677 Opened 18 years ago Closed 17 years ago

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

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sspeiche, Assigned: msterlin)

References

()

Details

(Keywords: fixed1.8.0.12, fixed1.8.1.4)

Attachments

(4 files, 2 obsolete files)

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
Attached file testcase
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
TCPMon: listen port = 8081; targetHostname = www.xformstest.org; targetPort=80
xforms:submission action=http://localhost:8081/cgi-bin/echo.sh or showinstance.sh



Attached patch patch (obsolete) — Splinter Review
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)
Assignee: xforms → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
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-
Attached file testcase2
testcase where upload is bound to an attribute node in the instance data.
Attached patch patch2 (obsolete) — Splinter Review
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)
Attachment #248189 - Flags: review?(surkov.alexander)
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+
Attached patch patch3Splinter Review
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 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 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)
(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?
(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?
(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.
(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.
(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.
(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 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 on attachment 248195 [details] [diff] [review]
patch3

r=me, sorry about traversal :)
Attachment #248195 - Flags: review?(surkov.alexander) → review+
checked into trunk for msterlin
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Whiteboard: xf-to-branch
Product: Core → Core Graveyard

(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

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

Attachment

General

Creator:
Created:
Updated:
Size: