libstore: remove FileTransferRequests::data

use separate upload and download methods instead.

Change-Id: I5baa2177c8ddd70268c75ff074e361b2f17dddbd

src/libfetchers/tarball.cc

@@ -46,7 +46,7 @@ DownloadFileResult downloadFile(
         request.expectedETag = getStrAttr(cached->infoAttrs, "etag");
     FileTransferResult res;
     try {
-        res = getFileTransfer()->enqueueFileTransfer(request).get();
+        res = getFileTransfer()->enqueueDownload(request).get();
     } catch (FileTransferError & e) {
         if (cached) {
             warn("%s; using cached version", e.msg());

src/libstore/filetransfer.cc

@@ -46,6 +46,7 @@ struct curlFileTransfer : public FileTransfer
         FileTransferRequest request;
         FileTransferResult result;
         Activity act;
+        std::optional<std::string> uploadData;
         bool done = false; // whether either the success or failure function has been called
         std::packaged_task<FileTransferResult(std::exception_ptr, FileTransferResult)> callback;
         std::function<void(TransferItem &, std::string_view data)> dataCallback;
@@ -81,18 +82,21 @@ struct curlFileTransfer : public FileTransfer
 
         std::string verb() const
         {
-            return request.data ? "upload" : "download";
+            return uploadData ? "upload" : "download";
         }
 
         TransferItem(curlFileTransfer & fileTransfer,
             const FileTransferRequest & request,
             std::invocable<std::exception_ptr> auto callback,
-            std::function<void(TransferItem &, std::string_view data)> dataCallback)
+            std::function<void(TransferItem &, std::string_view data)> dataCallback,
+            std::optional<std::string> uploadData
+        )
             : fileTransfer(fileTransfer)
             , request(request)
             , act(*logger, lvlTalkative, actFileTransfer,
-                fmt(request.data ? "uploading '%s'" : "downloading '%s'", request.uri),
+                fmt(uploadData ? "uploading '%s'" : "downloading '%s'", request.uri),
                 {request.uri}, request.parentAct)
+            , uploadData(std::move(uploadData))
             , callback([cb{std::move(callback)}] (std::exception_ptr ex, FileTransferResult r) {
                 cb(ex);
                 return r;
@@ -236,14 +240,14 @@ struct curlFileTransfer : public FileTransfer
         size_t readOffset = 0;
         size_t readCallback(char *buffer, size_t size, size_t nitems)
         {
-            if (readOffset == request.data->length())
+            if (readOffset == uploadData->length())
                 return 0;
-            auto count = std::min(size * nitems, request.data->length() - readOffset);
+            auto count = std::min(size * nitems, uploadData->length() - readOffset);
             assert(count);
             // Lint: this is turning a string into a byte array to hand to
             // curl, which is fine.
             // NOLINTNEXTLINE(bugprone-not-null-terminated-result)
-            memcpy(buffer, request.data->data() + readOffset, count);
+            memcpy(buffer, uploadData->data() + readOffset, count);
             readOffset += count;
             return count;
         }
@@ -295,11 +299,11 @@ struct curlFileTransfer : public FileTransfer
             if (request.head)
                 curl_easy_setopt(req, CURLOPT_NOBODY, 1);
 
-            if (request.data) {
+            if (uploadData) {
                 curl_easy_setopt(req, CURLOPT_UPLOAD, 1L);
                 curl_easy_setopt(req, CURLOPT_READFUNCTION, readCallbackWrapper);
                 curl_easy_setopt(req, CURLOPT_READDATA, this);
-                curl_easy_setopt(req, CURLOPT_INFILESIZE_LARGE, (curl_off_t) request.data->length());
+                curl_easy_setopt(req, CURLOPT_INFILESIZE_LARGE, (curl_off_t) uploadData->length());
             }
 
             if (request.verifyTLS) {
@@ -620,7 +624,7 @@ struct curlFileTransfer : public FileTransfer
 
     std::shared_ptr<TransferItem> enqueueItem(std::shared_ptr<TransferItem> item)
     {
-        if (item->request.data
+        if (item->uploadData
             && !item->request.uri.starts_with("http://")
             && !item->request.uri.starts_with("https://"))
             throw nix::Error("uploading to '%s' is not supported", item->request.uri);
@@ -651,7 +655,19 @@ struct curlFileTransfer : public FileTransfer
     }
 #endif
 
-    std::future<FileTransferResult> enqueueFileTransfer(const FileTransferRequest & request) override
+    std::future<FileTransferResult> enqueueDownload(const FileTransferRequest & request) override
+    {
+        return enqueueFileTransfer(request, std::nullopt);
+    }
+
+    std::future<FileTransferResult>
+    enqueueUpload(const FileTransferRequest & request, std::string data) override
+    {
+        return enqueueFileTransfer(request, std::move(data));
+    }
+
+    std::future<FileTransferResult>
+    enqueueFileTransfer(const FileTransferRequest & request, std::optional<std::string> data)
     {
         return enqueueFileTransfer(
             request,
@@ -660,13 +676,16 @@ struct curlFileTransfer : public FileTransfer
                     std::rethrow_exception(ex);
                 }
             },
-            {}
+            {},
+            std::move(data)
         );
     }
 
     std::future<FileTransferResult> enqueueFileTransfer(const FileTransferRequest & request,
         std::invocable<std::exception_ptr> auto callback,
-        std::function<void(TransferItem &, std::string_view data)> dataCallback)
+        std::function<void(TransferItem &, std::string_view data)> dataCallback,
+        std::optional<std::string> data
+    )
     {
         /* Ugly hack to support s3:// URIs. */
         if (request.uri.starts_with("s3://")) {
@@ -695,9 +714,11 @@ struct curlFileTransfer : public FileTransfer
             });
         }
 
-        return enqueueItem(std::make_shared<TransferItem>(
+        return enqueueItem(
-                               *this, request, std::move(callback), std::move(dataCallback)
+                   std::make_shared<TransferItem>(
-                           ))
+                       *this, request, std::move(callback), std::move(dataCallback), std::move(data)
+                   )
+        )
             ->callback.get_future();
     }
 
@@ -747,7 +768,8 @@ struct curlFileTransfer : public FileTransfer
                 thread. */
                 state->data.append(data);
                 state->avail.notify_one();
-            }
+            },
+            std::nullopt
         );
 
         struct InnerSource : Source

