Issue 7391 - Let rpy recursively parse filter options to dicts

abp/filters/rpy.py

Index: abp/filters/rpy.py
===================================================================
--- a/abp/filters/rpy.py
+++ b/abp/filters/rpy.py
@@ -26,6 +26,27 @@
 __all__ = ['line2dict']
 
 
+def parse_options(options):

[Vasily Kuznetsov, 2019/03/22 12:17:04]

Maybe we should change this name to something like `option_list_to_dict`?
Because we're not really doing parsing here.

[rhowell, 2019/03/27 19:14:06]

Done.

+    """Recursively parse filter options into dicts.
+
+        Parameters

[Vasily Kuznetsov, 2019/03/22 12:17:04]

Nit: The indentation of this line seems to be off.

[rhowell, 2019/03/27 19:14:06]

Done.

+    ----------
+    options: A list of tuples or namedtuples

[Vasily Kuznetsov, 2019/03/22 12:17:04]

We're treating them as just tuples so we probably don't need to mention
nametuples at all. Or do we?

[rhowell, 2019/03/27 19:14:07]

Done.

+        The filter options
+
+    Returns
+    -------
+    dict
+        The resulting dictionary
+
+    """
+    result = dict(options)
+    if 'domain' in result:
+        result['domain'] = parse_options(result.get('domain'))

[Vasily Kuznetsov, 2019/03/22 12:17:04]

Nit: `result.get('domain')` could be just `result['domain']`

[Vasily Kuznetsov, 2019/03/22 12:17:04]

This conversion will lose the order of the domain options, which has
significance to how the filter is matched. Also the order of content type
related options is significant.

We need to check with Stephan whether losing this information here is ok. If
not, we can probably still represent it somehow in the dict form because the
only important part is whether the first domain is True or False (same for the
content type options).

@sporz: please comment on this.

[sporz, 2019/03/25 17:30:03]

Thank you Vasily for pointing this out - I wasn't aware that option order and
domain order have an impact on execution!

For our current specific use case this is not an issue. For python-abp as a
resource to be used by many teams, however, it might be.

When I suggested this, I had the following idea in mind: How can python-abp
provide a consistent data structure to be used by end users. As tuple2dict() was
used on the main level (resulting in a named list in R), I focused on trying to
provide that same data structure in the nested tuples as well. 

Two ideas:

* How about we keep tuples, and don't use tuple2dict at all?
* How about we use an ordered dict instead of normal ones?
  * Hopefully it's converted correctly into R! (R has ordered named lists)

[sporz, 2019/03/25 17:30:03]

Interesting choice to put this as a function on it's own. Can you elaborate?

I'd have incorporated it into the tuple2dict function (using a check for list of
tuples as value instead of (domain, options) as key as deciding factor for
recursion.

[rhowell, 2019/03/27 19:14:06]

I ran into a few issues trying to call tuple2dict recursively.

1) A list of tuples doesn't have the function `_asdict` (it seems only
namedtuples have this function built in):
    *** AttributeError: 'list' object has no attribute '_asdict'

2) If we try to convert the list of tuples into a namedtuple, we run into issues
with character encoding, since some of the keys are unicode:
    *** ValueError: Type names and field names can only contain alphanumeric
characters and underscores: "(u'a', True)"

3) If we try to get really hacky and switch from Unicode to Str, we might lose
some unicode characters, and also tuples are immutable:
    TypeError: "'tuple' object does not support item assignment"

It was just getting really hacky and messy, so I figured it would be better to
make a helper function.

But I'd like to clarify - do we want only the 'domains' in the 'options' to be
sub-dicts? Looks like the only other 'option' which might contain a sub-list is
sitekey, and it seems we want to leave that as a list?

[rhowell, 2019/03/27 19:14:07]

I was able to implement this using OrderedDict and briefly test it in R, and it
seems to be working - properly parsing the domain options and preserving the
order.

As far as just using tuples, I'm not sure what the desired input/output would
look like there. rpy does have access to the `parse_line` function, maybe
something closer to that? Or what did you have in mind? (Some example
input/output in R would be helpful for me.)
Filter(text=u'example.com', selector={u'type': u'url-pattern', u'value':
u'example.com'}, action=u'block', options=[])

+
+    return result
+
+
 def tuple2dict(data):
     """Convert a parsed filter from a namedtuple to a dict.
 
@@ -40,7 +61,16 @@
         The resulting dictionary
 
     """
-    result = dict(data._asdict())
+    result = {}

[Vasily Kuznetsov, 2019/03/22 12:17:04]

Maybe we just add `result['options'] = parse_options(result['options'])` after
old version of this line. What do you think?

[rhowell, 2019/03/27 19:14:07]

Yeah, good idea. We also need to check that 'options' is a key in the result.
Lines like Header, Comment, EmptyLine, and others don't have options, and would
produce a KeyError.

+
+    for key in data._fields:
+        name = key
+        value = getattr(data, key)
+        if 'options' in name:
+            result[name] = parse_options(value)
+        else:
+            result[name] = value
+
     result['type'] = data.__class__.__name__
 
     return result
@@ -62,10 +92,6 @@
     """
     if isinstance(data, dict):
         return {strings2utf8(k): strings2utf8(v) for k, v in data.items()}
-    if isinstance(data, list):

[Vasily Kuznetsov, 2019/03/22 12:17:04]

Unfortunately we can't remove the list branch here because sitekey option also
has a list value (and that one probably shouldn't be converted to a dict).

[rhowell, 2019/03/27 19:14:06]

Ah, gotcha. Since we didn't have a test using the sitekey option, these lines
weren't being tested, so tox was failing. I added a test for sitekey, so test
coverage is back at 100%.

But, sitekey options are not turned into a dict because line 44/46 only converts
'domain' options into a nested dict. So that should be ok?

[Vasily Kuznetsov, 2019/04/02 17:36:21]

Yeah, seems reasonable to keep it as a list, unless Stephan has other ideas.

-        return [strings2utf8(v) for v in data]
-    if isinstance(data, tuple):
-        return tuple(strings2utf8(v) for v in data)
     if isinstance(data, type('')):
         # The condition is a Python 2/3 way of saying "unicode string".
         return data.encode('utf-8')