Add fix for ipa plugins command

[Akasurde]

Fix adds count of plugins loaded to return dict

Fixes https://fedorahosted.org/freeipa/ticket/6513

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

[martbab]

Thanks the patch makes the command work. However, the namespace names are returned as string, not unicode literals and thus the framework returns them as base64-encoded values:

# ipa plugins       
  ipaclient.plugins.automember.automember_add_condition: Q29tbWFuZA==, TWV0aG9k
  ipaclient.plugins.automount.automountlocation_import: Q29tbWFuZA==
  ipaclient.plugins.automount.automountlocation_tofiles: Q29tbWFuZA==, TWV0aG9k
  <snip />
# echo 'Q29tbWFuZA==' | base64 -d && echo
Command
# echo '# echo 'TWV0aG9k' | base64 -d && echo    
Method

One way to fix this is to wrap namespace name in six.test_type, this should work in both py2 and py3:

diff --git a/ipalib/misc.py b/ipalib/misc.py
index 264ec29..6234961 100644
--- a/ipalib/misc.py
+++ b/ipalib/misc.py
@@ -3,6 +3,9 @@
 #
 
 import re
+
+import six
+
 from ipalib import LocalOrRemote, _, ngettext
 from ipalib.output import Output, summary
 from ipalib import Flag
@@ -124,7 +127,7 @@ class plugins(LocalOrRemote):
             for plugin in self.api[namespace]():
                 cls = type(plugin)
                 key = '{}.{}'.format(cls.__module__, cls.__name__)
-                result.setdefault(key, []).append(namespace)
+                result.setdefault(key, []).append(six.text_type(namespace))
 
         return dict(
             result=result

[rcritten]

How about a test to prevent future regressions?

[MartinBasti]

Shouldn't be namespaces named using unicode strings?

[HonzaCholasta]

@mbasti-rh, no. Classes aren't named using unicode strings either.

[martbab]

@Akasurde are you OK with writing a simple regression test for this command as a part of this PR?

[Akasurde]

@martbab Yes, I will write a test case for this scenario and attach here.

[tiran]

Why should Python 2 class names are str instances prevent us from making the namespace keys text? In Python 2 ASCII str and ASCII unicode are equivalent dict keys (same hash, compare equaly). In Python 3 the keys are going to be text anyway.

Python 2 dicts with unicode are a tiny, tiny bit slower, but that doesn't really matter.

[HonzaCholasta]

The namespace keys are text (str) in both Python 2 and 3. The issue here is that the RPC layer assumes that str is binary data, which the patch correctly fixes by converting the keys to unicode before they enter the RPC layer. There is no benefit in making the keys themselves unicode.

[tiran]

In Python 2 str is a Chimera with the head of a text object and the body of a bytes object. It's just text if all text you got is ASCII. For clean polyglot code it's highly recommended to avoid Python 2 str and use Python 2's unicode for all text. Most of FreeIPA's Python code has been adopted to unicode for text very well. This one of the few places that slipped through.

The benefits are consistent treatment of text as Python 2 unicode, which leads to a proper fix instead of a patch (in this case decoding with six.text_type).

[HonzaCholasta]

@tiran, namespace keys are always ASCII. But feel free to open a ticket to convert all remaining uses of str as text to unicode, changing it for one random bit in this unrelated PR isn't particularly helpful when you take the big picture into account.

[tiran]

namespace keys are always ASCII and use the proper text datatype to represent text are not a contradiction.

At least two IPA developers (@MartinBasti and me) feel that it is strange to Python 2 str for name spaces keys. One of them just happens to be one of the Python core developers that made the text/bytes spilt in Python 3000 happen about 8, 9 year ago. wink wink, nudge nudge

The big picture is Python 3 support with working Python 2 support for the interim period. Python 2 code should follow Python 3 coding principals to give consistent results. This PR provides a quick patch to work around the symptoms of a flaw. It's not a solution for the core issue. Are we OK with a patch or do we prefer to understand and fix the root cause?

[HonzaCholasta]

We are OK with the patch because fixing the root cause is out of the scope of this PR.

[MartinBasti]

I would like to have py3 str <=> py2 unicode, py3 bytes <=> py2 str, but framework is far away from this ideal state.

So I have no strong opinion, and once we will drop py2, so I'm not sure if we want to migrate everything in py2 to unicode if it work in other cases.

[Akasurde]

@MartinBasti is there any action item pending on my side?

[MartinBasti]

NACK

This will generate wrong string in python3

>>> str(b'text')
"b'text'"

[MartinBasti]

you have to call .decode('utf-8')

[Akasurde]

@MartinBasti Done.

[MartinBasti]

@Akasurde sorry for delay, we still miss test. Otherwise I'm fine with this approach (when issue commented inline fixed)

[martbab]

You should use .decode('utf-8')' instead of .encode('utf-8') here.

[martbab]

Or rather avoid coercion to text type by directly decoding namespace:

result.setdefault(key, []).append(namespace.decode('utf-8'))

[Akasurde]

Oh!!! my bad.

[MartinBasti]

That test actually doesn't test output of command, IMO it should be xmlrpc_test too. But it can be done later, shouldn't block this PR

[MartinBasti]

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/b3c41f21e51e5389d95b5486dcdfdc3f9a8b0424