BIND 10 #473: ModuleCCSession only validates config data, not commands

[BIND 10 Development]

#473: ModuleCCSession only validates config data, not commands
-------------------------------------+-------------------------------------
                 Reporter:  jelte    |                Owner:  jinmei
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:
                 Priority:  major    |           Resolution:
                Component:           |            Sensitive:  0
  configuration                      |  Add Hours to Ticket:  0
                 Keywords:           |          Total Hours:  0
Estimated Number of Hours:  2.0      |
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''General comments'''
  - you'll probably want to add a changelog entry for this
  - there are some long lines I'd fold (also per the BIND 9 coding
  guideline), e.g:
 {{{
                         if
 (module_specification_.validate_command(cmd_str, arg, errors)) {
 }}}
   I'd modify this to:
 {{{
                         if
 (module_specification_.validate_command(cmd_str,
                                                                    arg,
 errors)) {
 }}}
  - how about the python part? shouldn't we validate commands in
    ccsession.py, too?  Note, however, that xfrin and zonemgr (and
    perhaps others) depend on "hidden" commands not listed in the
    formal spec.  anyway, if we want to validate commands in python,
    too, I think it should go to a separate ticket.

 '''ccsession.cc'''
  - parseCommand
   - (not related to this change) is the comment above the function
     still valid?  it seems that if the given 'command' doesn't look like
     a command it throws an exception.  in any case, IMO this
     description (with correction if necessary) should be merged in the
     description in the header file.
  - ModuleCCSession::checkCommand()
   - should the new logic move to a separate function, e.g.,
     handleCommand()?  this is largely a matter of preference, but due
     to the additional logic checkCommand() now looks quite long, and
     mostly beyond an understandable level at a glance.
   - I hate to see a magic number:-)  What is this "3"? (in that sense
     I don't like the hardcoded "1" below this line, either)
 {{{
                             answer = createAnswer(3, ss.str());
 }}}

 '''ccsession.h'''
  - (not directly related to this change) I think the description could
    be more detailed.  for example, an example would be useful.  I'd
    also to know which exceptions can be thrown, if any (and there are
    some, actually).

 '''module_spec.cc (): ModuleSpec::validate_command()'''
  - style issue about the opening brace.  I've directly fixed it in the
    branch and pushed it (mainly for my education of git operation:-)
  - I believe this can/should be a const member function.
  - validate_command() should be named validateCommand() according to
    the guideline
  - I'd defer the introduction of commands_spec until it's really
    necessary:
 {{{
     ConstElementPtr commands_spec =
 module_specification->find("commands");
     if (!commands_spec) {
         // there are no commands according to the spec.
     ...
 }}}

 '''module_spec.h'''
  - I'd like to see more description.  An example would help.
    Error cases (in which case the given input is considered invalid)
    should also be described.

 '''Tests'''
  - generally looks okay, one minor point: the following command
 description seems to be something not intended due to copy-paste.
 {{{
 +        "command_name": "command_with_arg",
 +        "command_description": "A bad command",
 }}}

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