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:  vorner
  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 jelte):

 * owner:  jelte => vorner


Comment:

 Replying to [comment:6 vorner]:
 > 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?
 >

 the checking function can in theory work with both, but on the full
 identifier it would be pretty useless (at least without the entire
 specification as context). But it is only used on one path element at a
 time (id_part). So this should work with nested lists.

 BTW, It may not work fully with multi-dimensional lists, but I think we
 should be able to live with that for now (given the way bindctl handles
 lists in the first place, not sure if we ever want multi-dimensional ones,
 and even if so, I think that could wait until the refactor, see below, i
 agree it should be very very high on the agenda).

 > 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 keep it in the same location as the other code that defines and handles
 indices (whether or not the entirety should be moved can be discussed, but
 I did not want to make even more of a mess of it)

 > > - 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.
 >

 fully agree

 > > - 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?
 >

 oh yes, of course, removed the initial check

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

 ack, changed.

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