From c72e7b0585edf515506a66c1b6b150315b81757d Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Tue, 1 Feb 2022 10:20:23 -0500
Subject: [PATCH 1/8] Implement HTTP share listing endpoint.

---
 docs/proposed/http-storage-node-protocol.rst |  4 +--
 src/allmydata/storage/http_client.py         | 29 ++++++++++++++----
 src/allmydata/storage/http_server.py         | 16 ++++++++++
 src/allmydata/test/test_storage_http.py      | 31 ++++++++++++++++++++
 4 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst
index 560220d00..f47a50d3e 100644
--- a/docs/proposed/http-storage-node-protocol.rst
+++ b/docs/proposed/http-storage-node-protocol.rst
@@ -630,8 +630,8 @@ Reading
 ``GET /v1/immutable/:storage_index/shares``
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
-Retrieve a list indicating all shares available for the indicated storage index.
-For example::
+Retrieve a list (semantically, a set) indicating all shares available for the
+indicated storage index. For example::
 
   [1, 5]
 
diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py
index d4837d4ab..8a1d03192 100644
--- a/src/allmydata/storage/http_client.py
+++ b/src/allmydata/storage/http_client.py
@@ -136,6 +136,7 @@ class UploadProgress(object):
     """
     Progress of immutable upload, per the server.
     """
+
     # True when upload has finished.
     finished = attr.ib(type=bool)
     # Remaining ranges to upload.
@@ -221,7 +222,7 @@ class StorageClientImmutables(object):
             headers=Headers(
                 {
                     "content-range": [
-                        ContentRange("bytes", offset, offset+len(data)).to_header()
+                        ContentRange("bytes", offset, offset + len(data)).to_header()
                     ]
                 }
             ),
@@ -268,11 +269,7 @@ class StorageClientImmutables(object):
             "GET",
             url,
             headers=Headers(
-                {
-                    "range": [
-                        Range("bytes", [(offset, offset + length)]).to_header()
-                    ]
-                }
+                {"range": [Range("bytes", [(offset, offset + length)]).to_header()]}
             ),
         )
         if response.code == http.PARTIAL_CONTENT:
@@ -282,3 +279,23 @@ class StorageClientImmutables(object):
             raise ClientException(
                 response.code,
             )
+
+    @inlineCallbacks
+    def list_shares(self, storage_index):  # type: (bytes,) -> Deferred[Set[int]]
+        """
+        Return the set of shares for a given storage index.
+        """
+        url = self._client._url(
+            "/v1/immutable/{}/shares".format(_encode_si(storage_index))
+        )
+        response = yield self._client._request(
+            "GET",
+            url,
+        )
+        if response.code == http.OK:
+            body = yield response.content()
+            returnValue(set(loads(body)))
+        else:
+            raise ClientException(
+                response.code,
+            )
diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index d79e9a38b..f4ba865ca 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -255,6 +255,22 @@ class HTTPServer(object):
             required.append({"begin": start, "end": end})
         return self._cbor(request, {"required": required})
 
+    @_authorized_route(
+        _app,
+        set(),
+        "/v1/immutable/<string:storage_index>/shares",
+        methods=["GET"],
+    )
+    def list_shares(self, request, authorization, storage_index):
+        """
+        List shares for the given storage index.
+        """
+        storage_index = si_a2b(storage_index.encode("ascii"))
+
+        # TODO in future ticket, handle KeyError as 404
+        share_numbers = list(self._storage_server.get_buckets(storage_index).keys())
+        return self._cbor(request, share_numbers)
+
     @_authorized_route(
         _app,
         set(),
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index dcefc9950..4ddfff62e 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -382,6 +382,37 @@ class ImmutableHTTPAPITests(SyncTestCase):
             )
             self.assertEqual(downloaded, expected_data[offset : offset + length])
 
+    def test_list_shares(self):
+        """
+        Once a share is finished uploading, it's possible to list it.
+        """
+        im_client = StorageClientImmutables(self.http.client)
+        upload_secret = urandom(32)
+        lease_secret = urandom(32)
+        storage_index = b"".join(bytes([i]) for i in range(16))
+        result_of(
+            im_client.create(
+                storage_index, {1, 2, 3}, 10, upload_secret, lease_secret, lease_secret
+            )
+        )
+
+        # Initially there are no shares:
+        self.assertEqual(result_of(im_client.list_shares(storage_index)), set())
+
+        # Upload shares 1 and 3:
+        for share_number in [1, 3]:
+            progress = result_of(im_client.write_share_chunk(
+                    storage_index,
+                    share_number,
+                    upload_secret,
+                    0,
+                    b"0123456789",
+            ))
+            self.assertTrue(progress.finished)
+
+        # Now shares 1 and 3 exist:
+        self.assertEqual(result_of(im_client.list_shares(storage_index)), {1, 3})
+
     def test_multiple_shares_uploaded_to_different_place(self):
         """
         If a storage index has multiple shares, uploads to different shares are

From 48a9bf745724c5c5333e46ad48f3ae6c3771c8fc Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Tue, 1 Feb 2022 10:25:13 -0500
Subject: [PATCH 2/8] Hook up more IStorageServer tests that can now pass with
 HTTP.

---
 src/allmydata/storage_client.py           | 33 +++++++++++++++++++++--
 src/allmydata/test/test_istorageserver.py |  2 --
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index ca977c9d9..e7dbb27a8 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -1042,7 +1042,7 @@ class _FakeRemoteReference(object):
 @attr.s
 class _HTTPBucketWriter(object):
     """
