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

[Jan Cholasta]

On 26.10.2012 16:35, Petr Viktorin wrote:
> 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.

It's better to be safe than sorry, IMHO.

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

We already use per-thread state frequently, we can use it here as well.

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

XML-RPC is not the only transport mechanism we have, so I don't think 
this XML-RPC-centric thinking is appropriate. If XML-RPC does not have 
native support for this, we certainly can do it some other way.

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

Debugging comes to mind. My point is that this thing should be 
extensible, so should we require a new message category in future, it 
can be added easily.

>
>>    * 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())`.

Sounds good.

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

Well, using Python's standard interface for warnings would have been 
nice, but if it can't be done properly, I think we can live without it.

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


-- 
Jan Cholasta