Refactor LoginDatabase migration

components/password_manager/core/browser/sql_table_builder.h

Index: components/password_manager/core/browser/sql_table_builder.h
diff --git a/components/password_manager/core/browser/sql_table_builder.h b/components/password_manager/core/browser/sql_table_builder.h
new file mode 100644
index 0000000000000000000000000000000000000000..c69f1a571589748b52e87486d23bf334a2fc93f4
--- /dev/null
+++ b/components/password_manager/core/browser/sql_table_builder.h
@@ -0,0 +1,149 @@
+// Copyright 2016 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.
+
+#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SQL_TABLE_BUILDER_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SQL_TABLE_BUILDER_H_
+
+#include <limits>
+#include <list>
+#include <string>
+
+#include "base/macros.h"
+
+namespace sql {
+class Connection;
+}
+
+namespace password_manager {
+
+// Use this class to represent the versioned evolution of a SQLite table
+// structure and generate creating and migrating statements for it.
+//
+// Two values are currently hard-coded, because they are the same in the
+// current use-cases:
+// * The table name is hard-coded as "logins".
+// * The index is built for column "signon_realm" which is expected to exist.
+//
+// Usage example:
+//
+// SQLTableBuilder builder;
+//
+// // First describe a couple of versions:
+// builder.AddColumn("name", "VARCHAR");
+// builder.AddColumn("icon", "VARCHAR");
+// builder.AddColumn("password", "VARCHAR NOT NULL");
+// unsigned version = builder.SealVersion();  // Version 0 is sealed.
+// DCHECK_EQ(0u, version);
+// builder.RenameColumn("icon", "avatar");
+// version = builder.SealVersion();  // Version 1 is sealed.
+// DCHECK_EQ(1u, version);
+//
+// // Now, assuming that |db| has a table "logins" in a state corresponding
+// // version 0, this will migrate it to version 1:
+// sql::Connection* db = ...;
+// builder.MigrateToNextFrom(0, db);
+//
+// // And assuming |db| has no table called "logins", this will create one
+// // in a state corresponding the latest sealed version:
+// builder.CreateTable(db);
+class SQLTableBuilder {
+ public:
+  // Create the builder for "logins".
+  SQLTableBuilder();
+
+  ~SQLTableBuilder();
+
+  // Adds a column in the table description, with |name| and |type|. |name|
+  // must not have been added to the table in this version before.
+  void AddColumn(std::string name, std::string type);

[dvadym, 2016/07/12 14:48:58]

Nit: comments for all methods *Column: It would be look nicer to user the same
type for arguments, i.e. std::string or const std::string&. As I see they are
used in the same way, so it seems there is not sense to make them of different
type.
And if std::string is used and move is done inside a method, probably it makes
sense to indicate in comments to the method that string is moved inside of the
method. WDYT?

[vabr (Chromium), 2016/07/13 09:12:13]

There is some logic in whether the string is passed by value or reference:
 (1) Look-up strings are const ref.
 (2) Strings to be parts of the stored structures are passed by value.

I'm not sure if a comment is needed about moving the string inside of the
method. It should not make a difference for the caller, they are not able to use
it again -- either they passed a temporary (hence no way to reference that
later), or a named object, in which case a copy was made.

I'm torn -- on one hand I'd really like to make a habit out of using the move
semantics where appropriate. And here it is appropriate, because all the
callsites use temporary arguments, and its likely to be like that in the future.

On the other hand, it is apparently still raising suspicion. And in this
particular case the methods are called rarely, so the performance difference is
negligible.

Based on the above I'm going with switching all in const references.

+
+  // As AddColumn but also adds column |name| to the unique key of the table.
+  // SealVersion must not have been called yet (the builder does not currently
+  // support migration code for changing the unique key between versions).
+  void AddColumnToUniqueKey(std::string name, std::string type);
+
+  // Renames column |old_name| to |new_name|. |old_name| must have been added in
+  // the past. The column "signon_realm" must not be renamed.
+  void RenameColumn(const std::string& old_name, std::string new_name);
+
+  // Removes column |name|. |name| must have been added in the past. The column
+  // "signon_realm" must not be renamed.
+  void DropColumn(const std::string& name);
+
+  // Increments the internal version counter and marks the current state of the
+  // table as that version. Returns the sealed version. Calling any of the
+  // *Column* methods above will result in starting a new version which is not
+  // considered sealed. The first version is 0. The column "signon_realm" must
+  // be present in |columns_| every time this is called, and at least one call
+  // to AddColumnToUniqueKey must have been done before this is called the first
+  // time.
+  unsigned SealVersion();
+
+  // Assuming that the database connected through |db| contains a table called
+  // "logins" in a state described by version |old_version|, migrates it to
+  // the current version, which must be sealed. Returns true on success.
+  bool MigrateFrom(unsigned old_version, sql::Connection* db);
+
+  // If |db| connects to a database where table "logins" already exists,
+  // this is a no-op and returns true. Otherwise, "logins" is created in a
+  // state described by the current version known to the builder. The current
+  // version must be sealed. Returns true on success.
+  bool CreateTable(sql::Connection* db);
+
+  // Returns the comma-separated list of all column names present in the last
+  // version. The last version must be sealed.
+  std::string ListAllNames() const;

[dvadym, 2016/07/12 14:48:59]

Nit: isn't it better to add "Column" in the function name i.e.
ListAllColumnNames

[vabr (Chromium), 2016/07/13 09:12:13]

Done.

+
+  // Same as ListAllNames, but for non-unique key names only, and with names
+  // followed by " = ?".
+  std::string ListAllNonuniqueKeyNames() const;
+
+  // Same as ListAllNonuniqueKeyNames, but for unique key names and separated by
+  // " AND ".
+  std::string ListAllUniqueKeyNames() const;
+
+  // Returns the number of all columns present in the last version. The last
+  // version must be sealed.
+  size_t NumberOfColumns() const;
+
+ private:
+  // Stores the information about one column (name, type, etc.).
+  struct Column;
+
+  static unsigned constexpr kInvalidVersion =
+      std::numeric_limits<unsigned>::max();
+
+  // Assuming that the database connected through |db| contains a table called
+  // "logins" in a state described by version |old_version|, migrates it to
+  // version |old_version + 1|. The current version known to the builder must be
+  // at least |old_version + 1| and sealed. Returns true on success.
+  bool MigrateToNextFrom(unsigned old_version, sql::Connection* db);
+
+  // Looks up column named |name| in |columns_|. If present, returns the last
+  // one.
+  std::list<Column>::reverse_iterator FindLastByName(const std::string& name);

[dvadym, 2016/07/12 14:48:58]

Nit: what's about making this function to be const?

[vabr (Chromium), 2016/07/13 09:12:13]

This won't work, the code modifies the list of columns through the returned
iterator.

[dvadym, 2016/07/13 09:45:11]

Acknowledged

+
+  // Returns whether the last version is |version| and whether it was sealed
+  // (by calling SealVersion with no table modifications afterwards).
+  bool IsVersionLastAndSealed(unsigned version) const;
+
+  // Whether |column| is present in the last version. The last version must be
+  // sealed.
+  bool IsInLastVersion(const Column& column) const;
+
+  // Last sealed version, kInvalidVersion means "none".
+  unsigned sealed_version_ = kInvalidVersion;
+
+  std::list<Column> columns_;  // Columns of the table, across all versions.
+
+  // The "UNIQUE" part of an SQL CREATE TABLE constraint. This value is
+  // computed dring sealing the first version (0).
+  std::string unique_constraint_;
+
+  DISALLOW_COPY_AND_ASSIGN(SQLTableBuilder);
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_SQL_TABLE_BUILDER_H_