Issue1549

classification
Title: Wrapping an InputStream with a PyFile wrongly carries out line-ending translation.
Type: behaviour Severity: major
Components: Core Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: amak Nosy List: amak, jfenwick, leosoto, pjenvey
Priority: Keywords:

Created on 2010-01-29.16:40:41 by amak, last changed 2010-04-12.22:51:28 by jfenwick.

Files
File name Uploaded Description Edit Remove
PyFileTest.java amak, 2010-01-29.16:40:40
Messages
msg5465 (view) Author: Alan Kennedy (amak) Date: 2010-01-29.16:40:40
Java.io.InputStreams are binary streams, containing bytes. They are used, among other things, for delivering POST data to java servlets, through a ServletInputStream. As such, there should be no line-ending translation on such InputStreams, since they are byte oriented.

But when wrapping an InputStream with a PyFile, through the use of org.python.core.util.FileUtil.wrap(inputstream), line-ending translation takes place. See attached java code which illustrates the issue.

This may only be happening on Windows, where "\r\n" is the standard line ending.

This issue appears to affecting processing of servlet POST data, which by web standards is separated with "\r\n". Again, this may only be happening on Windows.
msg5466 (view) Author: Alan Kennedy (amak) Date: 2010-01-29.16:41:33
Perhaps the correct solution to this problem is to add a mode argument to the FileUtil.wrap() method, so that text or binary mode can be explicitly specified.
msg5468 (view) Author: Leonardo Soto (leosoto) Date: 2010-01-29.23:20:54
I agree with that. I'm cc-ing Phillipe since (AFAIR) he knows more than anyone else about file mode handling on Jython.
msg5483 (view) Author: Jacob Fenwick (jfenwick) Date: 2010-02-02.16:40:20
I'd just like to point out that it's linked to these tickets:
http://bugs.jython.org/issue1171#
http://bugs.jython.org/issue1204

They never resolved Alan's question: Why add the FileUtil class?
It just wraps a call to PyFile anyways. 
And why do I get the "TypeError: coercing to Unicode: need string" error from issue 1204 if I call the PyFile constructor directly?
msg5490 (view) Author: Jacob Fenwick (jfenwick) Date: 2010-02-03.19:26:04
The next three posts will be patches I created to fix the issue.
msg5491 (view) Author: Jacob Fenwick (jfenwick) Date: 2010-02-03.19:29:37
It says "You do not have permission to add a file" when I try to add a file, so I'm pasting all my svn diffs here:

Index: FileUtil.java
===================================================================
--- FileUtil.java	(revision 6976)
+++ FileUtil.java	(working copy)
@@ -18,6 +18,13 @@
 public class FileUtil {
 
     /**
+     * Creates a PyFile that reads from the given <code>InputStream</code> with mode.
+     */
+    public static PyFile wrap(InputStream is, String mode) {
+        return new PyFile(is, "<Java InputStream '" + is + "' as file>", mode, -1, true);    
+    }
+
+    /**
      * Creates a PyFile that reads from the given <code>InputStream</code> with bufsize.
      */
     public static PyFile wrap(InputStream is, int bufsize) {



Index: PyFile.java
===================================================================
--- PyFile.java	(revision 6976)
+++ PyFile.java	(working copy)
@@ -96,7 +96,7 @@
         file___init__(raw, name, mode, bufsize);
     }
 
-    PyFile(InputStream istream, String name, String mode, int bufsize, boolean closefd) {
+    public PyFile(InputStream istream, String name, String mode, int bufsize, boolean closefd) {
         parseMode(mode);
         file___init__(new StreamIO(istream, closefd), name, mode, bufsize);
     }



Index: modjy_wsgi.py
===================================================================
--- modjy_wsgi.py	(revision 6976)
+++ modjy_wsgi.py	(working copy)
@@ -125,7 +125,7 @@
 
     def set_wsgi_streams(self, req, resp, dict):
         try:
-            dict["wsgi.input"]  = create_py_file(req.getInputStream())
+            dict["wsgi.input"]  = create_py_file(req.getInputStream(),"rb")
             dict["wsgi.errors"] =  create_py_file(System.err)
         except IOException, iox:
             raise ModjyIOException(iox)
msg5501 (view) Author: Leonardo Soto (leosoto) Date: 2010-02-05.16:50:24
Alan: are you going to further review the patch or is it ready to commit? 

I think it is fine, but since you reported the bug (and well, you are the modjy guru) I'm deferring to you.
msg5503 (view) Author: Alan Kennedy (amak) Date: 2010-02-05.17:22:32
I think that Philip Jenvey is the right person to review the patch.

Although the bug showed up for someone using modjy, this is really an I/O subsystem issue, and Philip is the authority for that.
msg5523 (view) Author: Philip Jenvey (pjenvey) Date: 2010-02-14.00:14:15
The intention of the patch is sound. My only correction is to add additional constructor methods to PyFile to accomodate this argspec along with the FileUtil wrappers. Even though PyFile has enough constructors, this is mostly to match how the other wrapper methods do it and then we don't have to duplicate the "<Java InputStream"> filename business
msg5683 (view) Author: Alan Kennedy (amak) Date: 2010-04-12.22:05:28
Fix checked in, i.e. Jacob's patch applied, and relevant unit tests added, at revision 7022.

Thanks to Jacob Fenwick for the patch.
msg5684 (view) Author: Alan Kennedy (amak) Date: 2010-04-12.22:06:15
Philip, I'm not sure what you mean in your comments. If you could be a little more specific, I'll add the extra constructors.
msg5685 (view) Author: Jacob Fenwick (jfenwick) Date: 2010-04-12.22:21:02
Note: I have not tested this against trunk because I was unable to get Django-Jython working on trunk. I have only tested successfully against Jython 2.5.1. However, I assume that if I were able to get Django-Jython working on trunk, the patch would still fix this issue.
msg5686 (view) Author: Alan Kennedy (amak) Date: 2010-04-12.22:37:30
I tested it against trunk before I committed it.

Added a unit test for the modjy component as well. We can be sure that the line-endings translation won't be affecting Django again, when trunk is released.
msg5687 (view) Author: Jacob Fenwick (jfenwick) Date: 2010-04-12.22:51:28
Alan, I think what Philip wants is a simpler wrapper function that moves the name of the object into the PyFile object. The name is what is printed out in Java when you pass the object to System.out.println.

Here is a possible fix:

FileUtil.java:
    public static PyFile wrap(InputStream is, String mode, int bufsize) {
        return new PyFile(is, mode, bufsize);    
    }

This would have a corresponding PyFile entry, which would call the PyFile entry from the patch:

PyFile.java:
    public PyFile(InputStream istream, String mode, int bufsize) {
        PyFile(istream, "<Java InputStream '" + istream + "' as file>", mode, bufsize, true)
    }


Note 1: I took the liberty of adding bufsize to the wrapper function, as most of the other wrapper functions in FileUtil have bufsize, so I assume it's something people want often.

Note 2: I didn't compile or try this, so please don't just check it in, try it.
History
Date User Action Args
2010-04-12 22:51:28jfenwicksetmessages: + msg5687
2010-04-12 22:37:31amaksetmessages: + msg5686
2010-04-12 22:21:02jfenwicksetmessages: + msg5685
2010-04-12 22:06:15amaksetmessages: + msg5684
2010-04-12 22:05:28amaksetstatus: open -> closed
assignee: amak
resolution: fixed
messages: + msg5683
2010-02-14 00:14:16pjenveysetmessages: + msg5523
2010-02-05 17:22:32amaksetmessages: + msg5503
2010-02-05 16:50:24leosotosetmessages: + msg5501
2010-02-03 19:29:38jfenwicksetmessages: + msg5491
2010-02-03 19:26:04jfenwicksetmessages: + msg5490
2010-02-02 16:40:20jfenwicksetmessages: + msg5483
2010-01-29 23:20:54leosotosetnosy: + pjenvey, leosoto
messages: + msg5468
2010-01-29 20:16:17jfenwicksetnosy: + jfenwick
2010-01-29 17:23:24amaksettype: behaviour
2010-01-29 16:41:33amaksetmessages: + msg5466
2010-01-29 16:40:42amakcreate