Repository: python/cpython
Pull request: gh-92019: Make sqlite3.Blob indexing conform with the norm
- get index now returns an int
- set index now requires an int in range(0, 256)

Resolves #92019

<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
gh-NNNNN: Summary of the changes made
```

Where: gh-NNNNN refers to the GitHub issue number.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `main`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNN)
```

Where: [X.Y] is the branch name, e.g. [3.6].

GH-NNNN refers to the PR number from `main`.

-->


Problem statement:
sqlite3.Blob indexing is not conformant with existing norms
Yesterday, @JelleZijlstra enlightened me that sqlite3.Blob indexing operate with `bytes` objects of length 1. This is a deviation from how `bytes`, `bytearray`, and `memoryview` work:

```
>>> bytes(b"0")[0]
48
>>> bytearray(b"0")[0]
48
>>> memoryview(b"0")[0]
48
>>> b = bytearray(b"0")
>>> b[0] = 1
>>> b[0] = b"1"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'bytes' object cannot be interpreted as an integer
>>> m = memoryview(b)
>>> m[0] = 1
>>> m[0] = b"1"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: memoryview: invalid type for format 'B'
>>> 
```

Suggesting to align sqlite3.Blob indexing behaviour so it expects `int`s and returns `int`s.

Hints from issue discussion:

Commit message: gh-92019: Make sqlite3.Blob indexing conform with the norm

- get index now returns an int
- set index now requires an int in range(0, 256)

Patch to review:
diff --git a/Doc/includes/sqlite3/blob.py b/Doc/includes/sqlite3/blob.py
index d947059b3ae647..ff58d6c352b652 100644
--- a/Doc/includes/sqlite3/blob.py
+++ b/Doc/includes/sqlite3/blob.py
@@ -9,8 +9,8 @@
     blob.write(b"hello, ")
     blob.write(b"world.")
     # Modify the first and last bytes of our blob
-    blob[0] = b"H"
-    blob[-1] = b"!"
+    blob[0] = ord("H")
+    blob[-1] = ord("!")
 
 # Read the contents of our blob
 with con.blobopen("test", "blob_col", 1) as blob:
diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py
index 8bfdce2bbe92e4..7e675a91b5d1fd 100644
--- a/Lib/test/test_sqlite3/test_dbapi.py
+++ b/Lib/test/test_sqlite3/test_dbapi.py
@@ -1173,14 +1173,14 @@ def test_blob_length(self):
         self.assertEqual(len(self.blob), 50)
 
     def test_blob_get_item(self):
-        self.assertEqual(self.blob[5], b"b")
-        self.assertEqual(self.blob[6], b"l")
-        self.assertEqual(self.blob[7], b"o")
-        self.assertEqual(self.blob[8], b"b")
-        self.assertEqual(self.blob[-1], b"!")
+        self.assertEqual(self.blob[5], ord("b"))
+        self.assertEqual(self.blob[6], ord("l"))
+        self.assertEqual(self.blob[7], ord("o"))
+        self.assertEqual(self.blob[8], ord("b"))
+        self.assertEqual(self.blob[-1], ord("!"))
 
     def test_blob_set_item(self):
-        self.blob[0] = b"b"
+        self.blob[0] = ord("b")
         expected = b"b" + self.data[1:]
         actual = self.cx.execute("select b from test").fetchone()[0]
         self.assertEqual(actual, expected)
@@ -1188,23 +1188,14 @@ def test_blob_set_item(self):
     def test_blob_set_item_with_offset(self):
         self.blob.seek(0, SEEK_END)
         self.assertEqual(self.blob.read(), b"")  # verify that we're at EOB
-        self.blob[0] = b"T"
-        self.blob[-1] = b"."
+        self.blob[0] = ord("T")
+        self.blob[-1] = ord(".")
         self.blob.seek(0, SEEK_SET)
         expected = b"This blob data string is exactly fifty bytes long."
         self.assertEqual(self.blob.read(), expected)
 
-    def test_blob_set_buffer_object(self):
+    def test_blob_set_slice_buffer_object(self):
         from array import array
-        self.blob[0] = memoryview(b"1")
-        self.assertEqual(self.blob[0], b"1")
-
-        self.blob[1] = bytearray(b"2")
-        self.assertEqual(self.blob[1], b"2")
-
-        self.blob[2] = array("b", [4])
-        self.assertEqual(self.blob[2], b"\x04")
-
         self.blob[0:5] = memoryview(b"12345")
         self.assertEqual(self.blob[0:5], b"12345")
 
