BIND 10 #1649: Config show allows listing non-existing sub-item in list

[BIND 10 Development]

#1649: Config show allows listing non-existing sub-item in list
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  vorner                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120221
                   Priority:  major  |            Resolution:
                  Component:  bind-  |             Sensitive:  0
  ctl                                |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  3
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jelte


Comment:

 Hello

 Replying to [comment:4 jelte]:
 > - first off I added a check for the above case, it should now report an
 error if a needed list index is missing. In the function that reports
 this, an additional argument can disable it (we need to have the ability
 not to specify indices when we are looking for a default value)

 I think the test kind of confirms it works for nested lists as well, but I
 don't see how it is possible, because the code:
 {{{#!python
          cur_el = _find_spec_part_single(cur_el, id_part)
          if strict_identifier and spec_part_is_list(cur_el) and\
               not isc.cc.data.identifier_has_list_index(id_part):
               raise isc.cc.data.DataNotFoundError(id_part +
                                                   " is a list and needs
 index")
          cur_el = _get_map_or_list(cur_el)
 }}}

 looks if it has any list index whereever in the identifier. Or do I
 understand it wrong and the identifier is just the one path element?

 Speaking about the `identifier_has_list_index`, is there any reason why it
 is directly inside `cc.data` and not in `config.data`, like the rest?

 > - to make it a bit more readable, i added a few helper functions; these
 might also be useful for the inevitable refactor of the config data stuff
 (the raw structures for both specs and data were chosen deliberately in
 the beginning IIRC, but I want to get rid of them and use classes, which
 should make both the code and fixes like this one (and any other behaviour
 changes) a lot simpler)

 I think we need the refactor rather soon. It probably won't happen in year
 3, but we really need it the first thing in year 4, because the code looks
 like a can of worms now after incrementally adding fixes for more and more
 corner cases. But also because the code is obviously unfixably broken, we
 have a report about this part of code misbehaving like every month.

 > - changed a number of cases where the code of these helper functions was
 used directly

 Maybe the `type(cur_spec) == dict` part is already included in the
 `spec_part_is_map` function?

 {{{#!diff
      # or a list (when it is the 'main' config_data element of a module).
 -    if type(cur_spec) == dict and 'map_item_spec' in cur_spec.keys():
 +    if type(cur_spec) == dict and spec_part_is_map(cur_spec):
          for cur_spec_item in cur_spec['map_item_spec']:
 }}}

 {{{#!diff
      raise isc.cc.data.DataNotFoundError(id + " not found")
 -    elif type(cur_spec) == dict and 'list_item_spec' in cur_spec.keys():
 +    elif type(cur_spec) == dict and spec_part_is_list(cur_spec):
      if cur_spec['item_name'] == id:
 }}}

 Also, I think the error message should be `Error: zone_config is a list
 and needs _an_ index`.

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