BIND 10 #1611: ZoneFinder::find refactoring: separate normal result codes and special case flags

[BIND 10 Development]

#1611: ZoneFinder::find refactoring: separate normal result codes and special case
flags
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120207
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:6 vorner]:

 Thanks for the prompt review.

 > First, is it OK that we're kind of inconsistent with python? When we had
 the previous version of FindResult, it acted mostly as a tuple. But now
 the C++ version has methods, while the python one has item in the tuple.
 But it is probably the easiest way to implement it.

 Good question.  I was aware of the mismatch, but I didn't fully think
 about it.  Honestly, I don't know what's the best.  In some sense
 using the tuple instead of a dedicated class is already a non trivial
 difference, so this would probably not so special.  I also thought
 that in dynamic languages like python it wouldn't make much sense to
 prevent "abuse" via interface - that's one slight reason I used a bare
 integer.  Also, in the vast majority cases Python applications
 wouldn't care about the optional flags information (they would
 basically want to know the simple result of whether a given name/type
 exists in the zone, and if it does, what it is in the form of RRset).
 On the other hand, I agree that it's generally better to keep these
 two interfaces as compatible as possible.

 If you have a specific suggestion, I'm happy to hear it.  Otherwise,
 it's probably okay to leave it open for now (especially for the last
 reason above).

 > Is the removal of `ZoneFinder::` from the front of FindResult returns in
 database.cc an unrelated cleanup or does it have any meaning in the scope
 of the ticket?

 Sorry for not being clear - it was just an unrelated cleanup.  I
 noticed they can be omitted in the same name scope (as a class) and it
 helped improve brevity.

 > Is this missing the NSEC3 flag?
 >
 > {{{#!c++
 > // For wildcard case with DNSSEC required, the caller would need to know
 > // whether it's NSEC or NSEC3 signed.  So we need to do an additional
 > // search here, even though the NSEC RR may not be returned.
 > if (wild && (options & FIND_DNSSEC) != 0 &&
 >     found.second.find(RRType::NSEC()) != found.second.end()) {
 >     flags = flags | RESULT_NSEC_SIGNED;
 > }
 > }}}

 Not really, but I admit the intent wasn't clear.  Right now we are not
 sure how we support NSEC3 in the database based backend, and how we
 set the NSEC3_SIGNED flag would depend on it.  Even for the NSEC case
 the additional search is probably not the best way (it's expensive,
 and for some half-broken zones it might be possible that the zone is
 signed with NSEC but some names miss corresponding NSEC RRs).  This
 fragment of code is the current workaround to adjust the
 implementation to the new interface with keeping compatibility, and I
 guess we should revisit it when we support NSEC for the database
 backend.  I've updated code comment so the intent is (hopefully)
 clearer.

 > This is not true in the python part, as it returns a tuple, not an
 object, so it has no methods:
 > {{{
 > The returned FindResult object can also provide supplemental\n\
 > information about the search result via its methods returning a\n\
 > boolean value. Such information may be useful for the caller if the\n\
 > caller wants to collect additional DNSSEC proofs based on the search\n\
 > result.\n\
 > }}}

 Ah, right.  I've re-checked the pydoc and found some other cases that
 mention FindResult.  I believe I've fixed all cases that had a
 mismatch on this point.

 > Why is this change included here? Because it is wrong, the python
 find_all doesn't have a target parameter (because return parameters are
 very unpythonish).
 > {{{#!diff
 >  Parameters:\n\
 > -  name       The domain name to be searched for.\n\
 > +  target     the successfull result is returned through this\n\
 >    options    The search options.\n\
 > }}}

 Oops, that was a mistake (I think).  Fixed it.

 > This is missing one combination (if there are 3 items, we can as well
 check comparing all of them):
 > {{{#!python
 > def test_findresultflags(self):
 >     self.assertNotEqual(ZoneFinder.RESULT_WILDCARD,
 >                         ZoneFinder.RESULT_NSEC_SIGNED)
 >     self.assertNotEqual(ZoneFinder.RESULT_NSEC_SIGNED,
 >                         ZoneFinder.RESULT_NSEC3_SIGNED)
 > }}}

 Fair enough, done.

 > Should this be checked for equality, so no extra flag is returned
 (multiple occurrences)
 > {{{#!diff
 >  self.assertEqual(finder.SUCCESS, result)
 > +self.assertTrue((flags & finder.RESULT_WILDCARD) != 0)
 >  self.assertEqual("foo.wild.example.com. 3600 IN A 192.0.2.255\n",
 > }}}

 Okay, done.

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