Repository: pauldmccarthy/indexed_gzip (language: python)

Issue:
Relax mode check
Hi,

I love the newly added support for Python file-like objects!

I'm currently trying to use that with a file-object returned by `zipfile`, however, I found that `zipfile` only supports `r` and `w` modes but `indexed_gzip` checks that the mode is exactly `rb`. Would it be possible to relax that mode check to also accept `r`? I only see problems if it was something like `rt` or `w` but I'm not sure what the conventions are for file modes in Python. Maybe the zipfile module is wrong for not setting the mode to `rb`. Currently, I'm circumventing this by overwriting the `mode` member manually before forwarding the file object to `indexed_gzip`.

Also, I noticed that the check does not work when specifying the file object as the first argument (the `filename` one). This is because the mode check is done before `filename` is checked whether it is a file object.

Steps to reproduce:

1. Create test file:

```bash
echo foo > bar
gzip -k bar
7z a bar.zip bar.gz
```

2. Try to resolve both compressions through Python:

```python3
import indexed_gzip
import zipfile
zip = zipfile.ZipFile("bar.zip")  # argument: mode='rb' won't work
file = zip.open("bar.gz")
# file.mode='rb'  # Kinda hacky but should work for a while to circumvent the mode check
gz = indexed_gzip.open(fileobj=file)
```

Edit: Actually mypy complains about that hack: `error: Property "mode" defined in "IO" is read-only`, so this is more pressing to me.


Discussion / hints:
Hi @mxmlnkn, hmm, annoying - it seems that the `zipfile` module uses a different convention than the built-in `open` function - for the latter, `'r'` is equivalent to `'rt'`, whereas for a `ZipFile`, and files opened via `ZipFile.open`, `'r'` is equal to `'rb'`.  So it's not possible to use `fileobj.mode` to distinguish between a text or binary stream. I'd have to use `fileobj.peek` or something, but I doubt that `peek` is available on *all* file-likes, and suspect that there's probably no reliable method to determine whether a file-like is text or binary without actually reading some data (which I don't want to do).

But I don't really see the harm in adjusting `indexed_gzip` though -  `mode` parameter accepts `'r'` or `'rb'`, so it makes sense to apply the same check to `fileobj.mode`.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 631e8229..4164d6ec 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,14 @@
 # `indexed_gzip` changelog
 
 
