Make //content responsible for generating notification Ids

content/browser/notifications/notification_database_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/notifications/notification_database.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/files/scoped_temp_dir.h"
+#include "base/guid.h"
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/public/browser/notification_database_data.h"
 #include "content/public/common/platform_notification_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 #include "url/gurl.h"
 
@@ skipping 28 matching lines @@
     return new NotificationDatabase(path);
   }
 
   // Creates a new notification for |service_worker_registration_id| belonging
   // to |origin| and writes it to the database. The written notification id
   // will be stored in |notification_id|.
   void CreateAndWriteNotification(NotificationDatabase* database,
                                   const GURL& origin,
                                   const std::string& tag,
                                   int64_t service_worker_registration_id,
-                                  int64_t* notification_id) {
+                                  std::string* notification_id) {
+    DCHECK(notification_id);
+
     NotificationDatabaseData database_data;
+    database_data.notification_id = base::GenerateGUID();

[johnme, 2016/09/08 15:21:50]

Why is there a GUID here? If this is just to generate a arbitrary random string
for the test, please make a wrapper GenerateNotificationId() function, in which
you have a comment explaining that GenerateGUID is just an easy way to get a
random string (I'm assuming that the ids need to be different; otherwise you'd
have used a kNotificationId constant).

Ditto everywhere below that you use GenerateGUID.

[Peter Beverloo, 2016/09/08 18:56:02]

Done.

     database_data.origin = origin;
     database_data.service_worker_registration_id =
         service_worker_registration_id;
     database_data.notification_data.tag = tag;
 
     ASSERT_EQ(NotificationDatabase::STATUS_OK,
-              database->WriteNotificationData(origin, database_data,
-                                              notification_id));
+              database->WriteNotificationData(origin, database_data));
+
+    *notification_id = database_data.notification_id;
   }
 
   // Populates |database| with a series of example notifications that differ in
   // their origin and Service Worker registration id.
   void PopulateDatabaseWithExampleData(NotificationDatabase* database) {
-    int64_t notification_id;
+    std::string notification_id;
     for (size_t i = 0; i < arraysize(kExampleNotificationData); ++i) {
       ASSERT_NO_FATAL_FAILURE(CreateAndWriteNotification(
           database, GURL(kExampleNotificationData[i].origin),
           kExampleNotificationData[i].tag,
           kExampleNotificationData[i].service_worker_registration_id,
           &notification_id));
     }
   }
 
   // Returns if |database| has been opened.
@@ skipping 88 matching lines @@
 TEST_F(NotificationDatabaseTest, NotificationIdIncrements) {
   base::ScopedTempDir database_dir;
   ASSERT_TRUE(database_dir.CreateUniqueTempDir());
 
   std::unique_ptr<NotificationDatabase> database(
       CreateDatabaseOnFileSystem(database_dir.path()));
 
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
+  EXPECT_EQ(database->GetNextNotificationId(), 1);
+  EXPECT_EQ(database->GetNextNotificationId(), 2);
+  EXPECT_EQ(database->GetNextNotificationId(), 3);
+
   GURL origin("https://example.com");
 
-  int64_t notification_id = 0;
-
-  // Verify that getting two ids on the same database instance results in
-  // incrementing values. Notification ids will start at 1.
+  std::string notification_id;
   ASSERT_NO_FATAL_FAILURE(
       CreateAndWriteNotification(database.get(), origin, "" /* tag */,
                                  0 /* sw_registration_id */, &notification_id));
-  EXPECT_EQ(notification_id, 1);
-
-  ASSERT_NO_FATAL_FAILURE(
-      CreateAndWriteNotification(database.get(), origin, "" /* tag */,
-                                 0 /* sw_registration_id */, &notification_id));
-  EXPECT_EQ(notification_id, 2);
 
   database.reset(CreateDatabaseOnFileSystem(database_dir.path()));
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(false /* create_if_missing */));
 
   // Verify that the next notification id was stored in the database, and
   // continues where we expect it to be, even after closing and opening it.
