Fixing actions upload format.

This CL fixes the following issues with the action upload:
1 - removes unnecessary URL params
2 - change content type to application/x-protobuf
3 - change the request body to use the FeedActionRequest proto instead of ActionRequest

Bug: b/160975622, 1108999
Change-Id: I15343ee2dcb149417932323a51055e126ffd5075
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2309002Reviewed-by: Dan H <harringtond@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Commit-Queue: Jonathan Freed <freedjm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791321}

components/feed/core/v2/feed_network.h

@@ -11,7 +11,7 @@
 #include "components/feed/core/v2/public/types.h"
 
 namespace feedwire {
-class ActionRequest;
+class FeedActionRequest;
 class Request;
 class Response;
 }  // namespace feedwire
@@ -51,11 +51,11 @@ class FeedNetwork {
       const feedwire::Request& request,
       base::OnceCallback<void(QueryRequestResult)> callback) = 0;
 
-  // Send a feedwire::ActionRequest, and receive the response in |callback|.
+  // Send a feedwire::FeedActionRequest, and receive the response in |callback|.
   // |callback| will be called unless the request is canceled with
   // |CancelRequests()|.
   virtual void SendActionRequest(
-      const feedwire::ActionRequest& request,
+      const feedwire::FeedActionRequest& request,
       base::OnceCallback<void(ActionRequestResult)> callback) = 0;
 
   // Cancels all pending requests immediately. This could be used, for example,

components/feed/core/v2/feed_network_impl.cc

