On 06/13/2012 11:40 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 06/12/2012 02:38 PM, Simo Sorce wrote:
On Tue, 2012-06-12 at 13:12 +0200, Petr Viktorin wrote:
This will make older clients usable if new output items get added to
commands.

Since there might be important information in the extra output, it's
not
ignored as the ticket asks. Instead it's printed, but not formatted
nicely as the client doesn't have enough info for that.

https://fedorahosted.org/freeipa/ticket/1721

Patch is missing.

Simo.


My apologies

I'd replace the print_line with print_indented so the output looks a
little nicer.

This sure does make an impression. It looks something like this (with
print_indented):

$ ipa user-show admin
User login: admin
Last name: Administrator
Home directory: /home/admin
Login shell: /bin/bash
UID: 1872200000
GID: 1872200000
Account disabled: False
Password: True
Member of groups: admins, trust admins
Kerberos keys available: True
------------------------------
Unexpected output from server:
------------------------------
new: new

It's hard to argue with this as being descriptive it just seems a bit
overbearing.

I have a couple of ideas on this.

1. We could detect and supress unexpected output by default and include
a note at the end, something like:

Unexpected output suppressed, use --all to show.

That would work with show/find, but you can't just re-run add/mod commands.

2. Replace the print_dashed with print_line and embed a \n in the value
so it would look like:

$ ipa user-show admin
User login: admin
Last name: Administrator
Home directory: /home/admin
Login shell: /bin/bash
UID: 1872200000
GID: 1872200000
Account disabled: False
Password: True
Member of groups: admins, trust admins
Kerberos keys available: True

Unexpected output from server:
new: new

I went with an extra print instead of the '\n' (it won't confuse translators as much).

I also now use print_plain instead of print_line, so the output doesn't get truncated.

I think we'll need to document this somewhere in any case, explaining
how this situation can happen. I think it could be very confusing.

I added a “please upgrade” message at the end. That should make the situation clear.

Functionally it works pretty well.

rob


--
Petr³
From 45137be38da224df7f2a060cc602d6f0ad1a02c0 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Tue, 5 Jun 2012 10:26:35 -0400
Subject: [PATCH] Don't crash when server returns extra output

When input new optional parameter is added to server API, its still
usable with old clients with old API, it just cannot use the new parameter.
However, when a new Output parameter is added, older clients immediately
failed with a validation error.

This patch relaxes the validation so that extra Output items are allowed
in the client.
The extra items are displayed at the end of a command's output (crudely
formatted and with raw names, since the older client doesn't have any
formatting info).
This should ensure that important messages are not lost.

On the server, the validation still does not allow extra Output items.

Also, fix an error where if the expected and actual number of Output
items was the same, validation succeeded even if the keys were different.

https://fedorahosted.org/freeipa/ticket/1721
---
 ipalib/frontend.py                 |   35 +++++++++++++++------
 tests/test_ipalib/test_frontend.py |   59 +++++++++++++++++++++++++++---------
 2 files changed, 70 insertions(+), 24 deletions(-)

diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 10087ba24bc310a036a22d4c52740825de3184ce..3d997bc5b6772e09ace9a50bfeb09a804c066f8d 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -904,18 +904,22 @@ def validate_output(self, output):
             raise TypeError('%s: need a %r; got a %r: %r' % (
                 nice, dict, type(output), output)
             )
-        if len(output) < len(self.output):
-            missing = sorted(set(self.output).difference(output))
+        missing = set(self.output).difference(output)
+        if missing:
             raise ValueError('%s: missing keys %r in %r' % (
-                nice, missing, output)
-            )
-        if len(output) > len(self.output):
-            extra = sorted(set(output).difference(self.output))
-            raise ValueError('%s: unexpected keys %r in %r' % (
-                nice, extra, output)
+                nice, sorted(missing), output)
             )
+        if self.api.env.in_server:
+            extra = set(output).difference(self.output)
+            if extra:
+                raise ValueError('%s: unexpected keys %r in %r' % (
+                    nice, sorted(extra), output)
+                )
         for o in self.output():