-  ASSERT_NO_FATAL_FAILURE(
-      CreateAndWriteNotification(database.get(), origin, "" /* tag */,
-                                 0 /* sw_registration_id */, &notification_id));
-  EXPECT_EQ(notification_id, 3);
+  EXPECT_EQ(database->GetNextNotificationId(), 4);
 }
 
 TEST_F(NotificationDatabaseTest, NotificationIdIncrementsStorage) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   GURL origin("https://example.com");
 
-  NotificationDatabaseData database_data;
-  database_data.notification_id = -1;
+  NotificationDatabaseData database_data, read_database_data;
+  database_data.notification_id = base::GenerateGUID();
 
-  int64_t notification_id = 0;
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->WriteNotificationData(origin, database_data, &notification_id));
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->WriteNotificationData(origin, database_data));
 
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->ReadNotificationData(notification_id, origin, &database_data));
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->ReadNotificationData(database_data.notification_id,
+                                           origin, &read_database_data));
 
-  EXPECT_EQ(notification_id, database_data.notification_id);
+  EXPECT_EQ(database_data.notification_id, read_database_data.notification_id);
 }
 
 TEST_F(NotificationDatabaseTest, NotificationIdCorruption) {
   base::ScopedTempDir database_dir;
   ASSERT_TRUE(database_dir.CreateUniqueTempDir());
 
   std::unique_ptr<NotificationDatabase> database(
       CreateDatabaseOnFileSystem(database_dir.path()));
 
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   GURL origin("https://example.com");
 
   NotificationDatabaseData database_data;
-  int64_t notification_id = 0;
+  database_data.notification_id = base::GenerateGUID();
 
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->WriteNotificationData(origin, database_data, &notification_id));
-  EXPECT_EQ(notification_id, 1);
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->WriteNotificationData(origin, database_data));
 
   // Deliberately write an invalid value as the next notification id. When
   // re-opening the database, the Open() method should realize that an invalid
   // value is being read, and mark the database as corrupted.
   ASSERT_NO_FATAL_FAILURE(
       WriteLevelDBKeyValuePair(database.get(), "NEXT_NOTIFICATION_ID", "-42"));
 
   database.reset(CreateDatabaseOnFileSystem(database_dir.path()));
   EXPECT_EQ(NotificationDatabase::STATUS_ERROR_CORRUPTED,
             database->Open(false /* create_if_missing */));
 }
 
 TEST_F(NotificationDatabaseTest, ReadInvalidNotificationData) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   NotificationDatabaseData database_data;
 
   // Reading the notification data for a notification that does not exist should
   // return the ERROR_NOT_FOUND status code.
   EXPECT_EQ(NotificationDatabase::STATUS_ERROR_NOT_FOUND,
-            database->ReadNotificationData(9001, GURL("https://chrome.com"),
+            database->ReadNotificationData("my-id", GURL("https://chrome.com"),

[johnme, 2016/09/08 15:21:50]

Nit: s/my-id/bad-id/ ?

[Peter Beverloo, 2016/09/08 18:56:02]

Done.

                                            &database_data));
 }
 
 TEST_F(NotificationDatabaseTest, ReadNotificationDataDifferentOrigin) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
-  int64_t notification_id = 0;
   GURL origin("https://example.com");
 
   NotificationDatabaseData database_data, read_database_data;
+  database_data.notification_id = base::GenerateGUID();
   database_data.notification_data.title = base::UTF8ToUTF16("My Notification");
 
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->WriteNotificationData(origin, database_data, &notification_id));
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->WriteNotificationData(origin, database_data));
 
   // Reading the notification from the database when given a different origin
   // should return the ERROR_NOT_FOUND status code.
