On 01/04/2013 07:20 PM, Petr Viktorin wrote:
On 12/14/2012 09:04 AM, Jan Cholasta wrote:
On 13.12.2012 18:09, Petr Viktorin wrote:
On 12/13/2012 04:43 PM, Martin Kosek wrote:
On 12/13/2012 10:59 AM, Petr Viktorin wrote:
It's time to give this to another set of eyes :)
Design document: http://freeipa.org/page/V3/Messages
Ticket: https://fedorahosted.org/freeipa/ticket/2732
More info is in commit messages.
Because of https://fedorahosted.org/freeipa/ticket/3294, I needed to
change the
design document: when the client doesn't send the API version, it is
assumed
it's at a version before capabilities were introduced (i.e. 2.47).
The client still gets a warning if the version is missing. Except for
those
commands where IPA didn't send a version -- ping, cert-show, etc. --
the
warning wouldn't pass validation on old clients. (I'm assuming that
our client
is so far the only one that validates so strictly.)
I did a basic test of this patch and also quickly read through the
patches and
besides nitpicks (like unused inspect module in
tests/test_ipalib/test_messages.py in patch 0105) I did not find any
obvious
errors in the Python code.
Noted, will fix in future versions of the patch.
However, this patch breaks WebUI badly, I did not even get to a log in
screen.
Cooperation with Petr Vobornik will be needed. In my case, I got blank
screen
and Javascript error:
TypeError: IPA.messages.dialogs is undefined
https://vm-037.idm.lab.bos.redhat.com/ipa/ui/ipa.js
Line 1460
I assume this is related to the Internal Error that was returned in
the JSON call
{
"error": null,
"id": null,
"principal": "ad...@idm.lab.bos.redhat.com",
"result": {
"count": 5,
"results": [
{
"error": "an internal error has occurred",
"error_code": 903,
"error_name": "InternalError"
},
{
...
This can be reproduced with:
# curl -v -H "Content-Type:application/json" -H
"referer:https://`hostname`/ipa"; -H "Accept:applicaton/json"
--negotiate -u :
--cacert /etc/ipa/ca.crt -d
'{"method":"i18n_messages","params":[[],{}],"id":0}' -X POST
https://`hostname`/ipa/json
Good catch! The i18n_messages plugin already defines a "messages"
output. When I renamed this from "warnings" to "messages" I forgot to
check for clashes.
Since i18n_messages is an internal command only used by the Web UI, we
can rename its output to "texts" without breaking compatibility.
I'm attaching a preliminary fix (for both backend and UI), but hopefully
it won't be necessary, see below.
I am also not sure I like the requirement of a specific version option
to be
always passed. I would prefer that missing version option would mean
"I use the
most recent version of API" instead - it would make the custom
JSONRPC/XMLRPC
calls easier to use.
But since the version option was not being sent for some commands, we
may not
have a choice anyway if we do not want to break old clients in case we
add some
capabilities to these commands.
I see three other options, all worse:
- Do not use capabilities for the affected commands, meaning no new
functionality can be added to them (and by extension, no new
functionality common to all commands can be added).
- Treat a missing version as the current version
- Break backwards compatibility
And one possibly better (thanks to Petr¹ and Martin for opening my eyes
off-list!):
- Deprecate XML-RPC. All XML-RPC requests would be pinned to current
version (2.47). Capabilities/messages would only apply to JSON-RPC.
This would also allow us to solve the above name-clashing problem
elegantly. Here is a reminder of what a JSON response looks like:
{
"error": null,
"id": 0,
"principal": "ad...@idm.lab.bos.redhat.com",
"result": {
"summary": "IPA server version 3.1.0GIT2e4bd02. API version
2.47"
},
"version": "3.1.0GIT2e4bd02"
}
A XML-RPC response only contains the "result" part of that.
So with JSON, we can put the messages in the top level, which is much
better design.
Custom info in the "top level" seems to be a violation of the JSON-RPC
spec. I'd rather not do more of those, so I'm withdrawing this idea.
XML-RPC sucks in other ways too. We already have a workaround for its
inability to attach extra info to errors (commit
88262a75ffe7a25640333dcc4da2100830cae821, Add instructions support to
PublicError).
I've opened a RFC here: https://fedorahosted.org/freeipa/ticket/3299.
+1, XML-RPC sucks. This should have been done a long time ago.
Honza
Here are new patches.
XML-RPC requests with missing version are assumed to be old (the version
before capabilities are introduced, 2.47). This takes care of backcompat
with clients with bug 3294.
JSON-RPC requests with missing version are assumed to be testing calls
(e.g. curl), they get behavior of the latest version but also a warning.
I've also added this info to the design doc.
It turns out that these patches don't depend on whether our client uses
XML-RPC or JSON-RPC. If/when it supports both, I'll be able to add some
extra unit tests.
Patch 106 had a minor conflict with master, attaching fixed version.
--
Petr³
From 617664d1e3a50d22f75d8b62e1c1862e10b69043 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Fri, 7 Dec 2012 10:54:07 -0500
Subject: [PATCH] Add client capabilities, enable messages
The API version the client sends can now be used to check what the client
expects or is capable of.
All version tests IPA does will be be named and listed in one module,
ipalib.capabilities, which includes a function to test a specific capability
against an API version.
Similarly to Python's __future__ module, capabilities.py also serves as
documentation of backwards-incompatible changes to the API.
The first capability to be defined is "messages". Recent enough clients can
accept a list of warnings or other info under the "messages" key in the
result dict.
If a JSON client does not send the API version, it is assumed this is a testing
client (e.g. curl from the command line). Such a client "has" all capabilities,
but it will always receive a warning mentioning that forward compatibility
is not guaranteed.
If a XML client does not send the API version, it is assumed it uses the API
version before capabilities were introduced. (This is to keep backwards
compatibility with clients containing bug https://fedorahosted.org/freeipa/ticket/3294)
Whenever a capability is added, the API version must be incremented.
To ensure that, capabilities are written to API.txt.
Design page: http://freeipa.org/page/V3/Messages
Ticket: https://fedorahosted.org/freeipa/ticket/2732
---
API.txt | 1 +
VERSION | 2 +-
ipalib/capabilities.py | 49 ++++++++++++++++++++++++++++++++
ipalib/frontend.py | 40 +++++++++++++++++++++-----
ipalib/messages.py | 6 ++++
ipaserver/rpcserver.py | 7 ++++-
makeapi | 4 ++
tests/test_ipalib/test_capabilities.py | 33 +++++++++++++++++++++
tests/test_ipalib/test_frontend.py | 43 +++++++++++++++++++++++++++-
tests/test_ipalib/test_messages.py | 29 +++++++++++++++++++
tests/test_xmlrpc/test_ping_plugin.py | 5 ++-
tests/test_xmlrpc/test_user_plugin.py | 3 +-
tests/test_xmlrpc/xmlrpc_test.py | 5 ++-
13 files changed, 211 insertions(+), 16 deletions(-)
create mode 100644 ipalib/capabilities.py
create mode 100644 tests/test_ipalib/test_capabilities.py
diff --git a/API.txt b/API.txt
index 31cf766312e444b3b7dfa8203d590d01bbccf7d8..0c619efd93d5df2ca869bf5de68bf2a4c81b1da7 100644
--- a/API.txt
+++ b/API.txt
@@ -3507,3 +3507,4 @@ option: Str('version?', exclude='webui')
output: Output('result', <type 'bool'>, None)
output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
output: Output('value', <type 'unicode'>, None)
+capability: messages 2.48
diff --git a/VERSION b/VERSION
index 61f578dbfc9415f6f94a6612f198218c5a5e0c9a..37af5ef73b74500e0cd7397fb2c109332c049bc6 100644
--- a/VERSION
+++ b/VERSION
@@ -89,4 +89,4 @@ IPA_DATA_VERSION=20100614120000
# #
########################################################
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=47
+IPA_API_VERSION_MINOR=48
diff --git a/ipalib/capabilities.py b/ipalib/capabilities.py
new file mode 100644
index 0000000000000000000000000000000000000000..101a9bc87da2525afc0d81b3bd32dc892b6277c0
--- /dev/null
+++ b/ipalib/capabilities.py
@@ -0,0 +1,49 @@
+# Authors:
+# Petr Viktorin <pvikt...@redhat.com>
+#
+# Copyright (C) 2012 Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+"""List of, and utilities for working with, client capabilities by API version
+
+The API version is given in ipapython.version.API_VERSION.
+
+This module defines a dict, ``capabilities``, that maps feature names to API
+versions they were introduced in.
+"""
+
+from distutils import version
+
+VERSION_WITHOUT_CAPABILITIES = u'2.47'
+
+capabilities = dict(
+ # messages: Server output may include an extra key, "messages", that
+ # contains a list of warnings and other messages.
+ # http://freeipa.org/page/V3/Messages
+ messages=u'2.48',
+
+)
+
+def client_has_capability(client_version, capability):
+ """Determine whether the client has the given capability
+
+ :param capability: Name of the capability to test
+ :param client_version: The API version string reported by the client
+ """
+
+ version_tuple = version.LooseVersion(client_version)
+
+ return version_tuple >= version.LooseVersion(capabilities[capability])
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index c27ff1389ecb16d060e87f1899eb5c87bbd14c47..06259823ccbf3fac9c56e91dba0b45ff32673dd1 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -23,18 +23,19 @@ Base classes for all front-end plugins.
import re
import inspect
+from distutils import version
+
+from ipapython.version import API_VERSION
+from ipapython.ipa_log_manager import root_logger
from base import lock, check_name, NameSpace
from plugable import Plugin, is_production_mode
from parameters import create_param, parse_param_spec, Param, Str, Flag, Password
from output import Output, Entry, ListOfEntries
from text import _, ngettext
-
from errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
- RequiresRoot, VersionError, RequirementError, OptionError)
-from errors import InvocationError
+ RequiresRoot, VersionError, RequirementError, OptionError, InvocationError)
from constants import TYPE_ERROR
-from ipapython.version import API_VERSION
-from distutils import version
+from ipalib import messages
RULE_FLAG = 'validation_rule'
@@ -740,11 +741,17 @@ class Command(HasParam):
performs is executed remotely.
"""
if self.api.env.in_server:
- if 'version' in options:
+ version_provided = 'version' in options
+ if version_provided:
self.verify_client_version(options['version'])
else:
options['version'] = API_VERSION
- return self.execute(*args, **options)
+ result = self.execute(*args, **options)
+ if not version_provided:
+ messages.add_message(
+ API_VERSION, result,
+ messages.VersionMissing(server_version=API_VERSION))
+ return result
return self.forward(*args, **options)
def execute(self, *args, **kw):
@@ -914,7 +921,7 @@ class Command(HasParam):
nice, dict, type(output), output)
)
expected_set = set(self.output)
- actual_set = set(output)
+ actual_set = set(output) - set(['messages'])
if expected_set != actual_set:
missing = expected_set - actual_set
if missing:
@@ -945,6 +952,21 @@ class Command(HasParam):
continue
yield param
+ def log_messages(self, output, logger):
+ logger_functions = dict(
+ debug=logger.debug,
+ info=logger.info,
+ warning=logger.warning,
+ error=logger.error,
+ )
+ for message in output.get('messages', ()):
+ try:
+ function = logger_functions[message['type']]
+ except KeyError:
+ logger.error('Server sent a message with a wrong type')
+ function = logger.error
+ function(message.get('message'))
+
def output_for_cli(self, textui, output, *args, **options):
"""
Generic output method. Prints values the output argument according
@@ -963,6 +985,8 @@ class Command(HasParam):
rv = 0
+ self.log_messages(output, root_logger)
+
order = [p.name for p in self.output_params()]
if options.get('all', False):
order.insert(0, 'dn')
diff --git a/ipalib/messages.py b/ipalib/messages.py
index 93b9812d7232fe4a357de50ddb2cbd613c39807c..a7f138baff08a1307293d40235d7705211b44af8 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -35,6 +35,12 @@ from inspect import isclass
from ipalib.constants import TYPE_ERROR
from ipalib.text import _ as ugettext
from ipalib.text import Gettext, NGettext
+from ipalib.capabilities import client_has_capability
+
+
+def add_message(version, result, message):
+ if client_has_capability(version, 'messages'):
+ result.setdefault('messages', []).append(message.to_dict())
def process_message_arguments(obj, format=None, message=None, **kw):
diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index 8bce48bea10481bb752bb162479555facdbbd313..ac86fadde05d367170802e87960e13fd8eee3ea2 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -35,7 +35,7 @@ from decimal import Decimal
import urlparse
import time
-from ipalib import plugable
+from ipalib import plugable, capabilities
from ipalib.backend import Executioner
from ipalib.errors import PublicError, InternalError, CommandError, JSONError, ConversionError, CCacheError, RefererError, InvalidSessionPassword, NotFound, ACIError, ExecutionError
from ipalib.request import context, Connection, destroy_context
@@ -732,6 +732,11 @@ class xmlserver(WSGIExecutioner, HTTP_Status, KerberosSession):
def unmarshal(self, data):
(params, name) = xml_loads(data)
(args, options) = params_2_args_options(params)
+ if 'version' not in options:
+ # Keep backwards compatibility with client containing
+ # bug https://fedorahosted.org/freeipa/ticket/3294:
+ # If `version` is not given in XML-RPC, assume an old version
+ options['version'] = capabilities.VERSION_WITHOUT_CAPABILITIES
return (name, args, options, None)
def marshal(self, result, error, _id=None):
diff --git a/makeapi b/makeapi
index 8981a97c2128c6ab6aab5c6507b34536484496e0..7b75c7ab9f01eac1e7e502165d9a04d7444c27a8 100755
--- a/makeapi
+++ b/makeapi
@@ -32,6 +32,7 @@ from ipalib import api
from ipalib.parameters import Param
from ipalib.output import Output
from ipalib.text import Gettext, NGettext
+from ipalib.capabilities import capabilities
API_FILE='API.txt'
@@ -211,6 +212,9 @@ def make_api():
fd.write('option: %s\n' % param_repr(o))
for o in sorted(cmd.output(), key=operator.attrgetter('name')):
fd.write('output: %s\n' % param_repr(o))
+ for name, version in sorted(
+ capabilities.items(), key=lambda (k, v): (v, k)):
+ fd.write('capability: %s %s\n' % (name, version))
fd.close()
return 0
diff --git a/tests/test_ipalib/test_capabilities.py b/tests/test_ipalib/test_capabilities.py
new file mode 100644
index 0000000000000000000000000000000000000000..4533999a8544fca1b1e56e8f556bcab971923ca8
--- /dev/null
+++ b/tests/test_ipalib/test_capabilities.py
@@ -0,0 +1,33 @@
+# Authors:
+# Petr Viktorin <pvikt...@redhat.com>
+#
+# Copyright (C) 2012 Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+"""
+Test the `ipalib.errors` module.
+"""
+
+from ipalib.capabilities import capabilities, client_has_capability
+
+
+def test_client_has_capability():
+ assert capabilities['messages'] == u'2.48'
+ assert client_has_capability(u'2.48', 'messages')
+ assert client_has_capability(u'2.50', 'messages')
+ assert client_has_capability(u'3.0', 'messages')
+ assert not client_has_capability(u'2.11', 'messages')
+ assert not client_has_capability(u'0.1', 'messages')
diff --git a/tests/test_ipalib/test_frontend.py b/tests/test_ipalib/test_frontend.py
index 4b4735599c94f81fb49ff671ad73ce8801c43d2a..3a540608d55a56ac6f660cbc318df6588bec192d 100644
--- a/tests/test_ipalib/test_frontend.py
+++ b/tests/test_ipalib/test_frontend.py
@@ -27,7 +27,7 @@ from tests.util import assert_equal
from ipalib.constants import TYPE_ERROR
from ipalib.base import NameSpace
from ipalib import frontend, backend, plugable, errors, parameters, config
-from ipalib import output
+from ipalib import output, messages
from ipalib.parameters import Str
from ipapython.version import API_VERSION
@@ -619,6 +619,47 @@ class test_Command(ClassChecker):
assert o.run.im_func is self.cls.run.im_func
assert ('forward', args, kw) == o.run(*args, **kw)
+ def test_messages(self):
+ """
+ Test correct handling of messages
+ """
+ class TestMessage(messages.PublicMessage):
+ type = 'info'
+ format = 'This is a message.'
+ errno = 1234
+
+ class my_cmd(self.cls):
+ def execute(self, *args, **kw):
+ result = {'name': 'execute'}
+ messages.add_message(kw['version'], result, TestMessage())
+ return result
+
+ def forward(self, *args, **kw):
+ result = {'name': 'forward'}
+ messages.add_message(kw['version'], result, TestMessage())
+ return result
+
+ args = ('Hello,', 'world,')
+ kw = dict(how_are='you', on_this='fine day?', version=API_VERSION)
+
+ expected = [TestMessage().to_dict()]
+
+ # Test in server context:
+ (api, home) = create_test_api(in_server=True)
+ api.finalize()
+ o = my_cmd()
+ o.set_api(api)
+ assert o.run.im_func is self.cls.run.im_func
+ assert {'name': 'execute', 'messages': expected} == o.run(*args, **kw)
+
+ # Test in non-server context
+ (api, home) = create_test_api(in_server=False)
+ api.finalize()
+ o = my_cmd()
+ o.set_api(api)
+ assert o.run.im_func is self.cls.run.im_func
+ assert {'name': 'forward', 'messages': expected} == o.run(*args, **kw)
+
def test_validate_output_basic(self):
"""
Test the `ipalib.frontend.Command.validate_output` method.
diff --git a/tests/test_ipalib/test_messages.py b/tests/test_ipalib/test_messages.py
index 4d24c8488d9f7b1834a16e300c96e0bb2acb1b58..3a7cf5d3cfb8f609c992c85cb034b69b5ea86cf1 100644
--- a/tests/test_ipalib/test_messages.py
+++ b/tests/test_ipalib/test_messages.py
@@ -22,6 +22,7 @@ Test the `ipalib.messages` module.
"""
from ipalib import messages
+from ipalib.capabilities import capabilities
from tests.test_ipalib import test_errors
class HelloMessage(messages.PublicMessage):
@@ -56,3 +57,31 @@ def test_to_dict():
)
assert HelloMessage(greeting='Hello', object='world').to_dict() == expected
+
+
+def test_add_message():
+ result = {}
+
+ assert capabilities['messages'] == u'2.48'
+
+ messages.add_message(u'2.48', result,
+ HelloMessage(greeting='Hello', object='world'))
+ messages.add_message(u'2.1', result,
+ HelloMessage(greeting="'Lo", object='version'))
+ messages.add_message(u'2.50', result,
+ HelloMessage(greeting='Hi', object='version'))
+
+ assert result == {'messages': [
+ dict(
+ name='HelloMessage',
+ type='info',
+ message='Hello, world!',
+ code=1234,
+ ),
+ dict(
+ name='HelloMessage',
+ type='info',
+ message='Hi, version!',
+ code=1234,
+ )
+ ]}
diff --git a/tests/test_xmlrpc/test_ping_plugin.py b/tests/test_xmlrpc/test_ping_plugin.py
index 284aed54f314f847f678e7033a4a55bfa2d2fa2f..3673b436f8dc0a83cfc9f1e585300fa740c02197 100644
--- a/tests/test_xmlrpc/test_ping_plugin.py
+++ b/tests/test_xmlrpc/test_ping_plugin.py
@@ -21,9 +21,10 @@
Test the `ipalib/plugins/ping.py` module, and XML-RPC in general.
"""
-from ipalib import api, errors, _
-from tests.util import assert_equal, Fuzzy
+from ipalib import api, errors, messages, _
+from tests.util import Fuzzy
from xmlrpc_test import Declarative
+from ipapython.version import API_VERSION
class test_ping(Declarative):
diff --git a/tests/test_xmlrpc/test_user_plugin.py b/tests/test_xmlrpc/test_user_plugin.py
index 50630a0f9f8073e9130aa027c32323558b248bf8..a61db23d501c4f25e860a7cc1dabfe3b01ddffcf 100644
--- a/tests/test_xmlrpc/test_user_plugin.py
+++ b/tests/test_xmlrpc/test_user_plugin.py
@@ -23,11 +23,12 @@
Test the `ipalib/plugins/user.py` module.
"""
-from ipalib import api, errors
+from ipalib import api, errors, messages
from tests.test_xmlrpc import objectclasses
from tests.util import assert_equal, assert_not_equal
from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid, fuzzy_password, fuzzy_string, fuzzy_dergeneralizedtime
from ipapython.dn import DN
+from ipapython.version import API_VERSION
user1=u'tuser1'
user2=u'tuser2'
diff --git a/tests/test_xmlrpc/xmlrpc_test.py b/tests/test_xmlrpc/xmlrpc_test.py
index 7c32be0db31d69373f988a2bb1ec7171679b36ae..03680228df69e0990324dd548ca80f8b03e78f17 100644
--- a/tests/test_xmlrpc/xmlrpc_test.py
+++ b/tests/test_xmlrpc/xmlrpc_test.py
@@ -25,9 +25,9 @@ import sys
import socket
import nose
from tests.util import assert_deepequal, Fuzzy
-from ipalib import api, request
-from ipalib import errors
+from ipalib import api, request, errors
from ipalib.x509 import valid_issuer
+from ipapython.version import API_VERSION
# Matches a gidnumber like '1391016742'
@@ -256,6 +256,7 @@ class Declarative(XMLRPC_test):
def check(self, nice, desc, command, expected, extra_check=None):
(cmd, args, options) = command
+ options.setdefault('version', API_VERSION)
if cmd not in api.Command:
raise nose.SkipTest('%r not in api.Command' % cmd)
if isinstance(expected, errors.PublicError):
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
