BIND 10 #2435: implement datasrc version of RRsetCollection

[BIND 10 Development]

#2435: implement datasrc version of RRsetCollection
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130122
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 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 think the interface is now okay.  Remaining issues are mostly
 editorial/documentation/cleanup.  So I suggest merging the branch at
 this point so other tasks can move forward, and address the remaining
 issues separately.

 As for `RRsetCollectionPtr`, I'd personally rather remove it based on
 YAGNI, unless we are *sure* we'll need it in a near future update.

 '''datasrc/rrset_collection_base.h'''

 - This shouldn't be necessary if you include zone.h:
 {{{#!cpp
 /// \brief A forward declaration
 class ZoneUpdater;
 }}}
 - maybe a matter of wording, but to me this `RRsetCollectionBase`
   looks like the "default implementation" rather than an abstract
   class.
 {{{#!cpp
 /// This is an abstract class that adds datasrc related detail to
 /// \c isc::dns::RRsetCollectionBase. Derived classes need to complete
 /// the implementation (add iterator support, etc.) before using it.
 }}}
   Although it's not clear what we'll need to do for the iterator case,
   it may still be possible to provide the default implementation only
   using public updater interfaces.
 - do `getBeginning` and `getEnd` have to be declared as pure virtual?
   These are inherited from the very base class and are already pure
   virtual.  It may also be related to the question of "default
   implementation" vs "abstract class" (above).  Especially if we
   consider it the former, I guess we'd rather add the default
   implementation (which currently just throws) in this "base" class.

 - documentation of this class (design decisions, restrictions, how
   it's expected to be used, etc) still seems to be insufficient.  in
   general, it should contain the detailed points we've discussed in
   this ticket.

 '''database.cc'''

 - this is not very accurate:
 {{{#!cpp
 /// \brief datasrc implementation of RRsetCollectionBase.
 }}}
   it's specific to DBs
 - destructor doesn't have to be defined explicitly in this case.

 '''zone.h'''

 - not necessary:
 {{{#!cpp
 /// \brief A forward declaration
 class RRsetCollectionBase;
 }}}

 - I'd like to see an explanation of why we have this restriction:
 {{{#!cpp
     /// Implementations of \c ZoneUpdater may not allow adding or
     /// deleting RRsets after \c getRRsetCollection() is called.
 }}}
 - Likewise.  And also, if this means the user of the base class
   shouldn't use the collection after commit (unless it knows a
   particular derived implementation that loosens the restriction),
   I'd note that explicitly.
 {{{#!cpp
     /// Implementations of \c ZoneUpdater may disable a previously
     /// returned \c RRsetCollection after \c commit() is called. If an
     /// \c RRsetCollection is disabled, using methods such as \c find()
     /// and using its iterator would cause an exception to be
     /// thrown. See \c isc::datasrc::RRsetCollectionBase for details.
 }}}
 - Likewise as the above two points.
 {{{#!cpp
     /// Implementations of \c ZoneUpdater may not allow adding or
     /// deleting RRsets after \c getRRsetCollection() is called. In this
     /// case, implementations throw an \c InvalidOperation exception.
 }}}
   (There are two copies of this text, btw)

 '''database_unittest.cc'''

 - I'd avoid C-style cast:
 {{{#!cpp
     (void) this->updater_->getRRsetCollection();
 }}}
   In this case you could probably use a simple (though redundant in
   some sense) test using find(), for example.

 ''' (dns)rrset_collection_base.h'''

 - s/an/a/ ?
 {{{#!cpp
 /// This exception is thrown when an calling implementation of
 }}}

 - I suspect this is not the main difficulty of RRSIG.  One fundamental
   issue is that it's not clear whether we want to return all RRSIGs
   of the given name covering any RR types (in which case how), or we
   need to extend the interface so we can specify the covered type.
   Also, I suspect the simple libdns++ version of collection could
   actually match RRSIG RRsets because its add interface doesn't reject
   the direction addition of RRSIGs.  So, what should be documented is
   that we have such fundamental open questions, and that a specific
   derived implementation may happen to return something if type RRSIG
   is specified, but nothing can be assumed as the base class interface
   (and we might say we may clarify and implement it in the future)
 {{{#!cpp
     /// This method's implementations currently are not specified to
     /// handle \c RRTypes such as RRSIG and NSEC3. RRSIGs are attached
     /// to their corresponding \c RRset and it is not straightforward to
     /// search for them. Searching for RRSIGs will return \c false
     /// always. Support for RRSIGs may be added in the future.
 }}}
 - This doesn't seem to be very accurate.
 {{{#!cpp
     /// Non-concrete types such as ANY and AXFR are unsupported and will
     /// return \c false always.
 }}}
   If that's the case, we should implement derived classes that way and
   test the behavior (but I don't think it's the case).  Similar to the
   case of RRSIG, the fact is that specific implementation may return
   something for these (we can create a type AXFR RRset and pass it to
   libdns++ collection, for example).  But, unlike the case of RRSIGs,
   these types of RRsets are not expected to be included in any
   implementation of collection in the first place (by definition of
   "meta types"), so asking for such types is basically an invalid
   operation.  While the API doesn't require implementations check this
   condition (and reject it), it should be considered an "undefined
   behavior".  (and that wouldn't change in future versions).

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