BIND 10 #2541: add addZone() interface for data source

[BIND 10 Development]

#2541: add addZone() interface for data source
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:5 jinmei]:
 > '''client.h'''
 >
 > - as for providing the default definition in the base class (and still
 >   not making it pure virtual): In general, I've been trying to avoid
 >   it so that these default versions are not used accidentally (at the
 >   cost of requesting defining something in each derived class).  I'd
 >   also generally avoid relying on the "not implemented" default,
 >   because it's quite likely to suggest that the abstraction of the
 >   base class isn't really good.  But for this particular case we're
 >   doing some kind of hack, which is a bit better than the famous
 >   "short term workaround", so, assuming we shouldn't be afraid of
 >   totally revisiting it, I'm okay with breaking these two principles.
 >   But I'd suggest adding some considerations as a comment related to
 >   this.  (See also the next bullet for providing the default
 >   definition).

 ok, added an extra line to the \note. TBH, (ignoring whether or not it is
 clean design on a higher level), if the default implementation is only the
 throwing of an exception, that is specifically intended for this scenario,
 I don't think it too bad.

 > - parameter `name` doesn't appear in the declaration.  To avoid that
 >   while also avoiding to have 'unused parameter' warning, we'd need to
 >   move the definition to a .cc file.

 yeah we didn't have one so i just did what the rest of the file did. But
 good point (one of the others has void casts for this). Added a client.cc
 file and move the three default implementations there.

 (noting that the documented reason for getIterator() to have a default
 notimplemented is different)

 > - "fully qualified" sounds redundant to me, because a `Name` object
 >   always represents an absolute name. (but this is a minor point)
 >

 That I actually did on purpose; but more to be very clear that there isn't
 some 'base name' that will be appended (and FQDN tends to be more clear
 than 'complete' or something). But also no strong opinion :)

 > '''database.h'''
 >
 > - addZone() declaration: shouldn't this `may` be `will` or `must` or
 >   something?
 > {{{#!cpp
 >     /// Callers must also start a transaction before calling this
 method,
 >     /// implementations may throw DataSourceError if this has not been
 done.
 > }}}

 'should' i guess, though in this case it is tricky to find the right
 wording (it depends on whether you are reading it as an implementor of a
 datasource or as a user of the API)

 > - createZone() declaration: I'd skip describing param and return if
 >   there's nothing special to the database version (to avoid repeating
 >   the same thing).
 >

 ok

 > '''database.cc'''
 >
 > - `TransactionHolder` destructor: rollback() can throw, so we need some
 >   consideration for that case.  See also `~DatabaseUpdater()`.
 >

 oh of course. Added, similar log description as in the updater case)

 > '''sqlite3_accessor.h'''
 >
 > - Per style guide, we'd use C++-style comments for doxygen
 > {{{#!cpp
 >     /**
 >      * \brief Add a zone
 >      *
 > }}}
 >   Maybe you were trying to be consistent with the style for the
 >   previous method, but consistency is already broken within the file,
 >   and since this branch isn't big, I'd suggest providing consistency
 >   throughout the file (by changing others too).
 >

 I was, but sure, done :)

 > - For `SQLite3` specific text, "empty" looks awkward:
 > {{{
 >      * This implements the addZone from DatabaseAccessor and adds an
 (empty)
 >      * zone into the zones table. If the zone exists already, it is
 still
 > }}}
 >   because this concept can't be represented in the `zones` table.  I'd
 >   simply say "adds a zone" here.
 >

 ok

 > '''sqlite3_accessor.cc'''
 >
 > - If we require a transaction in addZone(), I think something like
 >   `InvalidOperation` is a more appropriate exception for the
 >   violation (although I've noticed there are other similar cases in
 >   this file where the same argument could apply).
 >

 Yeah, I'm willing to change this, but I had originally kept it consistent
 with the other transaction errors.
 I also noted a few DataSourceError throws that should probably be
 SQLite3Error btw. Shall I change these as well or do we want to defer
 that? (on a slightly higher level, when doing 2542, I noticed we may want
 a bit of a rearrangement on the client/databaseclient level as well,
 DataSourceError is too overloaded right now IMO)

 > - I believe these could be SQLITE_STATIC (which is a bit more efficient)
 >   in this case:
 > {{{#!cpp
 >     proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
 >     proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
 > }}}
 >   in fact, I think TRANSIENT is a better choice in this case because
 >   it's safer and performance isn't important here.  But it's probably
 >   a good idea to explicitly note that in a comment to prevent someone
 >   from trying to unnecessarily "optimize" it later.
 >

 ok

 > '''sqlite3_accessor_unittest.cc'''
 > - Overall: I guess many if not all of ASSERT_xx can be EXPECT_xxx.

 I tend to prefer to have the first of 'a set' be an assert (as in, if that
 one fails, it is highly likely the rest will too anyway). But indeed some
 could as well be expect.

 > - I'd check the existence of the same name of zone but of a different
 >   RR class doesn't cause confusion

 added

 > - If possible I'd like to test the case where either/both of
 >   add/getZone fails (maybe possible with another write).
 >

 Added a test that'll make add fail, but not sure how to do it for getZone
 (after add succeeded)

 > '''database_unittest.cc'''
 >
 > - I'd like to test the case where rollback happens.

 In a way the case is handled (at least, rollback is called in case the
 zone exists already), but it is not checked

 Apart from duplicating the original source code in the NopClient (in which
 case we're not really testing anything) I'm not entirely sure how to
 incorporate that in these tests.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2541#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development