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.

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

--
Jan Cholasta

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

Reply via email to