ipalib.frontend: Do API version check before converting arguments

This results in the proper message being shown if the client sends
an option the server doesn't have yet.

It also adds the check to commands that override run() but not __call__,
such as `ipa ping`, and to commands run on the server. Adjust tests
for these changes.

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

Reviewed-By: Martin Kosek <mkosek@redhat.com>

3 files changed, 24 insertions, 22 deletions

diff --git a/ipalib/__init__.py b/ipalib/__init__.py
index 6895b46d4..6b1ec573a 100644
--- a/ipalib/__init__.py
+++ b/ipalib/__init__.py
@@ -140,7 +140,7 @@ implement a ``run()`` method, like this:
 >>> api = create_api()
 >>> api.register(my_command)
 >>> api.finalize()
->>> api.Command.my_command() # Call your command
+>>> api.Command.my_command(version=u'2.47') # Call your command
 {'result': 'My run() method was called!'}
 
 When `frontend.Command.__call__()` is called, it first validates any arguments
@@ -359,7 +359,7 @@ False
 
 And yet we can call ``my_command()``:
 
->>> api.Command.my_command()
+>>> api.Command.my_command(version=u'2.47')
 {'result': 'Just my_command.forward() getting called here.'}
 
 
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 66be7939f..a9a3dfeaa 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -419,6 +419,11 @@ class Command(HasParam):
         XML-RPC and the executed an the nearest IPA server.
         """
         self.ensure_finalized()
+        version_provided = 'version' in options
+        if version_provided:
+            self.verify_client_version(unicode(options['version']))
+        else:
+            options['version'] = API_VERSION
         params = self.args_options_2_params(*args, **options)
         self.debug(
             'raw: %s(%s)', self.name, ', '.join(self._repr_iter(**params))
@@ -429,11 +434,13 @@ class Command(HasParam):
         self.debug(
             '%s(%s)', self.name, ', '.join(self._repr_iter(**params))
         )
-        if not self.api.env.in_server and 'version' not in params:
-            params['version'] = API_VERSION
         self.validate(**params)
         (args, options) = self.params_2_args_options(**params)
         ret = self.run(*args, **options)
+        if not version_provided and isinstance(ret, dict):
+            messages.add_message(
+                API_VERSION, ret,
+                messages.VersionMissing(server_version=API_VERSION))
         if (
             isinstance(ret, dict)
             and 'summary' in self.output
@@ -441,7 +448,7 @@ class Command(HasParam):
         ):
             ret['summary'] = self.get_summary_default(ret)
         if self.use_output_validation and (self.output or ret is not None):
-            self.validate_output(ret, options.get('version', API_VERSION))
+            self.validate_output(ret, options['version'])
         return ret
 
     def soft_validate(self, values):
@@ -744,17 +751,7 @@ class Command(HasParam):
         performs is executed remotely.
         """
         if self.api.env.in_server:
-            version_provided = 'version' in options
-            if version_provided:
-                self.verify_client_version(options['version'])
-            else:
-                options['version'] = API_VERSION
-            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.execute(*args, **options)
         return self.forward(*args, **options)
 
     def execute(self, *args, **kw):
@@ -1318,15 +1315,18 @@ class Method(Attribute, Command):
 
     >>> list(api.Method)
     ['user_add']
-    >>> api.Method.user_add() # Will call user_add.run()
+    >>> api.Method.user_add(version=u'2.88')  # Will call user_add.run()
     {'result': 'Added the user!'}
 
+    (The "version" argument is the API version to use.
+    The current API version can be found in ipalib.version.API_VERSION.)
+
     Second, because `Method` is a subclass of `Command`, the ``user_add``
     plugin can also be accessed through the ``api.Command`` namespace:
 
     >>> list(api.Command)
     ['user_add']
-    >>> api.Command.user_add() # Will call user_add.run()
+    >>> api.Command.user_add(version=u'2.88') # Will call user_add.run()
     {'result': 'Added the user!'}
 
     And third, ``user_add`` can be accessed as an attribute on the ``user``
@@ -1336,7 +1336,7 @@ class Method(Attribute, Command):
     ['user']
     >>> list(api.Object.user.methods)
     ['add']
-    >>> api.Object.user.methods.add() # Will call user_add.run()
+    >>> api.Object.user.methods.add(version=u'2.88') # Will call user_add.run()
     {'result': 'Added the user!'}
 
     The `Attribute` base class implements the naming convention for the
diff --git a/ipatests/test_xmlrpc/test_automount_plugin.py b/ipatests/test_xmlrpc/test_automount_plugin.py
index 2eee80b1f..bd55c8583 100644
--- a/ipatests/test_xmlrpc/test_automount_plugin.py
+++ b/ipatests/test_xmlrpc/test_automount_plugin.py
@@ -51,7 +51,7 @@ class AutomountTest(XMLRPC_test):
 
         mock_ui = MockTextui()
         command = api.Command['automountlocation_tofiles']
-        command.output_for_cli(mock_ui, res, self.locname)
+        command.output_for_cli(mock_ui, res, self.locname, version=u'2.88')
         expected_output = self.tofiles_output
         assert_deepequal(expected_output, u'\n'.join(mock_ui))
 
@@ -87,7 +87,8 @@ class AutomountTest(XMLRPC_test):
             # Feed the files to automountlocation_import & check
             master_file = u'%s/auto.master' % conf_directory
             automountlocation_import = api.Command['automountlocation_import']
-            res = automountlocation_import(self.locname, master_file)
+            res = automountlocation_import(self.locname, master_file,
+                                           version=u'2.88')
             assert_deepequal(dict(
                 result=dict(
                     keys=lambda k: k,
@@ -233,7 +234,8 @@ class test_automount(AutomountTest):
         """
         Test the `automountlocation_tofiles` command.
         """
-        res = api.Command['automountlocation_tofiles'](self.locname)
+        res = api.Command['automountlocation_tofiles'](self.locname,
+                                                       version=u'2.88')
         assert_deepequal(dict(
             result=dict(
                 keys={'auto.direct': ()},