[2541] Documentation and cleanup

don't duplicate the duplicate zone check in sqlite3 accessor's addZone()
added documentation

src/lib/datasrc/client.h

@@ -379,15 +379,24 @@ public:
                   "Data source doesn't support getZoneCount");
     }
 
-    // It first checks if the specified name of the zone exists.  If it
-    // exists it returns false; otherwise it adds information of the
-    // new zone in backend-dependent manner and returns true.
-    // The DB-based version of this method would perform the check and add in
-    // a single transaction.
-    //
-    // Throws on any unexpected failure.
-    // Default implementation throws isc::NotImplemented
-
+    /// \brief Create a zone in the database
+    ///
+    /// Creates a new (empty) zone in the data source backend, which
+    /// can subsequently be filled with data (through getUpdater()).
+    ///
+    /// \note This is a tentative API, and this method is likely to change
+    /// or be removed in the near future.
+    ///
+    /// Apart from the two exceptions mentioned below, in theory this
+    /// call can throw anything, depending on the implementation of
+    /// the datasource backend.
+    ///
+    /// \throw NotImplemented If the datasource backend does not support
+    ///                       direct zone creation.
+    /// \throw DataSourceError If something goes wrong in the data source
+    ///                        while creating the zone.
+    /// \param name The (fully qualified) name of the zone to create
+    /// \return True if the zone was added, false if it already existed
     virtual bool createZone(const dns::Name&) {
         isc_throw(isc::NotImplemented,
                   "Data source doesn't support addZone");

src/lib/datasrc/database.h

@@ -180,14 +180,12 @@ public:
     /// \brief Add a new zone to the database
     ///
     /// This method creates a new (and empty) zone in the database.
-    /// If a zone with the given name exists already, its zone_id value is
-    /// returned. Otherwise it is created, and the newly created zone_id
-    /// is returned.
     ///
-    /// Given the above, implementations should first do a lookup for the
-    /// given zone, and return the id if it exists.
+    /// Like for addRecordToZone, implementations are not required to
+    /// check for the existence of the given zone name, it is the
+    /// responsibility of the caller to do so.
     ///
-    /// Callers must start a transaction before calling this method,
+    /// Callers must also start a transaction before calling this method,
     /// implementations may throw DataSourceError if this has not been done.
     /// Callers should also expect DataSourceErrors for other potential
     /// problems.
@@ -1393,7 +1391,18 @@ public:
     ///     should use it as a ZoneFinder only.
     virtual FindResult findZone(const isc::dns::Name& name) const;
 
-    virtual bool createZone(const isc::dns::Name&);
+    /// \brief Create a zone in the database
+    ///
+    /// This method implements \c DataSourceClient::createZone()
+    ///
+    /// It starts a transaction, checks if the zone exists, and if it
+    /// does not, creates it, commits, and returns true. If the zone
+    /// does exist already, it does nothing (except abort the transaction)
+    /// and returns false.
+    ///
+    /// \param name The (fully qualified) name of the zone to create
+    /// \return True if the zone was added, false if it already existed
+    virtual bool createZone(const isc::dns::Name& name);
 
     /// \brief Get the zone iterator
     ///

src/lib/datasrc/sqlite3_accessor.cc

@@ -624,21 +624,18 @@ SQLite3Accessor::addZone(const std::string& name) {
                   "data source without transaction");
     }
 
-    // First check if the zone exists, if it does, do nothing and
-    // return false
-    std::pair<bool, int> getzone_result = getZone(name);
-    if (getzone_result.first) {
-        return (getzone_result.second);
-    }
-
     StatementProcessor proc(*dbparameters_, ADD_ZONE, "add zone");
     proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
     proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
     proc.exec();
 
     // There are tricks to getting this in one go, but it is safer
-    // to do a new lookup
-    getzone_result = getZone(name);
+    // to do a new lookup (sqlite3_last_insert_rowid is unsafe
+    // regarding threads and triggers). This requires two
+    // statements, and is unpredictable in the case a zone is added
+    // twice, but this method assumes the caller does not do that
+    // anyway
+    std::pair<bool, int> getzone_result = getZone(name);
     assert(getzone_result.first);
     return (getzone_result.second);
 }

src/lib/datasrc/sqlite3_accessor.h

@@ -135,24 +135,23 @@ public:
      * \brief Add a zone
      *
      * This implements the addZone from DatabaseAccessor and adds an (empty)
-     * zone into the zones table. If the zone exists already, nothing is done,
-     * and the id of the existing zone is returned. Otherwise, the zone is
-     * created, and its id is returned (unless it raises an exception).
+     * zone into the zones table. If the zone exists already, it is still
+     * added, so the caller should make sure this does not happen (by making
+     * sure the zone does not exist). In the case of duplicate addition,
+     * it is undefined which zone id is returned.
      *
      * The class of the newly created zone is the class passed at construction
      * time of the accessor.
      *
-     * Because this method performs a lookup and probably an assert, it
-     * requires a transaction has been started (with \c beginTransaction)
-     * by the caller.
+     * This method requires a transaction has been started (with
+     * \c beginTransaction) by the caller.
      *
      * \exception DataSourceError if no transaction is active, or if there
      *                            is an SQLite3 error when performing the
      *                            queries.
      *
      * \param name The origin name of the zone to add
-     * \return the id of the zone (either an existing one if the zone exists
-     *         already, or the id of the newly created zone).
+     * \return the id of the zone that has been added
      */
     virtual int addZone(const std::string& name);
 

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -681,16 +681,11 @@ TEST_F(SQLite3Create, addZone) {
     // Calling addZone without transaction should fail
     ASSERT_THROW(accessor->addZone(zone_name), DataSourceError);
 
-    // Add the zone. Since it does not exist yet, it should return true
+    // Add the zone. Returns the new zone id
     accessor->startTransaction();
     const int new_zone_id = accessor->addZone(zone_name);
     accessor->commit();
 
-    // Calling addZone again should return the same zone id
-    accessor->startTransaction();
-    ASSERT_EQ(new_zone_id, accessor->addZone(zone_name));
-    accessor->rollback();
-
     // Check that it exists now, but has no records at this point
     const std::pair<bool, int> zone_info2(accessor->getZone(zone_name));
     ASSERT_TRUE(zone_info2.first);