@@ -1215,8 +1206,8 @@ def test_blob_set_buffer_object(self):
         self.assertEqual(self.blob[0:5], b"\x01\x02\x03\x04\x05")
 
     def test_blob_set_item_negative_index(self):
-        self.blob[-1] = b"z"
-        self.assertEqual(self.blob[-1], b"z")
+        self.blob[-1] = 255
+        self.assertEqual(self.blob[-1], 255)
 
     def test_blob_get_slice(self):
         self.assertEqual(self.blob[5:14], b"blob data")
@@ -1264,13 +1255,29 @@ def test_blob_get_item_error(self):
         with self.assertRaisesRegex(IndexError, "cannot fit 'int'"):
             self.blob[ULLONG_MAX]
 
+        # Provoke read error
+        self.cx.execute("update test set b='aaaa' where rowid=1")
+        with self.assertRaises(sqlite.OperationalError):
+            self.blob[0]
+
     def test_blob_set_item_error(self):
-        with self.assertRaisesRegex(ValueError, "must be a single byte"):
+        with self.assertRaisesRegex(TypeError, "cannot be interpreted"):
             self.blob[0] = b"multiple"
+        with self.assertRaisesRegex(TypeError, "cannot be interpreted"):
+            self.blob[0] = b"1"
+        with self.assertRaisesRegex(TypeError, "cannot be interpreted"):
+            self.blob[0] = bytearray(b"1")
         with self.assertRaisesRegex(TypeError, "doesn't support.*deletion"):
             del self.blob[0]
         with self.assertRaisesRegex(IndexError, "Blob index out of range"):
-            self.blob[1000] = b"a"
+            self.blob[1000] = 0
+        with self.assertRaisesRegex(ValueError, "must be in range"):
+            self.blob[0] = -1
+        with self.assertRaisesRegex(ValueError, "must be in range"):
+            self.blob[0] = 256
+        # Overflow errors are overridden with ValueError
+        with self.assertRaisesRegex(ValueError, "must be in range"):
+            self.blob[0] = 2**65
 
     def test_blob_set_slice_error(self):
         with self.assertRaisesRegex(IndexError, "wrong size"):
diff --git a/Modules/_sqlite/blob.c b/Modules/_sqlite/blob.c
index 0c57ff8ca4252b..d179e09540e03d 100644
--- a/Modules/_sqlite/blob.c
+++ b/Modules/_sqlite/blob.c
@@ -120,7 +120,25 @@ blob_seterror(pysqlite_Blob *self, int rc)
 }
 
 static PyObject *
-inner_read(pysqlite_Blob *self, Py_ssize_t length, Py_ssize_t offset)
+read_single(pysqlite_Blob *self, Py_ssize_t offset)
+{
+    assert(offset <= sqlite3_blob_bytes(self->blob));
+
+    unsigned long buf;
+    int rc;
+    Py_BEGIN_ALLOW_THREADS
+    rc = sqlite3_blob_read(self->blob, (void *)&buf, 1, (int)offset);
+    Py_END_ALLOW_THREADS
+
+    if (rc != SQLITE_OK) {
+        blob_seterror(self, rc);
+        return NULL;
+    }
+    return PyLong_FromUnsignedLong(buf);
+}
+
+static PyObject *
+read_multiple(pysqlite_Blob *self, Py_ssize_t length, Py_ssize_t offset)
 {
     assert(length <= sqlite3_blob_bytes(self->blob));
     assert(offset <= sqlite3_blob_bytes(self->blob));
@@ -175,7 +193,7 @@ blob_read_impl(pysqlite_Blob *self, int length)
         length = max_read_len;
     }
 
-    PyObject *buffer = inner_read(self, length, self->offset);
+    PyObject *buffer = read_multiple(self, length, self->offset);
     if (buffer == NULL) {
         return NULL;
     }
@@ -387,7 +405,7 @@ subscript_index(pysqlite_Blob *self, PyObject *item)
     if (i < 0) {
         return NULL;
     }
-    return inner_read(self, 1, i);
+    return read_single(self, i);
 }
 
 static int
@@ -411,9 +429,9 @@ subscript_slice(pysqlite_Blob *self, PyObject *item)
     }
 
     if (step == 1) {
-        return inner_read(self, len, start);
+        return read_multiple(self, len, start);
     }
-    PyObject *blob = inner_read(self, stop - start, start);
+    PyObject *blob = read_multiple(self, stop - start, start);
     if (blob == NULL) {
         return NULL;
     }