-    Emulate a ``RIBucketWriter``.
+    Emulate a ``RIBucketWriter``, but use HTTP protocol underneath.
     """
     client = attr.ib(type=StorageClientImmutables)
     storage_index = attr.ib(type=bytes)
@@ -1069,6 +1069,25 @@ class _HTTPBucketWriter(object):
         return defer.succeed(None)
 
 
+
+@attr.s
+class _HTTPBucketReader(object):
+    """
+    Emulate a ``RIBucketReader``.
+    """
+    client = attr.ib(type=StorageClientImmutables)
+    storage_index = attr.ib(type=bytes)
+    share_number = attr.ib(type=int)
+
+    def read(self, offset, length):
+        return self.client.read_share_chunk(
+            self.storage_index, self.share_number, offset, length
+        )
+
+    def advise_corrupt_share(self, reason):
+       pass  # TODO in later ticket
+
+
 # WORK IN PROGRESS, for now it doesn't actually implement whole thing.
 @implementer(IStorageServer)  # type: ignore
 @attr.s
@@ -1117,8 +1136,18 @@ class _HTTPStorageServer(object):
             })
         )
 
+    @defer.inlineCallbacks
     def get_buckets(
             self,
             storage_index,
     ):
-        pass
+        immutable_client = StorageClientImmutables(self._http_client)
+        share_numbers = yield immutable_client.list_shares(
+            storage_index
+        )
+        defer.returnValue({
+            share_num: _FakeRemoteReference(_HTTPBucketReader(
+                immutable_client, storage_index, share_num
+            ))
+            for share_num in share_numbers
+        })
diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py
index 96d4f687f..dc2aa0efb 100644
--- a/src/allmydata/test/test_istorageserver.py
+++ b/src/allmydata/test/test_istorageserver.py
@@ -1149,8 +1149,6 @@ class HTTPImmutableAPIsTests(
         "test_get_buckets_skips_unfinished_buckets",
         "test_matching_overlapping_writes",
         "test_non_matching_overlapping_writes",
-        "test_read_bucket_at_offset",
-        "test_written_shares_are_readable",
         "test_written_shares_are_allocated",
     }
 

From 7da506d5d0adbae71fefe87d6981b79540324caf Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Tue, 1 Feb 2022 10:26:42 -0500
Subject: [PATCH 3/8] News file.

---
 newsfragments/3871.minor | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 newsfragments/3871.minor

diff --git a/newsfragments/3871.minor b/newsfragments/3871.minor
new file mode 100644
index 000000000..e69de29bb

From 70d0bd0597b46015c45489ba57b5b566abe5e395 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Tue, 1 Feb 2022 10:41:12 -0500
Subject: [PATCH 4/8] Test and document what happens for non-existent storage
 index.

---
 docs/proposed/http-storage-node-protocol.rst | 3 +++
 src/allmydata/storage/http_server.py         | 2 --
 src/allmydata/test/test_storage_http.py      | 8 ++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst
index f47a50d3e..9c09eb362 100644
--- a/docs/proposed/http-storage-node-protocol.rst
+++ b/docs/proposed/http-storage-node-protocol.rst
@@ -635,6 +635,9 @@ indicated storage index. For example::
 
   [1, 5]
 
+An unknown storage index results in empty list, so that lack of existence of
+storage index is not leaked.
+
 ``GET /v1/immutable/:storage_index/:share_number``
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py
index f4ba865ca..f885baa22 100644
--- a/src/allmydata/storage/http_server.py
+++ b/src/allmydata/storage/http_server.py
@@ -266,8 +266,6 @@ class HTTPServer(object):
         List shares for the given storage index.
         """
         storage_index = si_a2b(storage_index.encode("ascii"))
-
-        # TODO in future ticket, handle KeyError as 404
         share_numbers = list(self._storage_server.get_buckets(storage_index).keys())
         return self._cbor(request, share_numbers)
 
diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index 4ddfff62e..b2def5581 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -413,6 +413,14 @@ class ImmutableHTTPAPITests(SyncTestCase):
         # Now shares 1 and 3 exist:
         self.assertEqual(result_of(im_client.list_shares(storage_index)), {1, 3})
 
