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

[Jan Cholasta]

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.

>
> 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).

   * 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

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

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

>
> === 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

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

Honza

-- 
Jan Cholasta