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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I made a few additional minor changes.  Please pull and check them.

 Replying to [comment:7 jelte]:

 > 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.

 My point is that if a base class needs to have a default
 implementation of a virtual method that throws a "not implemented"
 exception, it means the class has too much responsibility because the
 fact that it's the default means most of the derived classes wouldn't
 support the feature that the method originally intends to do (not
 throwing the exception).  Design-wise, if we find such is a case, what
 we should do to revisit the class design rather than to add a top
 level method that wouldn't be supported by most of actual derived
 classes.  But that's a general topic.  I'm not requesting a change to
 this branch due to this point.

 > > '''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 suggest creating a ticket for this work.  It will be beyond the
 scope of this task and may cause unexpected regressions that require
 more time to fix.

 > > - 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)

 For getZone() it's probably impossible to do in a reasonable way.

 As for the` addZoneWhileLocked` test:

 - I wonder whether we can't do this with another accessor (see also
   below).  I thought there are similar tests for updates.
 - If we need to use the direct SQLite3 operation, I'd like to make
   sure `db` is closed at the end of the test, no matter how it ends.
   So we need some guard/holder object to guarantee the cleanup.
 - do you mean "locked" here?
 {{{#!cpp
     // addZone should throw exception that it is locket
 }}}

 > > '''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.

 Can't we do this with (e.g.) SQLite3 and updater?  See also the
 previous point.

 And, on the revised version of the branch:

 '''database.cc'''

 - `TransactionHolder` destructor: the added comment seem to be a mere
   copy of the one for `~DatabaseUpdater()`.  According to DRY I'd keep
   only one of them, having the other just refer to it.

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