[Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types

Wed Nov 23 08:52:39 UTC 2011

On Mon, 2011-11-21 at 17:10 +0100, Jan Cholasta wrote:
> Dne 11.11.2011 20:07, Rob Crittenden napsal(a):
> > Jan Cholasta wrote:
> >> Dne 4.11.2011 22:25, Rob Crittenden napsal(a):
> >>> Jan Cholasta wrote:
> >>>> Dne 24.10.2011 17:42, Rob Crittenden napsal(a):
> >>>>> Jan Cholasta wrote:
> >>>>>> Dne 20.10.2011 13:20, Jan Cholasta napsal(a):
> >>>>>>> Parse comma-separated lists of values in all parameter types. This
> >>>>>>> can
> >>>>>>> enabled for a specific parameter by setting the "csvlist" option to
> >>>>>>> True.
> >>>>>>>
> >>>>>>> Remove List parameter type and replace all occurences with Str with
> >>>>>>> csvlist enabled.
> >>>>>>>
> >>>>>>> https://fedorahosted.org/freeipa/ticket/2007
> >>>>>>>
> >>>>>>> This change will be useful for
> >>>>>>> https://fedorahosted.org/freeipa/ticket/1487 and
> >>>>>>> https://fedorahosted.org/freeipa/ticket/1847
> >>>>>>>
> >>>>>>> Unit tests show no regressions.
> >>>>>>>
> >>>>>>> Honza
> >>>>>>>
> >>>>>>
> >>>>>> Self-NACK - I have noticed that the batch command no longer works.
> >>>>>>
> >>>>>> Updated patch attached.
> >>>>>>
> >>>>>> Honza
> >>>>>
> >>>>> What is the benefit of this over the List parameter type?
> >>>>>
> >>>>> rob
> >>>>
> >>>> Mainly because the List parameter type is just a hack. This is the
> >>>> right
> >>>> thing to do if we want to use comma-separated lists of parameters of
> >>>> any
> >>>> type, with all the validation and other parameter type-specific
> >>>> features.
> >>>>
> >>>> For example, I've added a new parameter type for IP addresses in my
> >>>> patch 46
> >>>> (http://www.redhat.com/archives/freeipa-devel/2011-September/msg00187.html)
> >>>>
> >>>>
> >>>>
> >>>> and use it for A and AAAA DNS records. Without this patch, we can
> >>>> either
> >>>> use List for the record parameters and lose validation in
> >>>> dnsrecord-find
> >>>> (because it is based on crud.Search, which strips all the custom
> >>>> validation rules - like _validate_ipaddr - from the command parameters,
> >>>> which is one of the causes of #1627) or use IPAddress for the record
> >>>> parameters and lose the ability to specify them as comma-separated list
> >>>> of values. With this patch, we can have both comma-separated lists and
> >>>> validation at the same time.
> >>>>
> >>>> Besides, the patch is not as big as it looks like, all the interesting
> >>>> stuff is in ipalib/parameters.py, everything else is just
> >>>> search-and-replace. Also I need it to fix #1487 and #1847 without doing
> >>>> ugly hacks.
> >>>>
> >>>> Honza
> >>>>
> >>>
> >>> I think this would constitute a major version change.
> >>
> >> I'm not sure about that, as the patch doesn't break API compatibility -
> >> a string containing a comma-separated list of values is used for list
> >> parameters both with and without the patch.
> >>
> >>>
> >>> One downside is you can no longer tell in the help with arguments take a
> >>> CSV and which don't.
> >>
> >> Why not? A simple look at csvlist value should provide enough
> >> information.
> >>
> >>>
> >>> I think the CSV-related Parameter options should all begin with csv,
> >>> separator and skipspace.
> >>
> >> OK.
> >>
> >>>
> >>> The batch command may eventually be made into a command, how will that
> >>> affect the Any type?
> >>
> >> Batch currently uses List for the "methods" parameter not because of its
> >> CSV capability, but because it doesn't do any type conversion and
> >> validation on the values. This allows it to use values of the form
> >> "{u'params': [args_list, kwargs_dict], u'method': method_name}". The Any
> >> parameter type exists so that this can still be done without List - it's
> >> basically a single-valued version of List (i.e. Any(csvlist=True) is
> >> equivalent to List()).
> >>
> >> IMO if batch is ever made into a command, it would have to be redesigned
> >> not to use List/Any, so that it can be used from CLI with validation of
> >> the parameter values. Any can then be easily removed.
> >>
> >>>
> >>> It otherwise seems to work in my spot-testing.
> >>>
> >>> rob
> >>
> >> Honza
> >>
> >
> > Given that we want to maintain backward API compatibility can you make
> > sure older clients will work with this? My gut tells me it will since
> > this is really a marshaling issue but I don't want to assume.
> 
> Just tested a few commands that use CSV (selfservice-find, dnszone-add, 
> dnszone-del) from an unpatched client and everything seems to be working 
> fine.
> 
> >
> > thanks
> >
> > rob
> 
> I have updated the patch with your suggestions and rebased it on top of 
> current master. See attachment.
> 
> Honza
> 

Your patch crashes the ./make-lint script:

# git apply freeipa-jcholast-55.2-comma-separated-list.patch
# ./make-lint 
Traceback (most recent call last):
  File "./make-lint", line 239, in <module>
    sys.exit(main())
  File "./make-lint", line 210, in main
    linter.check(files)
  File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 493, in check
    self.check_astng_module(astng, walker, rawcheckers)
  File "/usr/lib/python2.7/site-packages/pylint/lint.py", line 565, in check_astng_module
    walker.walk(astng)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 524, in walk
    self.walk(child)
  File "/usr/lib/python2.7/site-packages/pylint/utils.py", line 521, in walk
    cb(astng)
  File "./make-lint", line 98, in visit_getattr
    ignored = self._find_ignored_attrs(owner)
  File "./make-lint", line 82, in _find_ignored_attrs
    for klass in self._related_classes(owner):
  File "./make-lint", line 74, in _related_classes
    for base in klass.ancestors():
  File "/usr/lib/python2.7/site-packages/logilab/astng/bases.py", line 48, in __getattr__
    return getattr(self._proxied, name)
AttributeError: 'Function' object has no attribute 'ancestors'

Besides that, I consider this patch very useful. It is also a
prerequisite for my new DNS API patch as I use custom non-List based
parameter types and I need CSV functionality for them.

Martin