BIND 10 #2436: update datasrc::ZoneLoader so it performs post load checks using validateZone()

[BIND 10 Development]

#2436: update datasrc::ZoneLoader so it performs post load checks using
validateZone()
-------------------------------------+-------------------------------------
            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:  3             |                 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):

 '''dependency note'''

 In my review on #2435, I raised a discussion about whether it make
 more sense to have the updater create a collection, rather than
 constructing a collection from an updater.  Depending on the result of
 the discussion, this branch may also have to be updated (although the
 effect should be pretty local and small).

 '''b10-loadzone'''

 We also need to update the man page.  It talks about the limitation on
 validation (post load checks) of the initial version.  It's now fixed.
 Also check the guide (I don't remember, but there may be the same
 issue).

 '''datasrc_messages.mes'''

 - Maybe the log ID is better named something like DATASRC_CHECK_ERROR
   or DATASRC_CHECKZONE_ERROR for terminology consistency.  same for
   WARN.
 - DATASRC_VALIDATE_WARNING: maybe we should say "loaded into the data
   source successfully" here, too, at the cost/risk of repeating.
   I'd also clarify that the new version of the zone is still effective
   and will be used in this case.

 '''novalidate.zone'''

 - Like the log message, maybe s/in validation/in post zone check/ or
   something?
 {{{
 ; Missing the NS here, will generate an error in validation
 }}}
   I'd use example.com or something instead of root (used name in the
   real world) if it's not difficult.

 '''zone_loader.cc'''

 - The `Name` can be a reference (avoiding copy) here:
 {{{#!cpp
         const dns::Name zone_name(updater_->getFinder().getOrigin());
 }}}
   (although the saved copy overhead may be marginal in this context).
 - what's the purpose of this?
 {{{#!cpp
             // Validation failed.
             loaded_ok_ = false;
 }}}
   loaded_ok_ doesn't seem to be used after this point anyway.
 - maybe changing "Validation" to "Post load checks" or something?
 {{{#!cpp
             // Validation failed.
 }}}
 - I'd suggest including name.h and rrclass.h explicitly, rather than
   relying on including them indirectly from some other headers.

 '''zone_loader.h'''

 - s/validate/pass content checks/ or something?
 {{{#!cpp
 /// \brief Exception thrown when the zone doesn't validate.
 }}}
 - I'd describe around here that the zone loading is effectively canceled:
 {{{#!cpp
     /// After the last RR is loaded, a sanity check of the zone is
 performed by
     /// isc::dns::validateZone. It reports errors and warnings by logging
 them
     /// directly. If there are any errors, a ZoneContentError exception is
     /// thrown.
 }}}

 '''zone_loader_unittest'''

 - This doesn't have to be `BasicRRset`.  A mere `RRset` suffices:
 {{{#!cpp
         RRsetPtr newRRset(new isc::dns::BasicRRset(rrset.getName(),
 }}}
 - maybe rename it to postLoadCheck or something:
 {{{#!cpp
 TEST_F(ZoneLoaderTest, loadValidation) {
 }}}
   same for the copy version.
 - I suggest adding cases where warnings are logged, even if we cannot
   (easily) confirm it via EXPECT_xx etc.  We can at least confirm
   warnings don't disrupt the loading (there may happen to be such
   cases in existing tests, in which case please leave comments).
 - in copyValidation, shouldn't even prepareSource() fail once we
   complete #2499?
 - shouldn't we check the previous version of zone is intact when check
   zone fails?
 - not for this branch, but I happened to notice:
 {{{#!cpp
         // Currently, load uses an urelated implementation. In the long
 term,
 }}}
   "urelated" seems to be a typo.  unrelated? (but I'm not sure what
   "unrelated implementation" means).

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