[Freeipa-devel] [RFE] Warnings and client capabilities (Was: [PATCH] 0062 Don't crash when server returns extra output)

[Petr Viktorin]

On 10/25/2012 04:55 PM, Jan Cholasta wrote:
> Hi,
>
> On 23.10.2012 17:57, Petr Viktorin wrote:
>> Here is a draft design document for ticket 2732.
>> Please comment on both the feature itself, and on how to write design
>> documents.
>> Petr¹, please add how the UI should handle this.
>>
>> == Ticket summary ([https://fedorahosted.org/freeipa/ticket/2732
>> #2732]) ==
>>
>> Currently the only way to display a warning on client is to raise
>> NonFatalError. This is not particularly good, as it mutes normal command
>> output and only one warning can be displayed at a time.
>>
>> Provide a mechanism for displaying arbitrary number of warnings and
>> other messages on clients along with the normal command output.
>>
>> == Additional problem ==
>>
>> The client validates the response it receives from the server. If it
>> gets any extra items, the validation fails. Relaxing our validation is
>> not an option. To support older clients, we need a mechanism for
>> backwards-incompatible extensions to the API.
>>
>> == Solution ==
>>
>> === Backend ===
>>
>> Introduce a "capability" mechanism for backwards-incompatible API
>> changes. The first capability will be "warnings": a client with this
>> capability can get an extra key in the response dictionary, "warnings",
>> which contains a list of non-fatal error messages.
>>
>> Capabilities are determined by API version. The version is linear; it is
>> not possible for a client to support any custom subset of capabilities.
>>
>> If a client does not send an API version number, we will assume this is
>> a testing client (such as a curl from the command line). Such a client
>> is assumed to have all capabilities, but it will always receive a
>> warning saying that forward compatibility is not guaranteed if the
>> version number is not sent.
>
> I think this has potential to break stuff. An old client unaware of
> capabilities might not send version and as a result receive unexpected
> data, which might trigger some error.

Our client always sends the version. We already use it to detect clients 
that are too old or too new. Clients that do not send it are not 
protected from breakage. They are not using the API correctly.

Also, if any other clients currently exist, they're unlikely to validate 
the results as strictly as we do, so an extra "warnings" entry shouldn't 
hurt them.

>> Capabilities will be recorded in API.txt. When a new one is added, the
>> API version must be incremented.
>>
>> All Commands will be updated to explicitly list 'version?' as one of
>> their options in API.txt (most already do, and all take it).
>>
>> A missing version option will be set as part of validation/filling in
>> defaults, so execute() will always get it.
>>
>> Helper methods will be available for checking a capability and for
>> adding a warning to the result:
>>
>>      add_warning(version, result, _("What you're doing is a bad idea"))
>>
>> will be equivalent to:
>>
>>      if client_has_capability(version, 'warnings'):
>>          result.setdefault('warnings', []).append(_("What you're doing
>> is a bad idea"))
>>
>
> Here's a couple of things to consider:
>
>    * There should be an API that doesn't require the version and result
> arguments, because they might not be at hand when a warning needs to be
> issued (e.g. in pre/post command callbacks or utility functions).

I don't see how this can be done without introducing global/per-thread 
state or reworking the system.

Not having access to the output is a limitation of the callbacks. IMO 
it's a symptom of a deeper problem: they don't have access to any state 
other than what the current callbacks need, and we can't easily make 
more stuff available to them.
I think this problem is orthogonal to this RFE and should be solved 
separately.

(Note: the version is part of options, so callbacks do have acccess to it.)

>    * There should be more message levels than just "warning". I think it
> should be at least "error", "warning", "info".
>
>      I have included "error" here, because currently we can't send more
> than one error back to the client, but it might be handy:
>
>      # ipa command --opt1 ugly --opt2 uglier
>      ipa: ERROR: invalid 'opt1': must be an integer
>      ipa: ERROR: invalid 'opt2': must be at least 10 characters

Unfortunately, XML-RPC doesn't allow any extra data in error messages, 
it's just errno and text. So we can't give multiple errors to the client.

Regarding the "info" level, I don't see a use for it. Information is 
what the result dictionary already contains.

>    * We have numeric codes for errors, perhaps we should have them for
> warnings as well.

Yeah, we can do that for consistency's sake:
The warning info will be a dict with the code, name and message (same as 
we treat errors in JSON).
In IPA code, warnings will be objects. The classes will live in a new 
module, ipalib.warnings, modeled after ipalib.errors.
The add_warning API will become `add_warning(version, result, 
BadIdeaWarning())`.

>    * We might want to integrate this with the standard Python warnings
> module <http://docs.python.org/library/warnings.html>.

That possible, but I don't see what it would buy us.
Also, I think we'd have to massage it in nontrivial ways to do what we 
need (i.e. collect warnings in a list and put them in the result dict, 
but only in Command.execute()). Note that warnings.catch_warnings is not 
thread-safe.

However, subclassing our warnings from warnings.Warning sounds good.

>
>>
>> === Frontend ===
>>
>> In the CLI, warnings will be printed after the normal command
>> output.Example (from [https://fedorahosted.org/freeipa/ticket/2563
>> #2563])
>>
>>    # ipa dnsrecord-add --ttl=555 localnet r1 --txt-rec=TEST
>>      Record name: r1
>>      Time to live: 555
>>      A record: 1.2.3.4
>>      TXT record: TEST
>>     Warnings:
>>         It's not possible to have two records with same name and
>> different TTL values.
>
> I would prefer if we did the same thing as we do for errors here, so
> that warnings are printed to stderr and can be clearly distinguished
> from normal command output:
>
> # ipa dnsrecord-add --ttl=555 localnet r1 --txt-rec=TEST
> ipa: WARNING: It's not possible to have two records with same name and
> different TTL values
>    Record name: r1
>    Time to live: 555
>    A record: 1.2.3.4
>    TXT record: TEST

Yes, that's a good idea.

>
>>
>> The Web UI will display warnings in a modal message box.
>>
>
> Honza
>


-- 
Petr³