-  EXPECT_EQ(
-      NotificationDatabase::STATUS_ERROR_NOT_FOUND,
-      database->ReadNotificationData(
-          notification_id, GURL("https://chrome.com"), &read_database_data));
+  EXPECT_EQ(NotificationDatabase::STATUS_ERROR_NOT_FOUND,
+            database->ReadNotificationData(database_data.notification_id,
+                                           GURL("https://chrome.com"),
+                                           &read_database_data));
 
   // However, reading the notification from the database with the same origin
   // should return STATUS_OK and the associated notification data.
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
-            database->ReadNotificationData(notification_id, origin,
-                                           &read_database_data));
+            database->ReadNotificationData(database_data.notification_id,
+                                           origin, &read_database_data));
 
   EXPECT_EQ(database_data.notification_data.title,
             read_database_data.notification_data.title);
 }
 
 TEST_F(NotificationDatabaseTest, ReadNotificationDataReflection) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
-  int64_t notification_id = 0;
-
   GURL origin("https://example.com");
 
   PlatformNotificationData notification_data;
   notification_data.title = base::UTF8ToUTF16("My Notification");
   notification_data.direction =
       PlatformNotificationData::DIRECTION_RIGHT_TO_LEFT;
   notification_data.lang = "nl-NL";
   notification_data.body = base::UTF8ToUTF16("Hello, world!");
   notification_data.tag = "replace id";
   notification_data.icon = GURL("https://example.com/icon.png");
   notification_data.silent = true;
 
   NotificationDatabaseData database_data;
-  database_data.notification_id = notification_id;
+  database_data.notification_id = base::GenerateGUID();
   database_data.origin = origin;
   database_data.service_worker_registration_id = 42;
   database_data.notification_data = notification_data;
 
   // Write the constructed notification to the database, and then immediately
   // read it back from the database again as well.
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->WriteNotificationData(origin, database_data, &notification_id));
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->WriteNotificationData(origin, database_data));
 
   NotificationDatabaseData read_database_data;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
-            database->ReadNotificationData(notification_id, origin,
-                                           &read_database_data));
+            database->ReadNotificationData(database_data.notification_id,
+                                           origin, &read_database_data));
 
   // Verify that all members retrieved from the database are exactly the same
   // as the ones that were written to it. This tests the serialization behavior.
 
-  EXPECT_EQ(notification_id, read_database_data.notification_id);
+  EXPECT_EQ(database_data.notification_id, read_database_data.notification_id);
 
   EXPECT_EQ(database_data.origin, read_database_data.origin);
   EXPECT_EQ(database_data.service_worker_registration_id,
             read_database_data.service_worker_registration_id);
 
   const PlatformNotificationData& read_notification_data =
       read_database_data.notification_data;
 
   EXPECT_EQ(notification_data.title, read_notification_data.title);
   EXPECT_EQ(notification_data.direction, read_notification_data.direction);
   EXPECT_EQ(notification_data.lang, read_notification_data.lang);
   EXPECT_EQ(notification_data.body, read_notification_data.body);
   EXPECT_EQ(notification_data.tag, read_notification_data.tag);
   EXPECT_EQ(notification_data.icon, read_notification_data.icon);
   EXPECT_EQ(notification_data.silent, read_notification_data.silent);
 }
 
 TEST_F(NotificationDatabaseTest, ReadWriteMultipleNotificationData) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   GURL origin("https://example.com");
-  int64_t notification_id = 0;
+
+  std::vector<std::string> notification_ids;
+  std::string notification_id;
 
   // Write ten notifications to the database, each with a unique title and
   // notification id (it is the responsibility of the user to increment this).
   for (int i = 1; i <= 10; ++i) {
     ASSERT_NO_FATAL_FAILURE(CreateAndWriteNotification(
         database.get(), origin, "" /* tag */, i /* sw_registration_id */,
         &notification_id));
-    EXPECT_EQ(notification_id, i);
+
+    EXPECT_FALSE(notification_id.empty());
+
+    notification_ids.push_back(notification_id);
   }
 
   NotificationDatabaseData database_data;
 
