BIND 10 #2499: basic post-load validation for in-memory data source

[BIND 10 Development]

#2499: basic post-load validation for in-memory data source
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:  muks
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  data source   |                    Milestone:
            Keywords:                |  Sprint-20130122
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  4             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by muks):

 Hi Michal

 Replying to [comment:14 vorner]:
 > Some questions to the code:
 >  * Why does the Finder accept the origin as parameter, while the
 Iterator has one hardcoded? Shouldn't it be unified?

 Iterator's constructor now has an `origin` argument.

 >  * I don't think the whole TODO in `prepareSource` has been taken care
 of. Actually, it should have been taken care as a result of #2470, which
 made it not true, now it uses the same code, so it doesn't cross-check the
 implementation. I don't know if that's OK (but it wouldn't be in the scope
 of this ticket, I just ask for having a TODO with regard to it).

 I am sorry, I don't follow what is not taken care of. `InMemoryClient` now
 does the post-load check using `dns::checkZone()` in `load()` ->
 `loadZoneData()` -> `loadZoneDataInternal()`. After this test is
 introduced, the loading started failing. We no longer use the
 `InMemoryClient` for the source client for the test that depended on it
 not failing. This is what the old `prepareSource()` TODO comment said:
 {{{
 -        // TODO:
 -        // Currently, source_client_ is of InMemoryClient and its load()
 -        // uses a different code than the ZoneLoader (so we can cross-
 check
 -        // the implementations). Currently, the load() doesn't perform
 any
 -        // post-load checks. It will change in #2499, at which point the
 -        // loading may start failing depending on details of the test
 data. We
 -        // should prepare the data by some different method then.
 }}}

 What part of the TODO is not taken care of?

 >  * I don't think it is a good idea to take the test out of the text
 fixture. Not because it would need some variables from it, but having the
 related tests together makes it easier to filter them using
 `GTEST_FILTER`.

 Nod. I've moved it back to the same fixture.

 > > > And, I think there should be a test checking it throws on some
 invalid zones.
 > >
 > > I am not clearly following this. If it means that
 `ZoneLoaderTest.copyCheck` should be re-enabled, it is now.
 >
 > No, I meant that more zones are rejected now, during the load. A new
 test for the in-memory loader should be added and see that there's a zone
 that throws the new `ZoneValidationError` exception. Maybe I looked wrong,
 but I didn't see such test.

 There is a testcase that checks the `ZoneValidationError` exception case
 when an invalid zone is loaded. Different reasons when
 `ZoneValidationError` is thrown are not tested, but these are tested by
 `dns::checkZone()`. They are caught by the same code in
 `loadZoneDataInternal()` and this codepath is covered.

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