BIND 10 #1062: add a base for DatabaseZoneHandle::find()

[BIND 10 Development]

#1062: add a base for DatabaseZoneHandle::find()
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110816
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:9 jinmei]:
 > Replying to [comment:8 jelte]:
 >
 > A few preliminary notes:
 >
 > - (as always) I made some minor editorial changes
 > - please note that in #1061 it's likely we'll change "connection" to
 >   "accessor".  some of internal variable names, test cases, etc. may
 >   have to be adjusted as a result.

 Yes I noticed, I did a quick check yesterday and luckily git seems quite
 good at
 handling the changes for existing things. But yeah I'll still need to run
 through the things I added.

 > - I know comments below are not short, but most of them are quite
 >   minor or trivial to address.  The (almost) only real point to
 >   discuss is the use of try-catch in find().
 >
 > > > '''General Discussions'''
 > > >
 > > > - I agree with changing getNextRecord() to a non const member
 function
 > > >   for the reason you explained.  But this would also mean find()
 > > >   shouldn't be a cost member function of DatabaseClient::Finder (and
 > > >   therefore, of ZoneFinder).  [...]
 > >
 > > ok, unconsted it
 >
 > Hmm, it doesn't seem to be changed...
 >

 it appears I lost one of my commits :/ anyway. Unconsted again.

 > > > - Don't we need to use loggers in database and sqlite3_connection?
 > > >
 > >
 > > hmm, yes, completely forgot about those :/
 > >
 > > I added some in find(), it'll log either a successful return, or one
 of the
 > > errors as caught by the big try/catch introduced in this revision.
 >
 > Looks good, but I'd also include the class name and some identifier of
 > the underlying database (maybe we need a method in
 > DatabaseConnection?) for all these log messages, e.g.:
 > {{{
 >     logger.debug(DBG_TRACE_DETAILED,
 DATASRC_DATABASE_FIND_RECORDS).arg(name).arg(getClass()).arg(type).arg(connection_->getDBName());
 > }}}
 > (I assume a new method "getDBName()", which would result in something
 > like "sqlite3 (+ perhaps filename)")
 >

 I'm afraid putting in the full path may be a bit long, so I added yet
 another utility method to util/filename.h; getNameAndExtension, and the
 result
 for getDBName() is now "sqlite_<slitefile>". This can be ambiguous should
 there
 be more than one sqlite database with the same name in different
 directories,
 but I personally prefer it over full paths. Maybe we should make it
 something
 user-settable, esp if we want to support any number of
 any type of backend (where there may not be something like a file to base
 it
 on), and/or make it user-settable but default to something like this. But
 both
 of those are possible future enhancements :)


 > > > - in database.cc, I guess the exception/error handling needs to be
 > > >   revised. [...]
 > >
 > > I've implemented this approach for now, doing it with RAII is probably
 better,
 > > but may mean some other interface changes as well, and probably an
 entire
 > > different approach to the way we use the sqlite stmt variables right
 now
 > > (although i guess some of the potential problems i see would also be
 present
 > > now)
 >
 > The revised code seems to do the right thing, but I'm personally
 > concerned about the heavily nested try-catch logic:
 >


 Yes, we should do RAII, and we should probably do it in a form where
 searchForRecords returns some object you can call getNext() on (apparently
 michal is creating something similar for #1067, and the base class seems
 the
 same as what i need here).

 I am passing this back to you now though, though so you can look at these
 other
 changes


 > > I did add some tests that make sure resetSearch() (the cleanup method)
 is
 > > called, both on completion of a call to find() and on any exception
 that is
 > > raised.
 >
 > Regarding tests for resetSearch(): why are some of the tests commented
 > out?
 > {{{
 >     // Trigger the hardcoded exceptions and see if find() has cleaned up
 >     /*
 >     EXPECT_THROW(finder->find(isc::dns::Name("dsexception.in.search."),
 >                                               isc::dns::RRType::A(),
 >                                               NULL,
 ZoneFinder::FIND_DEFAULT),
 >                  DataSourceError);
 > ...
 > }}}
 >

 woops, they should not have been, fixed

 > > > '''database.h/cc'''
 > > >
 > > > - maybe we want to use a plain old array instead of vector for
 > > >   columns, if we are sure it has a fixed size? [...]
 > >
 > > Hmm, yes, a direct array is probably better, though also slightly
 > > more dangerous.  Therefore I also added a second argument, the
 > > vector size. Doesn't provide full protection against passing in a
 > > smaller array, but at least it's something; the dabaseimplementation
 > > should check it.
 >
 > If you like the efficiency of the array but want to make it safe, an
 > alternative would be to use boost::array (at the cost of the
 > additional dependency) and use at() instead of operator[] inside the
 > callee.  I don't have a strong opinion, and leave it to you.
 >

 no current array is fine imo, there has to be some method of foot-
 shooting, right ;)

 > > > - (general) I think it's better to define an enum or something to
 > > >   represent the semantics of the vector used in getNextRecord(),
 > > >   rather than hardcoding the magic numbers:
 > > > {{{
 > > >         if (columns.size() != 4) {
 > > > ...
 > > >             const isc::dns::RRType cur_type(columns[0]);
 > > >             const isc::dns::RRTTL cur_ttl(columns[1]);
 > > > }}}
 > >
 > > ack, done
 >
 > Looks good, but I guess RecordColumnCount should also be all upper
 > cased (with _, i.e. RECORD_COLUMN_COUNT) as it's a constant according
 > to the coding guideline.
 >

 ack

 > Also, this comment seems incomplete:
 > {{{
 >         TTL_COLUMN = 1,     ///< The TTL of the record (a
 > }}}
 >
 > > > - addOrCreate(): it's not clear how it's possible that type !=
 rrset->getType():
 > [...]
 > > >   (BTW, lcov indicates this case isn't covered by tests)
 > >
 > > yes well with the current code it shouldn't be possible, and since
 > > addOrCreate is in anonymous namespace we can't directly call it (or
 > > did we have a clean way to do that?), so it's not covered by a
 > > test...
 > >
 > > made it an assert.
 >
 > With the revised (with assert), I think code missing the test is okay.
 > One thing I didn't notice in the previous code: the assert() should
 > probably be better before the TTL check:
 > {{{
 >         // This is a check to make sure find() is not messing things up
 >         assert(type == rrset->getType());
 >
 >         if (ttl < rrset->getTTL()) {
 >             rrset->setTTL(ttl);
 >         }
 > }}}
 >

 Should not matter in practice, but yeah, is better :)

 > BTW maybe we want to log the event when we perform setTTL()?
 >

 good idea (long discussion on jabber room about level. no real consensus,
 making
 it INFO (but I can easily see it being DBG_TRACE_BASIC as well)

 btw, i also changed the if; if we have such a message we should also print
 it when the first TTL happens to be the lowest one


 > > > '''sqlite3_connection.cc'''
 > > >
 > > > - In searchForRecords(), sqlite3_bind_text() can fail (and
 > > >   sqlite3_bind_int() may also fail in theory?), and we should catch
 > > >   the case even though it would be a rare event.
 > >
 > > ack, done
 >
 > Looks good, but in #1068 I noticed we might rather use
 > sqlite3_errmsg() rather than including the raw result code.  That is,
 >
 > This way, we can eliminate the temporary variable, can make methods
 > shorter, and can provide more human readable what() message.
 >

 ack, changed

 > > > - convertToPlainChar(): (I'm asking partly because I forgot:-) in
 > > >   which case can ucp be NULL?  Also, the main purpose of this helper
 > > >   function is to work around the C++ strictness on the difference
 > > >   between 'unsigned char*' and 'char *' (sqlite3 uses the former
 > > >   throughout its API, while std::string accepts the latter).  And,
 > > >   from a point of type safe purist this conversion isn't correct
 > > >   (because in theory it's not guaranteed that these two pointer
 types
 > > >   are convertible).  I'd note these points as comments.
 > >
 > > IIRC the return value from sqlite_column_text() can be NULL if the
 database
 > > field itself was NULL or if it has run out of memory. [...]
 >
 > Ah, okay, I experimentally commented out the NULL check and saw the
 > tests triggered segfault:-)  Hmm, so, technically, I think we
 > should separate these two cases: if ucp == NULL and sqlite3_errcode()
 > is NOMEM we should probably throw an exception rather than silently
 > convert it to ""...
 >

 done. Added the dbparameters as an argument, needed for sqlite3_errcode

 >
 > > > - getNextRecord(): although it should be rare and might be purely in
 > > >   theory, this method can internally throw an exception (bad_alloc).
 > > >   we should probably catch that within this method and convert it to
 > > >   DataSourceError so that the application can handle that situation
 > > >   more gracefully (e.g., exit after closing the DB connection).
 > >
 > > done
 >
 > bad_alloc can happen only here:
 > {{{
 >                 columns[column] =
 convertToPlainChar(sqlite3_column_text(
 >                                                     current_stmt,
 column));
 > }}}
 > so we could narrow the try-catch scope.  But you might rather want to
 > avoid missing a non obvious case, so I'd leave it to you.
 >

 nah, reduced scope.

 > BTW, I now notice one another point in this method: we should probably
 > do validate column_count before sqlite3_step.
 >

 ack, moved.

 > Also note you may want to use sqlite3_errmsg() instead of playing with
 > the result code directly (see above).

 yes, though here it is slightly trickier, as we had a resetSearch() there
 (which
 made a couple of sqlite3_ calls that would cause the errmsg value to be
 either
 undefined or unrelated). But since we now catch the error in the calling
 method,
 and do resetSearch() there, I just removed the resetSearch() here.
 (the other option would be to copy the errmsg() result first)

 >
 > > > - not a big deal especially in tests, but if I understand it
 correctly
 > > >   "conn" can be scoped_ptr instead of shared_ptr:
 > >
 > > ack, changed. Actually, for countRecords, see below.
 >
 > Okay, and I suspect you can remove this:
 > {{{
 > #include <boost/shared_ptr.hpp>
 > }}}
 >

 yup


 > > >   - if not very hard, I'd check RDATA(s), not only the number of
 > > >     RDATAs and counts.  maybe we could use rrset[s]Check defined in
 > > >     lib/testutils/dnsmessage_test.h.
 >
 > This doesn't seem to be addressed.  Did you think it too hard?

 oh, oops, somehow read over this one. Added it by passing a vector of
 expected_rdatas and expected_sig_rdatas; the expected_counts that were
 there are
 now unnecessary (same as size() on those vectors), so i removed those as
 arguments. (i did not want to do something clever with the data as it is
 already
 in the database; as that could potentially cause internal problems we
 miss, like
 data not being checked at all)

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