On Mon, Oct 22, 2012 at 08:21:32PM +0100, Dato Simó wrote:
> Hi,
>
> I found the following situation when trying to add OpCodes for setting
> and deleting tags to OpCodes.hs.
>
> OpTagsSet and OpTagsDel take two required parameters ("kind" and
> "tags"), and then both take "name", which is defined in Python as:
>
> # Name is only meaningful for nodes and instances
> ("name", ht.NoDefault, ht.TMaybeString,
> "Name of object where tag(s) should be added"),
>
> The resulting combination, NoDefault + TMaybeString, is that the field
> can be None, but even in that case it must be explicitly specified
> (i.e., it must be present in the serialization even if as null). The
> CLI, for example, solves this by using "cluster" as the value for name
> in the case of `gnt-cluster add-tags`, though it could have as well used
> None (I may send a patch for that, actually).
TBH, it sends "cluster" in order to have a clear type. But maybe this is
the wrong approach.
The problem stems from the fact that the (kind, name) is in reality a
single type, but we can't express it as such at json level nicely:
data What = Cluster
| Node String
| Instance String
| Group String
…
I think that NoDefault + MaybeString is the correct type then, even if a
bit strange.
> From the Haskell point of view, however, defining this field with
> optionalField is not quite correct, since a value of Nothing will result
> in the field not getting included in the serialization (using a
> non-empty string as dummy value, as the CLI does, would work, of
> course), and thus the job failing.
Ack.
> Then I thought of defining the field as required, but use a type of
> Maybe String, instead of String. But (a) THH.hs is already using Maybe
> internally for optional fields; and (b) at the moment it just gets
> serialized into a <dict> structure, e.g. ("Just", "xxx"), or ("Nothing",
> null).
You can't use a Maybe field, because JSON includes (bad) default
instances for Maybe, which result in the above. If you were to go this
way (which I don't recommend), you would have to use a newtype which has
a separate JSON instance.
> I'm trying to think what the best way to proceed would be, and I see
> several options (from the simplest and less correct, to the more complex
> and possibly more correct):
>
> (1) Change the Python definition to specify a default value of None.
> (OpTagsGet/Set/Del are one of the two groups that uses NoDefault +
> TMaybeString, the other one being Op{Repair,Modify}NodeStorage).
>
> (2) Define the field as required, forcing the programmer to always
> specify a value, even if it's just a dummy one.
>
> (3) Define the field as optional, but still expect the programmer to
> specify a dummy value with Just even in the cases in which it's
> not required.
Uh, very ugly.
> (4) Have an additional flag in the definition of Field that says if it
> should always be included in the serialization, even if the value
> is Nothing.
(4b) Always serialise all optional fields as JSNull. Would 'inflate' the
opcodes…
> (However, at least in the case of "name" above, we would want to
> combine this with making the field optional, so that one would not
> have to specify a dummy string; in which case, there is a
> difference in API use, in which a Python programmer that forgot
> to specify the name would get a PreReq failure error, and a
> Haskell programmer would get a runtime error – NoneType has no
> upper() method or so.)
I think this would be a deserialisation error, not a 'upper' method
error. Anyway.
> (5) [This one probably wouldn't work] Allow the OpCode definitions in
> Haskell to have an explicit type of Maybe x, so that if the field
> is not marked as optional, it will still get serialized as null if
> the value is Nothing.
This would complicate the reasoning about field types somewhat, so I
don't like it.
> (6) ... other?
>
>
> If we don't find a clear winner, or the implementation is complex and
> would take some time, I would suggest using (2) for now, since there's
> precedent for it in the Python codebase already.
I'm tempted for (2), but (2) is not clean, since the dummy value is very
ugly.
(4) seems easily doable, and I can send a patch soon if we agree on
that…
thanks,
iustin
