Re: [Patch] (swig-py) accept core.svn_stream_t object for svn_stream_t *

On 12/6/18 5:06 PM, Yasuhito FUTATSUKI wrote:
> Hi, I found SWIP Python bindings of APIs using svn_stream_t * in _client,
> _delta, _diff, _fs, _ra and _repos don't accept core.svn_stream_t proxy
> objects, so I modified %typemap(in) svn_stream_t *WRAPPED_STRAM to accept
> them as well as file like objects.

Those who try to use APIs to pass svn_stream_t * from Python may try to
pass core.svn_stream_t object from other API returns it, because as there is
no appropriate document public available to describe what can be acceptable
for those APIs, so it is natural to do so. Actually I found the code below
in ViewVC:

   temp = tempfile.mktemp()
   stream = core.svn_stream_from_aprfile(temp)
   url = svnrepos._geturl(path)
   client.svn_client_cat(core.Stream(stream), url, _rev2optrev(rev),
                         svnrepos.ctx)
   core.svn_stream_close(stream)
   return temp

I think the author of this code try to pass 'stream' to client.svn_client_cat
directly and got exception, then wrap with core.Stream() to avoid it.
That's why I tried to modify this typemap.

Jun Omae kindly reviewed and rewrote my patch to move code to check object
type into svn_swig_py_make_stream() in swigutil_py.c to minimize expansion
of typemap, and added test for parse_fns3_invalid_set_fulltext() in
swigutil_py.c which is affected by modification of svn_swig_py_make_stream().
(https://github.com/jun66j5/subversion/commits/improve_swig_py_stream_IF)
I also add few modification after it, so I repost modified patch.

--- Start of patch ---
diff --git a/subversion/bindings/swig/include/svn_types.swg b/subversion/bindings/swig/include/svn_types.swg
index 4ad5d1658b..ad66cb160c 100644
--- a/subversion/bindings/swig/include/svn_types.swg
+++ b/subversion/bindings/swig/include/svn_types.swg
@@ -941,7 +941,15 @@ svn_ ## TYPE ## _swig_rb_closed(VALUE self)
  
  #ifdef SWIGPYTHON
  %typemap(in) svn_stream_t *WRAPPED_STREAM {
- $1 = svn_swig_py_make_stream ($input, _global_pool);
+ if ($input == Py_None) {
+ $1 = NULL;
+ }
+ else {
+ $1 = svn_swig_py_make_stream ($input, _global_pool);
+ if ($1 == NULL) {
+ SWIG_fail;
+ }
+ }
  }
  #endif
  
diff --git a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
index 27ee404269..392190a9f5 100644
--- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
+++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
@@ -2293,8 +2293,8 @@ static svn_error_t *parse_fn3_set_fulltext(svn_stream_t **stream,
                                             void *node_baton)
  {
    item_baton *ib = node_baton;
- PyObject *result;
- svn_error_t *err;
+ PyObject *result = NULL;
+ svn_error_t *err = SVN_NO_ERROR;
  
    svn_swig_py_acquire_py_lock();
  
@@ -2316,14 +2316,17 @@ static svn_error_t *parse_fn3_set_fulltext(svn_stream_t **stream,
        /* create a stream from the IO object. it will increment the
           reference on the 'result'. */
        *stream = svn_swig_py_make_stream(result, ib->pool);
+ if (*stream == NULL)
+ {
+ err = callback_exception_error();
+ goto finished;
+ }
      }
  
+ finished:
    /* if the handler returned an IO object, svn_swig_py_make_stream() has
       incremented its reference counter. If it was None, it is discarded. */
- Py_DECREF(result);
- err = SVN_NO_ERROR;
-
- finished:
+ Py_XDECREF(result);
    svn_swig_py_release_py_lock();
    return err;
  }
@@ -2575,17 +2578,59 @@ svn_swig_py_stream_destroy(void *py_io)
  svn_stream_t *
  svn_swig_py_make_stream(PyObject *py_io, apr_pool_t *pool)
  {
- svn_stream_t *stream;
+ PyObject *libsvn_core = NULL;
+ PyObject *py_stream_t = NULL;
+ PyObject *_stream = NULL;
+ svn_stream_t *result = NULL;
+ swig_type_info *typeinfo = svn_swig_TypeQuery("svn_stream_t *");
  
- stream = svn_stream_create(py_io, pool);
- svn_stream_set_read2(stream, read_handler_pyio, NULL);
- svn_stream_set_write(stream, write_handler_pyio);
- svn_stream_set_close(stream, close_handler_pyio);
- apr_pool_cleanup_register(pool, py_io, svn_swig_py_stream_destroy,
- apr_pool_cleanup_null);
- Py_INCREF(py_io);
+ libsvn_core = PyImport_ImportModule("libsvn.core");
+ if (PyErr_Occurred()) {
+ goto finished;
+ }
+ py_stream_t = PyObject_GetAttrString(libsvn_core, "svn_stream_t");
+ if (PyErr_Occurred()) {
+ goto finished;
+ }
+ if (PyObject_IsInstance(py_io, py_stream_t)) {
+ result = (svn_stream_t *)svn_swig_py_must_get_ptr(py_io, typeinfo, 0);
+ if (PyErr_Occurred()) {
+ result = NULL;
+ goto finished;
+ }
+ }
+ else if (PyObject_HasAttrString(py_io, "_stream")) {
+ _stream = PyObject_GetAttrString(py_io, "_stream");
+ if (PyObject_IsInstance(_stream, py_stream_t)) {
+ result = (svn_stream_t *)svn_swig_py_must_get_ptr(_stream, typeinfo, 0);
+ if (PyErr_Occurred()) {
+ result = NULL;
+ goto finished;
+ }
+ }
+ }
+ if (result == NULL) {
+ if ( !PyObject_HasAttrString(py_io, "read")
+ && !PyObject_HasAttrString(py_io, "write")) {
+ PyErr_SetString(PyExc_TypeError,
+ "expecting a svn_stream_t or file like object");
+ goto finished;
+ }
+ result = svn_stream_create(py_io, pool);
+ svn_stream_set_read2(result, read_handler_pyio, NULL);
+ svn_stream_set_write(result, write_handler_pyio);
+ svn_stream_set_close(result, close_handler_pyio);
+ apr_pool_cleanup_register(pool, py_io, svn_swig_py_stream_destroy,
+ apr_pool_cleanup_null);
+ Py_INCREF(py_io);
+ }
+
+finished:
+ Py_XDECREF(_stream);
+ Py_XDECREF(py_stream_t);
+ Py_XDECREF(libsvn_core);
  
- return stream;
+ return result;
  }
  
  PyObject *
diff --git a/subversion/bindings/swig/python/tests/delta.py b/subversion/bindings/swig/python/tests/delta.py
index 162cf322de..7f6ba6782f 100644
--- a/subversion/bindings/swig/python/tests/delta.py
+++ b/subversion/bindings/swig/python/tests/delta.py
@@ -19,6 +19,8 @@
  #
  #
  import unittest, setup_path
+import os
+import tempfile
  import svn.delta
  import svn.core
  from sys import version_info # For Python version check
@@ -47,6 +49,59 @@ class DeltaTestCase(unittest.TestCase):
         svn.delta.tx_apply(src_stream, target_stream, None)
      window_handler(None, baton)
  
+ def testTxWindowHandler_stream_IF(self):
+ """Test tx_invoke_window_handler, with svn.core.svn_stream_t object"""
+ pool = svn.core.Pool()
+ in_str = "hello world"
+ src_stream = svn.core.svn_stream_from_stringbuf(in_str)
+ content_str = "bye world"
+ content_stream = svn.core.svn_stream_from_stringbuf(content_str)
+ fd, fname = tempfile.mkstemp()
+ os.close(fd)
+ try:
+ target_stream = svn.core.svn_stream_from_aprfile2(fname, False)
+ window_handler, baton = \
+ svn.delta.tx_apply(src_stream, target_stream, None)
+ svn.delta.tx_send_stream(content_stream, window_handler, baton, pool)
+ fp = open(fname, 'rb')
+ out_str = fp.read()
+ fp.close()
+ self.assertEqual(content_str, out_str)
+ finally:
+ del pool
+ try:
+ os.remove(fname)
+ except OSError:
+ pass
+
+ def testTxWindowHandler_Stream_IF(self):
+ """Test tx_invoke_window_handler, with svn.core.Stream object"""
+ pool = svn.core.Pool()
+ in_str = "hello world"
+ src_stream = svn.core.Stream(
+ svn.core.svn_stream_from_stringbuf(in_str))
+ content_str = "bye world"
+ content_stream = svn.core.Stream(
+ svn.core.svn_stream_from_stringbuf(content_str))
+ fd, fname = tempfile.mkstemp()
+ os.close(fd)
+ try:
+ target_stream = svn.core.Stream(
+ svn.core.svn_stream_from_aprfile2(fname, False))
+ window_handler, baton = \
+ svn.delta.tx_apply(src_stream, target_stream, None)
+ svn.delta.tx_send_stream(content_stream, window_handler, baton, None)
+ fp = open(fname, 'rb')
+ out_str = fp.read()
+ fp.close()
+ self.assertEqual(content_str, out_str)
+ finally:
+ del pool
+ try:
+ os.remove(fname)
+ except OSError:
+ pass
+
    def testTxdeltaWindowT(self):
      """Test the svn_txdelta_window_t wrapper."""
      a = StringIO("abc\ndef\n")
diff --git a/subversion/bindings/swig/python/tests/repository.py b/subversion/bindings/swig/python/tests/repository.py
index 46580b723a..c185c508c3 100644
--- a/subversion/bindings/swig/python/tests/repository.py
+++ b/subversion/bindings/swig/python/tests/repository.py
@@ -231,6 +231,26 @@ class SubversionRepositoryTestCase(unittest.TestCase):
      # the comparison list gets too long.
      self.assertEqual(dsp.ops[:len(expected_list)], expected_list)
  
+ def test_parse_fns3_invalid_set_fulltext(self):
+ self.cancel_calls = 0
+ def is_cancelled():
+ self.cancel_calls += 1
+ return None
+ class DumpStreamParserSubclass(DumpStreamParser):
+ def set_fulltext(self, node_baton):
+ DumpStreamParser.set_fulltext(self, node_baton)
+ return 42
+ dump_path = os.path.join(os.path.dirname(sys.argv[0]),
+ "trac/versioncontrol/tests/svnrepos.dump")
+ stream = open(dump_path)
+ try:
+ dsp = DumpStreamParserSubclass()
+ ptr, baton = repos.make_parse_fns3(dsp)
+ self.assertRaises(TypeError, repos.parse_dumpstream3,
+ stream, ptr, baton, False, is_cancelled)
+ finally:
+ stream.close()
+
    def test_get_logs(self):
      """Test scope of get_logs callbacks"""
      logs = []
--- End of patch ---

Regards,

-- 
Yasuhito FUTATSUKI