@@ -41,7 +41,7 @@
 
 namespace feed {
 namespace {
-constexpr char kApplicationOctetStream[] = "application/octet-stream";
+constexpr char kApplicationXProtobuf[] = "application/x-protobuf";
 constexpr base::TimeDelta kNetworkTimeout = base::TimeDelta::FromSeconds(30);
 
 signin::ScopeSet GetAuthScopes() {
@@ -127,12 +127,10 @@ void ParseAndForwardResponse(base::OnceCallback<void(RESULT)> result_callback,
   std::move(result_callback).Run(std::move(result));
 }
 
-void AddMothershipPayloadQueryParams(bool is_post,
-                                     const std::string& payload,
+void AddMothershipPayloadQueryParams(const std::string& payload,
                                      const std::string& language_tag,
                                      GURL& url) {
-  if (!is_post)
-    url = net::AppendQueryParameter(url, "reqpld", payload);
+  url = net::AppendQueryParameter(url, "reqpld", payload);
   url = net::AppendQueryParameter(url, "fmt", "bin");
   if (!language_tag.empty())
     url = net::AppendQueryParameter(url, "hl", language_tag);
@@ -146,8 +144,7 @@ int PopulateRequestBody(const std::string& request_body,
     return 0;
   std::string compressed_request_body;
   compression::GzipCompress(request_body, &compressed_request_body);
-  loader->AttachStringForUpload(compressed_request_body,
-                                kApplicationOctetStream);
+  loader->AttachStringForUpload(compressed_request_body, kApplicationXProtobuf);
   return compressed_request_body.size();
 }
 
@@ -311,7 +308,7 @@ class FeedNetworkImpl::NetworkFetch {
                          network::ResourceRequest& request) const {
     if (has_request_body) {
       request.headers.SetHeader(net::HttpRequestHeaders::kContentType,
-                                kApplicationOctetStream);
+                                kApplicationXProtobuf);
       request.headers.SetHeader("Content-Encoding", "gzip");
     }
 
@@ -448,8 +445,8 @@ void FeedNetworkImpl::SendQueryRequest(
   if (url.is_empty())
     return std::move(callback).Run({});
 
-  AddMothershipPayloadQueryParams(/*is_post=*/false, base64proto,
-                                  delegate_->GetLanguageTag(), url);
+  AddMothershipPayloadQueryParams(base64proto, delegate_->GetLanguageTag(),
+                                  url);
   Send(url, "GET", /*request_body=*/{},
        base::BindOnce(&ParseAndForwardResponse<QueryRequestResult,
                                                NetworkRequestType::kFeedQuery>,
@@ -457,15 +454,12 @@ void FeedNetworkImpl::SendQueryRequest(
 }
 
 void FeedNetworkImpl::SendActionRequest(
-    const feedwire::ActionRequest& request,
+    const feedwire::FeedActionRequest& request,
     base::OnceCallback<void(ActionRequestResult)> callback) {
   std::string binary_proto;
   request.SerializeToString(&binary_proto);
 
   GURL url = GetUploadActionURL(chrome_channel_);
-  AddMothershipPayloadQueryParams(/*is_post=*/true,
-                                  /*payload=*/{}, delegate_->GetLanguageTag(),
-                                  url);
   Send(url, "POST", std::move(binary_proto),
        base::BindOnce(
            &ParseAndForwardResponse<ActionRequestResult,

components/feed/core/v2/feed_network_impl.h

@@ -57,7 +57,7 @@ class FeedNetworkImpl : public FeedNetwork {
       base::OnceCallback<void(QueryRequestResult)> callback) override;
 
   void SendActionRequest(
-      const feedwire::ActionRequest& request,
+      const feedwire::FeedActionRequest& request,
       base::OnceCallback<void(ActionRequestResult)> callback) override;
 
   // Cancels all pending requests immediately. This could be used, for example,

components/feed/core/v2/feed_network_impl_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/test/task_environment.h"
 #include "components/feed/core/common/pref_names.h"
 #include "components/feed/core/proto/v2/wire/action_request.pb.h"
+#include "components/feed/core/proto/v2/wire/feed_action_request.pb.h"
 #include "components/feed/core/proto/v2/wire/feed_action_response.pb.h"
 #include "components/feed/core/proto/v2/wire/request.pb.h"
 #include "components/feed/core/proto/v2/wire/response.pb.h"
@@ -58,9 +59,10 @@ feedwire::Response GetTestFeedResponse() {
   return response;
 }
 
-feedwire::ActionRequest GetTestActionRequest() {
-  feedwire::ActionRequest request;
-  request.set_request_version(feedwire::ActionRequest::FEED_UPLOAD_ACTION);
+feedwire::FeedActionRequest GetTestActionRequest() {
+  feedwire::FeedActionRequest request;
+  request.add_feed_action()->mutable_content_id()->set_content_domain(
+      "example.com");
   return request;
 }
 
@@ -409,10 +411,8 @@ TEST_F(FeedNetworkTest, SendActionRequestSendsValidRequest) {
   network::ResourceRequest resource_request =
       RespondToActionRequest(GetTestActionResponse(), net::HTTP_OK);
 
-  EXPECT_EQ(
-      GURL(
-          "https://discover-pa.googleapis.com/v1/actions:upload?fmt=bin&hl=en"),
-      resource_request.url);
+  EXPECT_EQ(GURL("https://discover-pa.googleapis.com/v1/actions:upload"),
+            resource_request.url);
 
   EXPECT_EQ("POST", resource_request.method);
   std::string content_encoding;

components/feed/core/v2/feed_stream_unittest.cc

@@ -279,7 +279,7 @@ class TestFeedNetwork : public FeedNetwork {
         FROM_HERE, base::BindOnce(std::move(callback), std::move(result)));
   }
   void SendActionRequest(
-      const feedwire::ActionRequest& request,
+      const feedwire::FeedActionRequest& request,
       base::OnceCallback<void(ActionRequestResult)> callback) override {
     action_request_sent = request;
     ++action_request_call_count;
@@ -327,7 +327,7 @@ class TestFeedNetwork : public FeedNetwork {
     result.response_body = nullptr;
     InjectActionRequestResult(std::move(result));
   }
-  base::Optional<feedwire::ActionRequest> action_request_sent;
+  base::Optional<feedwire::FeedActionRequest> action_request_sent;
   int action_request_call_count = 0;
   std::string consistency_token;
 
@@ -1419,17 +1419,13 @@ TEST_F(FeedStreamTest, LoadStreamFromNetworkUploadsActions) {
   WaitForIdleTaskQueue();
 
   EXPECT_EQ(1, network_.action_request_call_count);
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
 
   // Uploaded action should have been erased from store.
   stream_->UploadAction(MakeFeedAction(100ul), true, base::DoNothing());
   WaitForIdleTaskQueue();
   EXPECT_EQ(2, network_.action_request_call_count);
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
 }
 
 TEST_F(FeedStreamTest, LoadMoreUploadsActions) {
@@ -1445,22 +1441,16 @@ TEST_F(FeedStreamTest, LoadMoreUploadsActions) {
   stream_->LoadMore(surface.GetSurfaceId(), base::DoNothing());
   WaitForIdleTaskQueue();
 
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
   EXPECT_EQ("token-12", stream_->GetMetadata()->GetConsistencyToken());
 
   // Uploaded action should have been erased from the store.
   network_.action_request_sent.reset();
   stream_->UploadAction(MakeFeedAction(100ul), true, base::DoNothing());
   WaitForIdleTaskQueue();
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
-  EXPECT_EQ(100ul, network_.action_request_sent->feed_action_request()
-                       .feed_action(0)
-                       .content_id()
-                       .id());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
+  EXPECT_EQ(100ul,
+            network_.action_request_sent->feed_action(0).content_id().id());
 }
 
 TEST_F(FeedStreamTest, UploadActionsOneBatch) {
@@ -1469,16 +1459,12 @@ TEST_F(FeedStreamTest, UploadActionsOneBatch) {
   WaitForIdleTaskQueue();
 
   EXPECT_EQ(1, network_.action_request_call_count);
-  EXPECT_EQ(
-      3,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(3, network_.action_request_sent->feed_action_size());
 
   stream_->UploadAction(MakeFeedAction(99ul), true, base::DoNothing());
   WaitForIdleTaskQueue();
   EXPECT_EQ(2, network_.action_request_call_count);
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
 }
 
 TEST_F(FeedStreamTest, UploadActionsMultipleBatches) {
@@ -1500,9 +1486,7 @@ TEST_F(FeedStreamTest, UploadActionsMultipleBatches) {
   stream_->UploadAction(MakeFeedAction(99ul), true, base::DoNothing());
   WaitForIdleTaskQueue();
   EXPECT_EQ(4, network_.action_request_call_count);
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
 }
 
 TEST_F(FeedStreamTest, UploadActionsSkipsStaleActionsByTimestamp) {
@@ -1517,13 +1501,9 @@ TEST_F(FeedStreamTest, UploadActionsSkipsStaleActionsByTimestamp) {
 
   // Just one action should have been uploaded.
   EXPECT_EQ(1, network_.action_request_call_count);
-  EXPECT_EQ(
-      1,
-      network_.action_request_sent->feed_action_request().feed_action_size());
-  EXPECT_EQ(3ul, network_.action_request_sent->feed_action_request()
-                     .feed_action(0)
-                     .content_id()
-                     .id());
+  EXPECT_EQ(1, network_.action_request_sent->feed_action_size());
+  EXPECT_EQ(3ul,
+            network_.action_request_sent->feed_action(0).content_id().id());
 
   ASSERT_TRUE(cr.GetResult());
   EXPECT_EQ(1ul, cr.GetResult()->upload_attempt_count);
@@ -1545,9 +1525,7 @@ TEST_F(FeedStreamTest, UploadActionsErasesStaleActionsByAttempts) {
 
   // Four requests, three pending actions in the last request.
   EXPECT_EQ(4, network_.action_request_call_count);
-  EXPECT_EQ(
-      3,
-      network_.action_request_sent->feed_action_request().feed_action_size());
+  EXPECT_EQ(3, network_.action_request_sent->feed_action_size());
 
   // Action 0 should have been erased.
   ASSERT_TRUE(cr.GetResult());

components/feed/core/v2/tasks/upload_actions_task.cc

@@ -230,16 +230,11 @@ void UploadActionsTask::OnUpdateActionsFinished(
       batch->disown_feed_action_request();
   request->mutable_consistency_token()->set_token(consistency_token_);
 
-  feedwire::ActionRequest action_request;
-  action_request.set_request_version(
-      feedwire::ActionRequest::FEED_UPLOAD_ACTION);
-  action_request.set_allocated_feed_action_request(request.release());
-
   FeedNetwork* network = stream_->GetNetwork();
   DCHECK(network);
 
   network->SendActionRequest(
-      action_request,
+      *request,
       base::BindOnce(&UploadActionsTask::OnUploadFinished,
                      weak_ptr_factory_.GetWeakPtr(), std::move(batch)));
 }