src/libstore/filetransfer.hh

@@ -62,7 +62,6 @@ struct FileTransferRequest
     size_t tries = fileTransferSettings.tries;
     unsigned int baseRetryTimeMs = 250;
     ActivityId parentAct;
-    std::optional<std::string> data;
 
     FileTransferRequest(std::string_view uri)
         : uri(uri), parentAct(getCurActivity()) { }
@@ -88,11 +87,17 @@ struct FileTransfer
     virtual ~FileTransfer() { }
 
     /**
-     * Enqueue a data transfer request, returning a future to the result of
+     * Enqueues a download request, returning a future for the result of
-     * the download. The future may throw a FileTransferError
+     * the download. The future may throw a FileTransferError exception.
-     * exception.
      */
-    virtual std::future<FileTransferResult> enqueueFileTransfer(const FileTransferRequest & request) = 0;
+    virtual std::future<FileTransferResult> enqueueDownload(const FileTransferRequest & request) = 0;
+
+    /**
+     * Enqueue an upload request, returning a future for the result of
+     * the upload. The future may throw a FileTransferError exception.
+     */
+    virtual std::future<FileTransferResult>
+    enqueueUpload(const FileTransferRequest & request, std::string data) = 0;
 
     /**
      * Download a file, returning its contents through a source. Will not return

src/libstore/http-binary-cache-store.cc

@@ -116,7 +116,7 @@ protected:
         try {
             FileTransferRequest request{makeURI(path)};
             request.head = true;
-            getFileTransfer()->enqueueFileTransfer(request).get();
+            getFileTransfer()->enqueueDownload(request).get();
             return true;
         } catch (FileTransferError & e) {
             /* S3 buckets return 403 if a file doesn't exist and the
@@ -133,10 +133,10 @@ protected:
         const std::string & mimeType) override
     {
         FileTransferRequest req{makeURI(path)};
-        req.data = StreamToSourceAdapter(istream).drain();
+        auto data = StreamToSourceAdapter(istream).drain();
         req.headers = {{"Content-Type", mimeType}};
         try {
-            getFileTransfer()->enqueueFileTransfer(req).get();
+            getFileTransfer()->enqueueUpload(req, std::move(data)).get();
         } catch (FileTransferError & e) {
             throw UploadToHTTP("while uploading to HTTP binary cache at '%s': %s", cacheUri, e.msg());
         }
@@ -171,7 +171,7 @@ protected:
         FileTransferRequest request{makeURI(path)};
 
         try {
-            return std::move(getFileTransfer()->enqueueFileTransfer(request).get().data);
+            return std::move(getFileTransfer()->enqueueDownload(request).get().data);
         } catch (FileTransferError & e) {
             if (e.error == FileTransfer::NotFound || e.error == FileTransfer::Forbidden)
                 return {};

src/nix/upgrade-nix.cc

@@ -286,7 +286,7 @@ struct CmdUpgradeNix : MixDryRun, EvalCommand
 
         // FIXME: use nixos.org?
         auto req = FileTransferRequest(storePathsUrl);
-        auto res = getFileTransfer()->enqueueFileTransfer(req).get();
+        auto res = getFileTransfer()->enqueueDownload(req).get();
 
         auto state = std::make_unique<EvalState>(SearchPath{}, store);
         auto v = state->allocValue();

tests/unit/libstore/filetransfer.cc

@@ -163,7 +163,7 @@ TEST(FileTransfer, NOT_ON_DARWIN(reportsSetupErrors))
     auto [port, srv] = serveHTTP("404 not found", "", [] { return ""; });
     auto ft = makeFileTransfer();
     ASSERT_THROW(
-        ft->enqueueFileTransfer(FileTransferRequest(fmt("http://[::1]:%d/index", port))).get(),
+        ft->enqueueDownload(FileTransferRequest(fmt("http://[::1]:%d/index", port))).get(),
         FileTransferError
     );
 }
@@ -219,7 +219,7 @@ TEST(FileTransfer, usesIntermediateLinkHeaders)
     auto ft = makeFileTransfer();
     FileTransferRequest req(fmt("http://[::1]:%d/first", port));
     req.baseRetryTimeMs = 0;
-    auto result = ft->enqueueFileTransfer(req).get();
+    auto result = ft->enqueueDownload(req).get();
     ASSERT_EQ(result.immutableUrl, "http://foo");
 }