BIND 10 #2432: define and implement base and libdns++ version of RRsetCollection

[BIND 10 Development]

#2432: define and implement base and libdns++ version of RRsetCollection
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130108
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:22 jinmei]:
 > > Documentation added. For renaming to `Iterator`, it seems that for
 collections with `std::iterator`s, the preferred naming is
 `CollectionType::iterator` which is why it was named such. If you still
 think this should be changed to `Iterator`, please tell me and I'll update
 it.
 >
 > What's the source of the preference?  But, in any event, I'd leave it
 > to you two - I don't have a strong opinion.  But if the result is to
 > keep "iterator" (lower cased), please document why we don't follow our
 > style guideline here.  In fact, I guessed it might be related to the
 > fact that std iterator classes are named "iterator", but it's not
 > clear from the naming itself.

 I have renamed it to `Iterator` now as you are asking for it knowing it
 was named such due to `std::iterator` naming.

 > > > - I'd explain what kind of collection is constructed by the default
 constructor.
 > >
 > > I did not understand this.
 >
 > I meant describing the initial state of the collection if it's
 > constructed by the default constructor (there's no RR, etc).

 This is documented now.

 Replying to [comment:23 jinmei]:
 > > > And yet another...I'd explain the ownership of the passed rrset
 after
 > > > the call:
 > > > {{{#!cpp
 > > >     /// \brief Add an RRset to the collection.
 > > >     ///
 > > >     /// Does not do any validation whether \c rrset belongs to a
 > > >     /// particular zone or not. It throws an \c
 isc::InvalidParameter
 > > >     /// exception if an rrset with the same class, type and name
 already
 > > >     /// exists.
 > > >     void addRRset(isc::dns::RRsetPtr rrset);
 > > > }}}
 > >
 > > Done. :)
 >
 > Actually, I'd be more concerned about whether it's okay for the caller
 > to modify the rrset after calling `addRRset`.

 This is documented now.

 > And another thing:
 > {{{#!cpp
 >     /// \brief Remove an RRset from the collection.
 >     ///
 >     /// RRset(s) matching the \c name, \c rrclass and \c rrtype are
 >     /// removed from the collection.
 >     void removeRRset(const isc::dns::Name& name,
 >                      const isc::dns::RRClass& rrclass,
 >                      const isc::dns::RRType& rrtype);
 > }}}
 > what if the specified RRset doesn't exist in the collection?  At the
 > very least we should document it, and, personally, I prefer notifying
 > the caller as an error in some way (either via a return value or by
 > throwing an exception), not just ignore it.

 Instead of using an exception, I've made it return `bool` so that
 the caller can simply ignore what happens if required (by ignoring
 the return value).

 Replying to [comment:25 jinmei]:
 > And I just encountered a more serious issue.
 >
 > `RRsetCollection` rejects loading (on it construction) a zone (from a
 > file or a stream) if an RRset consists of multiple RRs.  It's too
 > restrictive.  It should allow that, and for that I suggest using
 > `RRCollator`.

 `RRCollator` is used now inside `RRsetCollection`.

 > Further, the behavior is still not good enough even with `RRCollator`,
 > because RRs can appear in any order, and can be interleaved.
 > `RRsetCollection` is intended to be used for general purpose, so it
 > should be generic enough.

 I want to ask, should we extend `RRCollator` to handle this case, or
 make another class? Or perhaps we can modify the `RRsets` directly
 in our internal `std::map` to avoid using another collection of
 sets during collation.

 Replying to [comment:26 jinmei]:
 > Hmm...and another critical point (not for the branch itself, but the
 > design/description)...I'm not sure why I described it that way in the
 > wiki page
 >
 (http://bind10.isc.org/wiki/ZoneLoadingAPIDesign#a2.2ClassesFunctionsforValidating),
 > but I don't think it feasible to return `AbstractRRset*` from
 > `RRsetCollectionBase::find()` because the ownership management will be
 > quite tricky.  In fact, the sample code for the datasrc case looks
 > buggy:
 > {{{#!cpp
 > class RRsetCollection : public RRsetCollectionBase {
 > public:
 >     RRsetCollection(ZoneUpdater& updater);
 >     virtual const AbstractRRset* find(const Name& name, RRType type)
 const {
 >         return (updater.getFinder().find(name, type,
 >
 NO_WILDCARD|FIND_GLUE_OK).get());
 >     }
 > };
 > }}}
 > because it's not guaranteed that the pointer stored in the result of
 > `ZoneFinder::find()` is valid after returning from
 > `RRsetCollection::find()`.
 >
 > I believe we should change the type of the return value to
 `ConstRRsetPtr`.

 I've removed the method that returns `const AbstractRRset*` and made one
 of the `(Const)RRsetPtr` methods of `isc::dns::RRsetCollection` be
 virtual.

 Returning to Jinmei for answer to the collation question.

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