Opened 8 years ago

Closed 8 years ago

#1649 closed defect (fixed)

Config show allows listing non-existing sub-item in list

Reported by: vorner Owned by: jelte
Priority: medium Milestone: Sprint-20120221
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 1.18 Internal?: no

Description

From email https://lists.isc.org/pipermail/bind10-users/2012-February/000167.html:

If there's a list of maps, it is possible to list an item of the map as if it lived directly in the list node, not in the indexed one and shows default value:

> config show Xfrout/zone_config/transfer_acl
Xfrout/zone_config/transfer_acl[0]      {"action": "ACCEPT"}    any     (default)

The zone_config is a list, so there are items like zone_config[0]/transfer_acl, but zone_config/transfer_acl doesn't exist in reality. The value is the default which would be in the new item until changed. This should instead say that the item doesn't exist.

Subtickets

Change History (9)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 8 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20120221

comment:3 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:4 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review. As is usual in these parts of the code, I did a tiny bit more than just this fix;

  • 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)
  • 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)
  • changed a number of cases where the code of these helper functions was used directly

and while i was at it, i ran into another easy to fix issue, which has been bugging me for quite some time, and now became absolutely annoying;

  • it strips off the '/' at the end of config commands; this should not have effect on the behaviour itself, but does produce much nicer output; no more things like
    > config show Foo/
    Foo//bar1
    Foo//bar2
    

comment:5 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:6 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

Hello

Replying to 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:

         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?

     # 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']:
     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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

Replying to 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:

         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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jelte
  • Total Hours changed from 0 to 1.18

Hello

Replying to jelte:

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.

OK, I see.

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

ACK, I don't think we need multidimensional lists right now.

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)

Hmm, is it possible we'll get rid of these functions in the refactor? I don't think it is the best place for them, but I don't think we need to spend too much time moving them from place to place right now.

Anyway, I think this can be merged. But I just noticed there's no changelog and this bugfix would probably deserve one.

comment:9 in reply to: ↑ 8 Changed 8 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

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)

Hmm, is it possible we'll get rid of these functions in the refactor? I don't think it is the best place for them, but I don't think we need to spend too much time moving them from place to place right now.

Yes they are highly likely to be removed (at the very least as separate functions). Maybe I should try to find some time to write down the ideas i had :)

Anyway, I think this can be merged. But I just noticed there's no changelog and this bugfix would probably deserve one.

Thanks, merged, changelog discussied on jabber, closing ticket.

Note: See TracTickets for help on using tickets.