+    def test_list_shares_unknown_storage_index(self):
+        """
+        Listing unknown storage index's shares results in empty list of shares.
+        """
+        im_client = StorageClientImmutables(self.http.client)
+        storage_index = b"".join(bytes([i]) for i in range(16))
+        self.assertEqual(result_of(im_client.list_shares(storage_index)), set())
+
     def test_multiple_shares_uploaded_to_different_place(self):
         """
         If a storage index has multiple shares, uploads to different shares are

From aebb5056de20f59f2362431b83514b2a62634f42 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Wed, 2 Feb 2022 11:00:16 -0500
Subject: [PATCH 5/8] Don't use real reactor in these tests.

---
 src/allmydata/test/test_storage_http.py | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py
index b2def5581..982e22859 100644
--- a/src/allmydata/test/test_storage_http.py
+++ b/src/allmydata/test/test_storage_http.py
@@ -23,6 +23,7 @@ from treq.testing import StubTreq
 from klein import Klein
 from hyperlink import DecodedURL
 from collections_extended import RangeMap
+from twisted.internet.task import Clock
 
 from .common import SyncTestCase
 from ..storage.server import StorageServer
@@ -230,8 +231,11 @@ class HttpTestFixture(Fixture):
     """
 
     def _setUp(self):
+        self.clock = Clock()
         self.tempdir = self.useFixture(TempDir())
-        self.storage_server = StorageServer(self.tempdir.path, b"\x00" * 20)
+        self.storage_server = StorageServer(
+            self.tempdir.path, b"\x00" * 20, clock=self.clock
+        )
         self.http_server = HTTPServer(self.storage_server, SWISSNUM_FOR_TEST)
         self.client = StorageClient(
             DecodedURL.from_text("http://127.0.0.1"),
@@ -401,13 +405,15 @@ class ImmutableHTTPAPITests(SyncTestCase):
 
         # Upload shares 1 and 3:
         for share_number in [1, 3]:
-            progress = result_of(im_client.write_share_chunk(
+            progress = result_of(
+                im_client.write_share_chunk(
                     storage_index,
                     share_number,
                     upload_secret,
                     0,
                     b"0123456789",
-            ))
+                )
+            )
             self.assertTrue(progress.finished)
 
         # Now shares 1 and 3 exist:

From 7454929be0d761b87909f04e26a9cf61d85a5702 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Fri, 4 Feb 2022 09:26:25 -0500
Subject: [PATCH 6/8] Less code duplication.

---
 src/allmydata/storage/http_client.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py
index 8a1d03192..2dc133b52 100644
--- a/src/allmydata/storage/http_client.py
+++ b/src/allmydata/storage/http_client.py
@@ -238,7 +238,7 @@ class StorageClientImmutables(object):
             raise ClientException(
                 response.code,
             )
-        body = loads((yield response.content()))
+        body = yield _decode_cbor(response)
         remaining = RangeMap()
         for chunk in body["required"]:
             remaining.set(True, chunk["begin"], chunk["end"])
@@ -293,8 +293,8 @@ class StorageClientImmutables(object):
             url,
         )
         if response.code == http.OK:
-            body = yield response.content()
-            returnValue(set(loads(body)))
+            body = yield _decode_cbor(response)
+            returnValue(set(body))
         else:
             raise ClientException(
                 response.code,

From 5e3a31166d7782e8b84094b58fe4bca40faf1995 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Fri, 4 Feb 2022 09:26:58 -0500
Subject: [PATCH 7/8] Better explanation.

---
 src/allmydata/storage_client.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index e7dbb27a8..cf5fb65a2 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -1073,7 +1073,7 @@ class _HTTPBucketWriter(object):
 @attr.s
 class _HTTPBucketReader(object):
     """
-    Emulate a ``RIBucketReader``.
+    Emulate a ``RIBucketReader``, but use HTTP protocol underneath.
     """
     client = attr.ib(type=StorageClientImmutables)
     storage_index = attr.ib(type=bytes)

From 83d8f2eb78f1c9da1eadf97429e9f0eac12deafd Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@itamarst.org>
Date: Fri, 4 Feb 2022 09:27:29 -0500
Subject: [PATCH 8/8] Remove incorrect editorial.

---
 docs/proposed/http-storage-node-protocol.rst | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/proposed/http-storage-node-protocol.rst b/docs/proposed/http-storage-node-protocol.rst
index 9c09eb362..b00b327e3 100644
--- a/docs/proposed/http-storage-node-protocol.rst
+++ b/docs/proposed/http-storage-node-protocol.rst
@@ -635,8 +635,7 @@ indicated storage index. For example::
 
   [1, 5]
 
-An unknown storage index results in empty list, so that lack of existence of
-storage index is not leaked.
+An unknown storage index results in an empty list.
 
 ``GET /v1/immutable/:storage_index/:share_number``
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!