BIND 10 #551: wildcard handling for memory zone: find (main cases)

[BIND 10 Development]

#551: wildcard handling for memory zone: find (main cases)
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:  A-Team-
                 Priority:  major    |  Sprint-20110223
                Component:  data     |           Resolution:
  source                             |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  8.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:3 vorner]:

 For review, I've created a local branch from master, merged trac551
 and taken the diff from the branch point.

 > I'm not sure if it is correct in the way how the strange nested
 wildcards are handled (eg. wild.*.foo.example.org). See the tests, please,
 for that.

 The implementation looks correct, but I have some suggestions about
 more test cases.  See below.  I'd also suggest when you are not sure
 about specific behavior you check how other implementations do.  That
 would also be useful/helpful even if you believe you are doing the
 right thing.  Sometimes you find you were actually wrong (which is
 obviously useful), sometimes you find it might be controversial
 behavior in non standardized cases, and in some rare cases you even
 find a bug of other implementations.

 > I think we can add the changelog entry with #553, as this will complete
 the wildcard family of tasks.

 Yes, and I actually think the changelog will be about a complete "in
 memory data source (in terms of query handling (without DNSSEC)),
 giving credit to all A team developers.

 Detailed comments:

 '''memory_datasrc.cc'''
  - I noticed an editorial error in my original implementation (code
    and comment didn't match) and fixed it.
  - MemoryZoneImpl::find() now seems too big to understand.  I'd like
    to refactor it by (e.g) moving the PARTIALMATCH case outside the
    method.  But that would be beyond the scope of this ticket.  I'm
    okay with moving forward and leaving the refactoring to a separate
    task.
  - as for the note about RRSIG:
 {{{
             // TODO What about signatures? If we change the name, it would
 be
             // wrong anyway...
 }}}
    I wouldn't worry about it for now.  My expectation is that the
    protocol wise consideration for wildcard + RRSIG will be naturally
    implemented.

 '''memory_datasrc_unittest.cc'''
  - findTest()
   - maybe something like "actualIt" is better than "gotIt" (to be
     consistent with output from gtest)?  That may be a minor preference
     matter, though.
   - shouldn't the value of gotIt be find_result.rrset->getRdataIterator()?
     if so, it makes me think the tests might not be developed with the
     spirit of "test driven" (i.e., starting with failures)
   - I guess we should generalize the RDATA check in
     lib/testutils/. (but that would better be deferred to a separate
     task)
   - I guess one of the following is intended for RRclass:
 {{{
                     EXPECT_EQ(answer->getType(),
                         find_result.rrset->getType());
                     EXPECT_EQ(answer->getType(),
                         find_result.rrset->getType());
 }}}
  - this comment can now be removed?
 {{{
     // TODO:
     // glue name would match a wildcard under a zone cut: wildcard match
     // shouldn't happen under a cut and result must be PARTIALMATCH
     // (This case cannot be tested yet)
 }}}
  - I don't understand what "it will be here" means.
 {{{
     // Search at the parent. The parent will not have the A, but it will
 be here
 }}}
  - delegatedWildcard test should also check the GLUEOK case.
  - emptyWildcard should check 'wild.bar.foo.example.org' doesn't match.
  - nestedEmptyWildcard should test match and unmatch cases:
    - baz.foo.*.bar.example.org (should match)
    - baz.foo.baz.bar.example.org (should not match)
    - *.foo.baz.bar.example.org (should not match)
  - we should also test the case where we have *.example.com and
    foo.example.com, with checking bar.foo.example.com doesn't match.

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