+## 1.6.3 (September 14th 2021)
+
+
+* Relaxed `mode` check when creating an `IndexedGzipFile` from an open file
+  handle - `fileobj.mode` may now be `'r'` or `'rb'` (or may not exist at all)
+  (#85, #86).
+
+
 ## 1.6.2 (September 2nd 2021)
 
 
diff --git a/indexed_gzip/__init__.py b/indexed_gzip/__init__.py
index 49ab8ae9..ad097221 100644
--- a/indexed_gzip/__init__.py
+++ b/indexed_gzip/__init__.py
@@ -19,4 +19,4 @@
 """
 
 
-__version__ = '1.6.2'
+__version__ = '1.6.3'
diff --git a/indexed_gzip/indexed_gzip.pyx b/indexed_gzip/indexed_gzip.pyx
index 14d97ba5..f91ed815 100644
--- a/indexed_gzip/indexed_gzip.pyx
+++ b/indexed_gzip/indexed_gzip.pyx
@@ -288,7 +288,8 @@ cdef class _IndexedGzipFile:
                  skip_crc_check=False):
         """Create an ``_IndexedGzipFile``. The file may be specified either
         with an open file handle (``fileobj``), or with a ``filename``. If the
-        former, the file must have been opened in ``'rb'`` mode.
+        former, the file is assumed have been opened for reading in binary
+        mode.
 
         .. note:: The ``auto_build`` behaviour only takes place on calls to
                   :meth:`seek`.
@@ -337,20 +338,21 @@ cdef class _IndexedGzipFile:
            (fileobj is not None and filename is not None):
             raise ValueError('One of fileobj or filename must be specified')
 
-        if fileobj is not None and getattr(fileobj, 'mode', 'rb') != 'rb':
-            raise ValueError('The gzip file must be opened in '
-                             'read-only binary ("rb") mode')
-
-        if (fileobj is None) and (mode not in (None, 'r', 'rb')):
-            raise ValueError('Invalid mode ({}), must be '
-                             '\'r\' or \'rb\''.format(mode))
-
         # filename can be either a
         # name or a file object
         if  hasattr(filename, 'read'):
             fileobj  = filename
             filename = None
 
+        if fileobj is not None and \
+           getattr(fileobj, 'mode', 'rb') not in (None, 'r', 'rb'):
+            raise ValueError('Invalid mode - fileobj must be opened '
+                             'in read-only binary ("rb") mode')
+
+        if (fileobj is None) and (mode not in (None, 'r', 'rb')):
+            raise ValueError('Invalid mode ({}), must be '
+                             '"r" or "rb"'.format(mode))
+
         # If __file_handle is called on a file
         # that doesn't exist, it passes the
         # path directly to fopen, which causes


Test patch:
diff --git a/indexed_gzip/tests/ctest_indexed_gzip.pyx b/indexed_gzip/tests/ctest_indexed_gzip.pyx
index a0b89f6e..39da7bf9 100644
--- a/indexed_gzip/tests/ctest_indexed_gzip.pyx
+++ b/indexed_gzip/tests/ctest_indexed_gzip.pyx
@@ -112,6 +112,54 @@ def test_open_close_ctxmanager(testfile, nelems, seed, drop):
     assert f.closed
 
 
+def test_open_mode():
+
+    modes = [('r',  True),
+             ('rb', True),
+             (None, True),
+             ('rt', False),
+             ('w',  False),
+             ('wt', False)]
+
+    # open from file
+    with tempdir():
+        with gzip.open('f.gz', 'wb') as f:
+            f.write(b'12345')
+        for mode, expect in modes:
+            if expect:
+                gzf = igzip.IndexedGzipFile('f.gz', mode=mode)
+                assert gzf.read() == b'12345'
+            else:
+                with pytest.raises(ValueError):
+                    igzip.IndexedGzipFile('f.gz', mode=mode)
+
+    # open from fileobj
+    class BytesIOWithMode(BytesIO):
+        pass
+
+    # accept file-like without mode attribute
+    modes.append(('del', True))
+
+    for mode, expect in modes:
+
+        if mode == 'del' and hasattr(BytesIOWithMode, 'mode'):
+            delattr(BytesIOWithMode, 'mode')
+        else:
+            BytesIOWithMode.mode = mode
+
+        fileobj = BytesIOWithMode()
+        with gzip.GzipFile(fileobj=fileobj, mode='wb') as f:
+            f.write(b'12345')
+
+        print(mode, expect)
+
+        if expect:
+            assert igzip.IndexedGzipFile(fileobj=fileobj).read() == b'12345'
+        else:
+            with pytest.raises(ValueError):
+                igzip.IndexedGzipFile(fileobj=fileobj).read()
+
+
 def test_atts(testfile, drop):
 
     modes = [None, 'rb', 'r']
diff --git a/indexed_gzip/tests/test_indexed_gzip.py b/indexed_gzip/tests/test_indexed_gzip.py
index ca9eff65..fde13505 100644
--- a/indexed_gzip/tests/test_indexed_gzip.py
+++ b/indexed_gzip/tests/test_indexed_gzip.py
@@ -30,6 +30,9 @@ def test_open_close_ctxmanager(testfile, nelems, seed):
 def test_open_close_ctxmanager_drop_handles(testfile, nelems, seed):
     ctest_indexed_gzip.test_open_close_ctxmanager(testfile, nelems, seed, True)
 
+def test_open_mode():
+    ctest_indexed_gzip.test_open_mode()
+
 def test_atts(testfile):
     ctest_indexed_gzip.test_atts(testfile, False)