-            value = output[o.name]
+            try:
+                value = output[o.name]
+            except KeyError:
+                continue
             if not (o.type is None or isinstance(value, o.type)):
                 raise TypeError('%s:\n  output[%r]: need %r; got %r: %r' % (
                     nice, o.name, o.type, type(value), value)
@@ -999,6 +1003,19 @@ def output_for_cli(self, textui, output, *args, **options):
             elif isinstance(result, int):
                 textui.print_count(result, '%s %%d' % unicode(self.output[o].doc))
 
+        # If there is extra output from the server, it probably means that
+        # the server is newer and an output item was added.
+        # Since the item might be important, we show it even though we can't
+        # format it correctly
+        extra = set(output).difference(self.output)
+        if extra:
+            textui.print_plain('')
+            textui.print_plain(unicode(_('Unexpected output from server:')))
+            for item in sorted(extra):
+                textui.print_indented('%s: %s' % (item, output[item]))
+            textui.print_plain(unicode(
+                _('Please upgrade the IPA client on this machine')))
+
         return rv
 
     # list of attributes we want exported to JSON
diff --git a/tests/test_ipalib/test_frontend.py b/tests/test_ipalib/test_frontend.py
index b717a43ad4c68940582f901308f4dd4da77834ee..dcdea694a34adbf0e351cbfa924b2f2b0666b06c 100644
--- a/tests/test_ipalib/test_frontend.py
+++ b/tests/test_ipalib/test_frontend.py
@@ -610,69 +610,98 @@ def forward(self, *args, **kw):
         assert o.run.im_func is self.cls.run.im_func
         assert ('forward', args, kw) == o.run(*args, **kw)
 
-    def test_validate_output(self):
+    def test_validate_output_basic(self):
         """
         Test the `ipalib.frontend.Command.validate_output` method.
         """
-        class Example(self.cls):
+        class example(self.cls):
             has_output = ('foo', 'bar', 'baz')
 
-        inst = Example()
-        inst.finalize()
+        (api, home) = create_test_api(in_server=True)
+        api.register(example)
+
+        api.finalize()
+        inst = api.Command.example
 
         # Test with wrong type:
         wrong = ('foo', 'bar', 'baz')
         e = raises(TypeError, inst.validate_output, wrong)
         assert str(e) == '%s.validate_output(): need a %r; got a %r: %r' % (
-            'Example', dict, tuple, wrong
+            'example', dict, tuple, wrong
         )
 
         # Test with a missing keys:
         wrong = dict(bar='hello')
         e = raises(ValueError, inst.validate_output, wrong)
         assert str(e) == '%s.validate_output(): missing keys %r in %r' % (
-            'Example', ['baz', 'foo'], wrong
+            'example', ['baz', 'foo'], wrong
         )
 
-        # Test with extra keys:
+        # Test with extra keys (api.env.in_server is set, so this raises):
         wrong = dict(foo=1, bar=2, baz=3, fee=4, azz=5)
         e = raises(ValueError, inst.validate_output, wrong)
         assert str(e) == '%s.validate_output(): unexpected keys %r in %r' % (
-            'Example', ['azz', 'fee'], wrong
+            'example', ['azz', 'fee'], wrong
         )
 
+        # Test with different keys:
+        wrong = dict(baz=1, xyzzy=2, quux=3)
+        e = raises(ValueError, inst.validate_output, wrong)
+        assert str(e) == '%s.validate_output(): missing keys %r in %r' % (
+            'example', ['bar', 'foo'], wrong
+        ), str(e)
+
+    def test_validate_output_per_item(self):
+        """
+        Test the `ipalib.frontend.Command.validate_output` method.
+        """
         # Test with per item type validation:
-        class Complex(self.cls):
+        class complex_command(self.cls):
             has_output = (
                 output.Output('foo', int),
                 output.Output('bar', list),
             )
-        inst = Complex()
-        inst.finalize()
+        (api, home) = create_test_api(in_server=False)
+        api.register(complex_command)
+
+        api.finalize()
+        inst = api.Command.complex_command
 
         wrong = dict(foo=17.9, bar=[18])
         e = raises(TypeError, inst.validate_output, wrong)
         assert str(e) == '%s:\n  output[%r]: need %r; got %r: %r' % (
-            'Complex.validate_output()', 'foo', int, float, 17.9
+            'complex_command.validate_output()', 'foo', int, float, 17.9
         )
 
         wrong = dict(foo=18, bar=17)
         e = raises(TypeError, inst.validate_output, wrong)
         assert str(e) == '%s:\n  output[%r]: need %r; got %r: %r' % (
-            'Complex.validate_output()', 'bar', list, int, 17
+            'complex_command.validate_output()', 'bar', list, int, 17
         )
 
+        # Test with extra keys (api.env.in_server not set, no error raised)
+        wrong = dict(foo=1, bar=[2], baz=3, fee=4, azz=5)
+        inst.validate_output(wrong)
+
+    def test_validate_output_nested(self):
+        """
+        Test the `ipalib.frontend.Command.validate_output` method.
+        """
         class Subclass(output.ListOfEntries):
             pass
 
         # Test nested validation:
         class nested(self.cls):
             has_output = (
                 output.Output('hello', int),
                 Subclass('world'),
             )
-        inst = nested()
-        inst.finalize()
+        (api, home) = create_test_api(in_server=False)
+        api.register(nested)
+
+        api.finalize()
+        inst = api.Command.nested
+
         okay = dict(foo='bar')
         nope = ('aye', 'bee')
 
-- 
1.7.10.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to