BIND 10 #2298: Xfrout/zones and XML stats

[BIND 10 Development]

#2298: Xfrout/zones  and XML stats
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  naokikambe
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20121106
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  statistics                         |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  2
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by naokikambe):

 Replying to [comment:13 jinmei]:
 > Replying to [comment:12 naokikambe]:
 >
 > > Thank you for reviewing. But I'm not sure whether you are a real
 reviewer for this ticket. So I left it '!UnAssigned'. If this is wrong,
 please assign yourself.
 >
 > I noticed some minor things, but this chunk seems okay.
 >
 > The minor comments are:
 >
 > - This can be simplified:
 > {{{#!python
 >         keys = list(elem.keys())
 >         keys.sort()
 >         for key in keys:
 > }}}
 >   to
 > {{{#!python
 >         for key in sorted(elem.keys()):
 > }}}
 Thank you for suggestion. I didn't know that way. I fixed at:
 {{{
 27cb11a [2298_1] use sorted() instead of using sort() redundantly
 }}}


 >
 > - probably not a issue of this branch, but these two cases are bit
 >   awkward:
 > {{{#!python
 >         # for specifying empty strings in element and identifier
 >         self.assertEqual([],
 >                          stats_httpd.item_name_list('', ''))
 >         # for specifying wrong element, which is an non-empty string,
 >         # and an non-empty string in identifier
 >         self.assertRaises(isc.cc.data.DataTypeError,
 >                          stats_httpd.item_name_list, 'a', 'a')
 > }}}
 >   because the first parameter it not a dict but it somehow succeeds.
 This behavior depends on isc.cc.data.find(). As the following, this method
 always returns the first arg if the second arg is an empty string. However
 it raises ``!DataTypeError`` unless the first arg is dict and unless the
 second arg is an empy string.
 {{{#!python
 def find(element, identifier):
 ...
     if identifier == "":
         return element
     if type(element) != dict:
         raise DataTypeError("element in find() is not a dict")
 ...
 }}}

 >
 > - related to the previous point:
 >
 > > > - is it okay not to consider the case where ident is empty here?
 > > > {{{#!python
 > > >             idstr = '%s[%s]' % (ident, i)
 > > > }}}
 >   Actually it can happen as in this test:
 > {{{#!cpp
 >         self.assertRaises(isc.cc.data.DataTypeError,
 >                          stats_httpd.item_name_list, [1,2,3], '')
 > }}}
 >   and how it fails is not so trivial.  It fails in the recursive call
 >   to item_name_list() because now the 2nd parameter to find() is not
 >   empty.  I don't know if this means we need to do something in this
 >   branch, though.

 (Sorry, I forgot to reply.) Yes, but it's no problem in stats_httpd if
 this case fails.
 The top level data type should be dict. For example, this test is passed:
 {{{#!python
         self.assertEqual(['a', 'a[0]', 'a[1]', 'a[2]'],
                          stats_httpd.item_name_list({'a':[1,2,3]}, ''))
 }}}

 Replying to [comment:14 jinmei]:
 > ...and I've spent some time to try to understand the second chunk of
 > the diff (starting from a675312), but it seems to be beyond my
 > capability of comprehension.  The before-after diff is so big, and
 > tests are not very direct (meaning, not just passing some basic type
 > to the method and check the result using assertEqual), so I couldn't
 > even understand what this diff tries to do at the higher level, much
 > less being confident it's safe.

 In the second chunk, before-diff doesn't exactly match after-diff.
 xml_handler() is same name but the content is wholy different. Sorry for
 the confusion.

 >
 > At this point I think I have to give up reviewing this branch.  Could
 > you find someone else for the rest of the review?

 Thank you for reviewing! I'll look for another reviewer.

 Regards,

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