[pve-devel] [PATCH manager v2 1/2] ui/Parser: add generic functions property_strings

[Dominik Csapak]

On 08/02/2018 03:13 PM, Thomas Lamprecht wrote:
> Am 08/02/2018 um 02:51 PM schrieb Dominik Csapak:
>> On 08/02/2018 12:04 PM, Thomas Lamprecht wrote:
>>> Am 08/01/2018 um 08:29 PM schrieb Stoiko Ivanov:
>>>
>>> great for tackling this! some comments inline..
>>>
>>>> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
>>>> ---
>>>>    www/manager6/Parser.js | 43
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 43 insertions(+)
>>>>
>>>> diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
>>>> index 13dce766..df2be5dc 100644
>>>> --- a/www/manager6/Parser.js
>>>> +++ b/www/manager6/Parser.js
>>>> @@ -45,6 +45,49 @@ Ext.define('PVE.Parser', { statics: {
>>>>               value === 'true';
>>>>        },
>>>>    +    parsePropertyString: function(value, defaultKey) {
>>>> +    var res = {},
>>>> +    errors = false;
>>>
>>> please indent errors here, else it may seem like errors is already
>>> defined (e.g., as global variable) a be a bit confusing...
>>> Do:
>>> var res = {},
>>>       errors = false;
>>>
>>>> +
>>>> +    Ext.Array.each(value.split(','), function(p) {
>>>> +        var kv = p.split('=', 2);
>>>> +        if (Ext.isDefined(kv[1])) {
>>>> +        res[kv[0]] = kv[1];
>>>> +        } else if (Ext.isDefined(defaultKey)) {
>>>> +        if (Ext.isDefined(res[defaultKey])) {
>>>> +            errors = true;
>>>> +            return false; //break
>>>> +        }
>>>> +        res[defaultKey] = kv[0];
>>>> +        } else {
>>>> +        errors = true;
>>>> +        return false; // break
>>>> +        }
>>>> +    });
>>>> +
>>>> +    if (errors) {
>>>> +        return;
>>>
>>> Hmm, maybe throw an exception? the above cases suggest a situation which
>>> continuing with and empty result may not be desired.
>>> You may want to omit the errors variable and just throw directly above.
>>>
>>> throw "there can only be one defaultKey"
>>> throw "invalid property string"
>>>
>>> (as suggestion)
>>
>> all parsers in Parser.js do this and it does not make sense to throw
>> an exception here, because in that case the user never notices
>> except that the code stopped working
> sounds like quite a big notification that something is not OK ;-)
> IMO, a property string has this hard asserts as it get passed from the
> backend, if those do not hold it like an BUG_ON in the kernel, and
> execution _should_ be stopped.

i agree, but it should be stopped in an obvious way and not only
a log in the browser console, nobody has open normally
also, depending on where we parse, we might want to clean something
up/close a window/etc. and so far we do not use
try {} catch {} at all

a user reporting the msgbox 'could not parse drive options' is very
useful us, since we instantly know that the drive parser in the gui
is out of sync with the backend

a user reporting 'editing a disk does not save values' is really subtle
since this can be many things

so far the only places we throw an exception (afair) are when
using components to make sure all necessary parameters are set
e.g. when it needs a vmid or nodename

> 
>>
>> in the case of parseXXX the caller is responsible to check
>> if the returned value is an object or undefined
>> and handle a parse error (be it a msg box or something else)
>>
> What would you show in the message box? Effectively you have to skip
> everything you could do there, because you have no valid state, thus
> Read Copy Update cannot be done safely, as UI and backend are out of
> sync - and that operation is the normal use case for this helper.
> 
> at least I'd then save the exception message in error and log it, so
> that if an user reports something resulting from this, which can be
> really subtle you get a hind in the console...
>