Re: [Freeipa-devel] [PATCH] 0062 Don't crash when server returns extra output
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: 187220
GID: 187220
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: 187220
GID: 187220
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)):
Re: [Freeipa-devel] [PATCH] 0062 Don't crash when server returns extra output
On Thu, 2012-06-14 at 10:51 +0200, Petr Viktorin wrote: On 06/13/2012 11:40 PM, Rob Crittenden wrote: [snip] 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. I would add a new option for this purpose directly to ipa command, i.e. something like this: # ipa --all user-show admin Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0062 Don't crash when server returns extra output
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: 187220 GID: 187220 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. 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: 187220 GID: 187220 Account disabled: False Password: True Member of groups: admins, trust admins Kerberos keys available: True Unexpected output from server: new: new 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. Functionally it works pretty well. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0062 Don't crash when server returns extra output
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. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0062 Don't crash when server returns extra output
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
--
Petr³
From e039780d0038b57b621620eb429061a3fdfa311c 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 | 32 +--
tests/test_ipalib/test_frontend.py | 59 +++-
2 files changed, 67 insertions(+), 24 deletions(-)
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 10087ba24bc310a036a22d4c52740825de3184ce..e05638b2f42025338aa5c6f35e7166462ca8f196 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,16 @@ 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_dashed(unicode(_('Unexpected output from server:')))
+for item in sorted(extra):
+textui.print_line('%s: %s' % (item, output[item]))
+
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
