On 07/19/2016 01:25 PM, Martin Babinsky wrote:
On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:
On Mon, 18 Jul 2016, Martin Babinsky wrote:
On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, Alexander Bokovoy wrote:
> > > Hi,
> > > > > > I had some time and was blocked by these bugs to do my
tickets so I
> > > actually fixed these three problems that are assigned to Martin
> > > Babinsky. Hopefully, Martin wouldn't be offended by that. :)
> > > > > > ------
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.
Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.

We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.


I have botched the wording in the comment, attaching updated patch.

--
Martin^3 Babinsky
From 5b45004ffac840aedec989728321afeeeff15ff6 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt                         |  8 ++++----
 VERSION                         |  4 ++--
 ipalib/output.py                | 10 ++++++++++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py      |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..cbe23f4bde3a29cf3f28a9e361f83e176ede08e0 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
-output: PrimaryKey('value')
+output: Output('value', type=[<type 'unicode'>])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
-output: PrimaryKey('value')
+output: Output('value', type=[<type 'unicode'>])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
-output: PrimaryKey('value')
+output: Output('value', type=[<type 'unicode'>])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5584,7 +5584,7 @@ option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', val
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
-output: PrimaryKey('value')
+output: Output('value', type=[<type 'unicode'>])
 command: trustdomain_add/1
 args: 2,8,3
 arg: Str('trustcn', cli_name='trust')
diff --git a/VERSION b/VERSION
index 0559741451a858dd0adfa99a8bf653261d771601..ca489965050f32d2d8987dfd251ec2b2a0ba1768 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=210
-# Last change: Add --ca option to cert-status
+IPA_API_VERSION_MINOR=211
+# Last change: mbabinsk: allow 'value' output param in commands without primary key
diff --git a/ipalib/output.py b/ipalib/output.py
index 19dd9adadeb8521caf9f0dc52981ce57a7f0c8b6..b104584631629f33280164dd1d23922d21ddea49 100644
--- a/ipalib/output.py
+++ b/ipalib/output.py
@@ -217,3 +217,13 @@ simple_value = (
     Output('result', bool, _('True means the operation was successful')),
     Output('value', unicode, flags=['no_display']),
 )
+
+# custom shim for commands like `trustconfig-show`,
+# `automember-default-group-*` which put stuff into output['value'] despite not
+# having primary key themselves. Designing commands like this is not a very
+# good practice, so please do not use this for new code.
+simple_entry = (
+    summary,
+    Entry('result'),
+    Output('value', unicode, flags=['no_display']),
+)
diff --git a/ipaserver/plugins/automember.py b/ipaserver/plugins/automember.py
index dfa8498a6bd44352d854bff7f8eedaba8f731eef..8e9356a9d30c98b7c72735ffb9ac05c672546a0d 100644
--- a/ipaserver/plugins/automember.py
+++ b/ipaserver/plugins/automember.py
@@ -586,6 +586,7 @@ class automember_default_group_set(LDAPUpdate):
         ),
     ) + group_type
     msg_summary = _('Set default (fallback) group for automember "%(value)s"')
+    has_output = output.simple_entry
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
         dn = DN(('cn', options['type']), api.env.container_automember,
@@ -609,6 +610,7 @@ class automember_default_group_remove(LDAPUpdate):
 
     takes_options = group_type
     msg_summary = _('Removed default (fallback) group for automember "%(value)s"')
+    has_output = output.simple_entry
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
         dn = DN(('cn', options['type']), api.env.container_automember,
@@ -644,6 +646,7 @@ class automember_default_group_show(LDAPRetrieve):
     obj_name = 'automember_default_group'
 
     takes_options = group_type
+    has_output = output.simple_entry
 
     def pre_callback(self, ldap, dn, attrs_list, *keys, **options):
         dn = DN(('cn', options['type']), api.env.container_automember,
diff --git a/ipaserver/plugins/trust.py b/ipaserver/plugins/trust.py
index 18a7cfd3447143393fb087f840edb406e98285a4..97c828dfc104c4ce373a6e43858bb04693c6569f 100644
--- a/ipaserver/plugins/trust.py
+++ b/ipaserver/plugins/trust.py
@@ -1323,6 +1323,7 @@ class trustconfig_show(LDAPRetrieve):
     __doc__ = _('Show global trust configuration.')
 
     takes_options = LDAPRetrieve.takes_options + (_trust_type_option,)
+    has_output = output.simple_entry
 
     def execute(self, *keys, **options):
         result = super(trustconfig_show, self).execute(*keys, **options)
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to