+  int64_t service_worker_registration_id = 1;
+
   // Read the ten notifications from the database, and verify that the titles
   // of each of them matches with how they were created.
-  for (int i = 1; i <= 10; ++i) {
+  for (const std::string& notification_id : notification_ids) {
     ASSERT_EQ(NotificationDatabase::STATUS_OK,
-              database->ReadNotificationData(i /* notification_id */, origin,
+              database->ReadNotificationData(notification_id, origin,
                                              &database_data));
 
-    EXPECT_EQ(i, database_data.service_worker_registration_id);
+    EXPECT_EQ(service_worker_registration_id++,
+              database_data.service_worker_registration_id);
   }
 }
 
 TEST_F(NotificationDatabaseTest, DeleteInvalidNotificationData) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   // Deleting non-existing notifications is not considered to be a failure.
-  ASSERT_EQ(NotificationDatabase::STATUS_OK,
-            database->DeleteNotificationData(9001, GURL("https://chrome.com")));
+  ASSERT_EQ(
+      NotificationDatabase::STATUS_OK,
+      database->DeleteNotificationData("9001", GURL("https://chrome.com")));

[johnme, 2016/09/08 15:21:50]

Nit: s/9001/bad-id/

[Peter Beverloo, 2016/09/08 18:56:02]

Done.

 }
 
 TEST_F(NotificationDatabaseTest, DeleteNotificationDataSameOrigin) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
-  int64_t notification_id = 0;
+  const std::string notification_id = base::GenerateGUID();
 
   NotificationDatabaseData database_data;
+  database_data.notification_id = notification_id;
+
   GURL origin("https://example.com");
 
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->WriteNotificationData(origin, database_data, &notification_id));
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->WriteNotificationData(origin, database_data));
 
   // Reading a notification after writing one should succeed.
   EXPECT_EQ(
       NotificationDatabase::STATUS_OK,
       database->ReadNotificationData(notification_id, origin, &database_data));
 
   // Delete the notification which was just written to the database, and verify
   // that reading it again will fail.
   EXPECT_EQ(NotificationDatabase::STATUS_OK,
             database->DeleteNotificationData(notification_id, origin));
   EXPECT_EQ(
       NotificationDatabase::STATUS_ERROR_NOT_FOUND,
       database->ReadNotificationData(notification_id, origin, &database_data));
 }
 
 TEST_F(NotificationDatabaseTest, DeleteNotificationDataDifferentOrigin) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
-  int64_t notification_id = 0;
+  const std::string notification_id = base::GenerateGUID();
 
   NotificationDatabaseData database_data;
+  database_data.notification_id = notification_id;
+
   GURL origin("https://example.com");
 
-  ASSERT_EQ(
-      NotificationDatabase::STATUS_OK,
-      database->WriteNotificationData(origin, database_data, &notification_id));
+  ASSERT_EQ(NotificationDatabase::STATUS_OK,
+            database->WriteNotificationData(origin, database_data));
 
   // Attempting to delete the notification with a different origin, but with the
   // same |notification_id|, should not return an error (the notification could
   // not be found, but that's not considered a failure). However, it should not
   // remove the notification either.
   EXPECT_EQ(NotificationDatabase::STATUS_OK,
             database->DeleteNotificationData(notification_id,
                                              GURL("https://chrome.com")));
 
   EXPECT_EQ(
@@ skipping 63 matching lines @@
 
 TEST_F(NotificationDatabaseTest, DeleteAllNotificationDataForOrigin) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   ASSERT_NO_FATAL_FAILURE(PopulateDatabaseWithExampleData(database.get()));
 
   GURL origin("https://example.com:443");
 
-  std::set<int64_t> deleted_notification_set;
+  std::set<std::string> deleted_notification_ids;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->DeleteAllNotificationDataForOrigin(
-                origin, "" /* tag */, &deleted_notification_set));
+                origin, "" /* tag */, &deleted_notification_ids));
 
