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:  muks
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  libdns++      |                    Milestone:
            Keywords:                |  Sprint-20130108
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  5             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hello

 Replying to [comment:10 muks]:
 > I think databases return a resultset handle (to an object) which is
 different from
 > a connection object (which is shared among many result sets). I am
 guessing such a
 > resultset handle could not be copied/shared as they are forward
 iterators
 > themselves and two copies will both move forward the same resultset
 iterator.
 > Behavior may very well vary depending on the database used too.

 Whatever it is, it hardly can be copied like this. So the current
 interface
 seems to limiting. I don't know if it's reasonable to merge it in this way
 if
 we know it would have to be changed in the next ticket anyway. One thing
 is
 when we don't know about a problem with interface and it is then changed
 as a
 result of discovering the problem. But now, we know about the problem, so
 we
 shouldn't have code we know is broken.

 > > Why is there the rrclass parameter both with the constructor and the
 add/remove
 > > interface, and not in find? I think it should be consistent and in
 case the
 > > rrclass is there, the collection should allow storing RRsets of
 different
 > > classes (which does make some sense) or the rrclass should be
 nowhere/possibly
 > > only in the constructor and different rrclass rrsets rejected in add.
 >
 > `RRClass` has been added to all `find()` variants now (and we use it in
 the collection key).

 Yes. Regarding that commit, the different orders of arguments quite scare
 me.
 For one, it is really annoying to remember why something has one order and
 something else a different one. But the bigger problem is, if they return
 different things, it is very easy to call the wrong one by accident.

 Is there any reason why we need two different versions anyway?

 > I may not follow what you mean by 'different types of collections' here.
 > I am assuming it is different implementations of `RRsetCollectionBase`.
 I guess we
 > won't be comparing two different iterators in our code like that, but
 regardless,
 > the code was incorrect as it could throw. `equals()` has been modified
 to return
 > inequality in this case without throwing, and I've also added a test
 with a mock
 > `RRsetCollectionBase` implementation for it.

 Yes, I meant that. From the interface point of view, it is now fixed. But
 I
 don't really like the way how it is fixed. This requires each
 implementator of
 each derived class to check it and not forget. Could that be checked on
 the
 base class level somehow?

 > >  * It would be nice to test various error cases, what happens with
 duplicate
 > >    RRset is add, etc.
 >
 > I am assuming it should throw when an rrset with the same class, type
 and name are added, so I've added such behaviour.

 I'm not completely sure about it. The `std::map` and `std::set` replace
 the
 original value. But it is probably reasonable to throw in that case, so
 this is
 OK.

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