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          |  vorner
            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 => vorner


Comment:

 Hi Michal

 Replying to [comment:8 vorner]:
 > While it is nice to have an std iterator interface, I'm not sure it is
 working
 > out well. If I don't count the magic with shared pointers (which I don't
 think
 > I can do better), I think it'll be a real challenge with stuff like the
 data
 > sources, or a database. Imagine you have a database handle and you
 invoke a
 > statement and then you try to iterate through it. But then, if you do
 the
 > postfix version of iterator, what do you do with the handle? Or, if you
 copy
 > the iterator and try to iterate independently with each copy. I don't
 think the
 > database handle can be „forked“ like this. I'm not sure I'm explaining
 my mind
 > well enough.

 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.

 I think such an issue is best investigated when we write the datasrc
 implementation.
 It should be easy to remove the std iterator compliance from the interface
 (whether copies of iterators should be allowed, influencing the postfix
 operator++).

 > Another high-level thing is the only constructor of the
 `RRsetCollection` takes
 > the filename to master file. This is quite limiting. What if I didn't
 want to
 > create one by loading, but wanted an empty one and fill some data in in
 some
 > other ways?

 Good point. A no-argument constructor has been added now.

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

 > Also, there are some implementation comments as well as design ones.
 >  * With the `operator ==` and `operator !=`, how are iterators from
 different
 >    types of collections handled? Which brings me to the concrete
 implementaion
 >    of `equal` from the libdns++ implementation, which would throw in
 case
 >    a wrong type is passed there. I'd guess the intuitive behaviour would
 be
 >    to return false (as they aren't the same, because they are of
 different type).

 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.

 >  * I'm not sure if it is possible in the datasrc case, but I'd expect
 the
 >    find, begin and end to be const, because they return const RRsets. Or
 don't they?

 I don't know how these are meant to be used. Here again, I think this is
 better
 investigated when we write the datasrc implementation. If they become
 `const`,
 the `iterator` implementations will have to be changed to
 `const_iterator`s.

 >  * Do we really? `// We just ignore errors and warnings.`

 The comment was not clear and has been updated. We ignore the callbacks.

 >  * The tests findBase, find and findConst look very similar to me. What
 is the
 >    difference there?

 I have unified the `find()` and `findConst()` tests. `findBase()` uses a
 different
 order of arguments and returns a different type, so it's better off as a
 different test.
 I guess you already know the differences (and wanted me to unify the
 tests), but in case
 you really asked how they are different, `findConst()` tests the `const`
 method on a
 `const RRsetCollection`.

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


 Replying to [comment:9 vorner]:
 > The `rdata_collection_base.cc` is effectively empty. Is it needed there?
 And,
 > also, is it really possible to claim copyright of mostly empty file?

 This was initially written and left in by mistake. It has been removed.

 > And, another style thing. I don't think it is reasonable to do anything
 about
 > it now, but for the next time, is it possible to commit by smaller
 chunks? For
 > example, separate the base class and the concrete implementation?

 I did think about splitting it up into chunks, but the tests are
 interwined.
 I had initially written most of the class interface and the iterator
 interface (`.h` files)
 to understand how they could fit together. `RRsetCollection`'s interface
 is minimal, and
 the tests use much of it in every test to check if the methods behave
 correctly. So it all
 went into a single commit.

 OTOH, it is a fresh set of classes and tests that does not touch anything
 else which is already there.

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