@@ -455,24 +473,27 @@ ass_subscript_index(pysqlite_Blob *self, PyObject *item, PyObject *value)
                         "Blob doesn't support item deletion");
         return -1;
     }
+    if (!PyLong_Check(value)) {
+        PyErr_Format(PyExc_TypeError,
+                     "'%s' object cannot be interpreted as an integer",
+                     Py_TYPE(value)->tp_name);
+        return -1;
+    }
     Py_ssize_t i = get_subscript_index(self, item);
     if (i < 0) {
         return -1;
     }
 
-    Py_buffer vbuf;
-    if (PyObject_GetBuffer(value, &vbuf, PyBUF_SIMPLE) < 0) {
-        return -1;
+    long val = PyLong_AsLong(value);
+    if (val == -1 && PyErr_Occurred()) {
+        PyErr_Clear();
+        val = -1;
     }
-    int rc = -1;
-    if (vbuf.len != 1) {
-        PyErr_SetString(PyExc_ValueError, "Blob assignment must be a single byte");
-    }
-    else {
-        rc = inner_write(self, (const char *)vbuf.buf, 1, i);
+    if (val < 0 || val > 255) {
+        PyErr_SetString(PyExc_ValueError, "byte must be in range(0, 256)");
+        return -1;
     }
-    PyBuffer_Release(&vbuf);
-    return rc;
+    return inner_write(self, (const void *)&val, 1, i);
 }
 
 static int
@@ -507,7 +528,7 @@ ass_subscript_slice(pysqlite_Blob *self, PyObject *item, PyObject *value)
         rc = inner_write(self, vbuf.buf, len, start);
     }
     else {
-        PyObject *blob_bytes = inner_read(self, stop - start, start);
+        PyObject *blob_bytes = read_multiple(self, stop - start, start);
         if (blob_bytes != NULL) {
             char *blob_buf = PyBytes_AS_STRING(blob_bytes);
             for (Py_ssize_t i = 0, j = 0; i < len; i++, j += step) {


Code review comments:
diff_hunk: @@ -120,7 +120,25 @@ blob_seterror(pysqlite_Blob *self, int rc)
 }
 
 static PyObject *
-inner_read(pysqlite_Blob *self, Py_ssize_t length, Py_ssize_t offset)
+read_single(pysqlite_Blob *self, Py_ssize_t offset)
+{
+    assert(offset <= sqlite3_blob_bytes(self->blob));
path: Modules/_sqlite/blob.c
text: @user1:
This can be `<`, right? I was looking at this code last night and noticed this. I think inner_read also has a `<=` where it could be `<`.

@author:
Yeah, good catch. I prefer to handle this in a separate PR / issue. Are you ok with that?

@author:
...or we could include it here; here's the patch:

<details>

```diff
diff --git a/Modules/_sqlite/blob.c b/Modules/_sqlite/blob.c
index d7045f030e..6d28151cc0 100644
--- a/Modules/_sqlite/blob.c
+++ b/Modules/_sqlite/blob.c
@@ -122,8 +122,6 @@ blob_seterror(pysqlite_Blob *self, int rc)
 static PyObject *
 read_single(pysqlite_Blob *self, Py_ssize_t offset)
 {
-    assert(offset <= sqlite3_blob_bytes(self->blob));
-
     unsigned long buf = 0;
     int rc;
     Py_BEGIN_ALLOW_THREADS
@@ -141,7 +139,7 @@ static PyObject *
 read_multiple(pysqlite_Blob *self, Py_ssize_t length, Py_ssize_t offset)
 {
     assert(length <= sqlite3_blob_bytes(self->blob));
-    assert(offset <= sqlite3_blob_bytes(self->blob));
+    assert(offset < sqlite3_blob_bytes(self->blob));
 
     PyObject *buffer = PyBytes_FromStringAndSize(NULL, length);
     if (buffer == NULL) {
@@ -193,6 +191,11 @@ blob_read_impl(pysqlite_Blob *self, int length)
         length = max_read_len;
     }
 
+    assert(length >= 0);
+    if (length == 0) {
+        return PyBytes_FromStringAndSize(NULL, 0);
+    }
+
     PyObject *buffer = read_multiple(self, length, self->offset);
     if (buffer == NULL) {
         return NULL;
```

</details>

@user1:
Either is fine with me, I think it's small enough to just put it in here.

@author:
All right, I put it in here :)

@user1:
Looks like you didn't push?

@author:
I did; look again ;)