-  EXPECT_EQ(4u, deleted_notification_set.size());
+  EXPECT_EQ(4u, deleted_notification_ids.size());
 
   std::vector<NotificationDatabaseData> notifications;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->ReadAllNotificationDataForOrigin(origin, &notifications));
 
   EXPECT_EQ(0u, notifications.size());
 }
 
 TEST_F(NotificationDatabaseTest, DeleteAllNotificationDataForOriginWithTag) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
@@ skipping 16 matching lines @@
   for (const auto& database_data : notifications) {
     if (database_data.notification_data.tag == tag)
       ++notifications_with_tag;
     else
       ++notifications_without_tag;
   }
 
   ASSERT_GT(notifications_with_tag, 0u);
   ASSERT_GT(notifications_without_tag, 0u);
 
-  std::set<int64_t> deleted_notification_set;
+  std::set<std::string> deleted_notification_ids;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->DeleteAllNotificationDataForOrigin(
-                origin, "foo" /* tag */, &deleted_notification_set));
+                origin, "foo" /* tag */, &deleted_notification_ids));
 
-  EXPECT_EQ(notifications_with_tag, deleted_notification_set.size());
+  EXPECT_EQ(notifications_with_tag, deleted_notification_ids.size());
 
   std::vector<NotificationDatabaseData> updated_notifications;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->ReadAllNotificationDataForOrigin(origin,
                                                        &updated_notifications));
 
   EXPECT_EQ(notifications_without_tag, updated_notifications.size());
 
   size_t updated_notifications_with_tag = 0;
   size_t updated_notifications_without_tag = 0;
 
   for (const auto& database_data : updated_notifications) {
     if (database_data.notification_data.tag == tag)
       ++updated_notifications_with_tag;
     else
       ++updated_notifications_without_tag;
   }
 
   EXPECT_EQ(0u, updated_notifications_with_tag);
   EXPECT_EQ(notifications_without_tag, updated_notifications_without_tag);
 }
 
 TEST_F(NotificationDatabaseTest, DeleteAllNotificationDataForOriginEmpty) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   GURL origin("https://example.com");
 
-  std::set<int64_t> deleted_notification_set;
+  std::set<std::string> deleted_notification_ids;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->DeleteAllNotificationDataForOrigin(
-                origin, "" /* tag */, &deleted_notification_set));
+                origin, "" /* tag */, &deleted_notification_ids));
 
-  EXPECT_EQ(0u, deleted_notification_set.size());
+  EXPECT_EQ(0u, deleted_notification_ids.size());
 }
 
 TEST_F(NotificationDatabaseTest,
        DeleteAllNotificationDataForServiceWorkerRegistration) {
   std::unique_ptr<NotificationDatabase> database(CreateDatabaseInMemory());
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->Open(true /* create_if_missing */));
 
   ASSERT_NO_FATAL_FAILURE(PopulateDatabaseWithExampleData(database.get()));
 
   GURL origin("https://example.com:443");
-  std::set<int64_t> deleted_notification_set;
+  std::set<std::string> deleted_notification_ids;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->DeleteAllNotificationDataForServiceWorkerRegistration(
                 origin, kExampleServiceWorkerRegistrationId,
-                &deleted_notification_set));
+                &deleted_notification_ids));
 
-  EXPECT_EQ(2u, deleted_notification_set.size());
+  EXPECT_EQ(2u, deleted_notification_ids.size());
 
   std::vector<NotificationDatabaseData> notifications;
   ASSERT_EQ(NotificationDatabase::STATUS_OK,
             database->ReadAllNotificationDataForServiceWorkerRegistration(
                 origin, kExampleServiceWorkerRegistrationId, &notifications));
 
   EXPECT_EQ(0u, notifications.size());
 }
 
 }  // namespace content